Re: [PATCH] [RFC]power: battery: Generic battery driver using IIO

[anish singh]

On Tue, Sep 11, 2012 at 3:29 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 09/10/2012 05:40 PM, anish kumar wrote:
>> From: anish kumar <anish198519851985@xxxxxxxxx>
>>
>> This is the cleaned up code after the valuable inputs from
>> the Jonathan, Lars and Anton.
>>
>> I have tried to accomodate all the concerns however please
>> let me know incase something is missed out.
>>
>> Signed-off-by: anish kumar <anish198519851985@xxxxxxxxx>
>
> Hi,
>
>
>> ---
>>  drivers/power/generic-adc-battery.c       |  431 +++++++++++++++++++++++++++++
>>  include/linux/power/generic-adc-battery.h |   33 +++
>>  2 files changed, 464 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/power/generic-adc-battery.c
>>  create mode 100644 include/linux/power/generic-adc-battery.h
>>
>> diff --git a/drivers/power/generic-adc-battery.c b/drivers/power/generic-adc-battery.c
>> new file mode 100644
>> index 0000000..3459740
>> --- /dev/null
>> +++ b/drivers/power/generic-adc-battery.c
>> @@ -0,0 +1,431 @@
> [...]
>> +
>> +static int generic_adc_bat_get_property(struct power_supply *psy,
>> +             enum power_supply_property psp,
>> +             union power_supply_propval *val)
>> +{
>> +     struct generic_adc_bat *adc_bat;
>> +     int scaleint, scalepart, iio_val, ret = 0;
>> +     long result = 0;
>> +
>> +     adc_bat = to_generic_bat(psy);
>> +     if (!adc_bat) {
>> +             dev_err(psy->dev, "no battery infos ?!\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     switch (psp) {
>> +     case POWER_SUPPLY_PROP_POWER_NOW:
>> +             ret = iio_read_channel_raw(adc_bat->channel[POWER],
>> +                             &iio_val);
>> +             ret = iio_read_channel_scale(adc_bat->channel[POWER],
>> +                             &scaleint, &scalepart);
>> +             if (ret < 0)
>> +                     return ret;
>> +             break;
>> +     case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +             ret = iio_read_channel_raw(adc_bat->channel[VOLTAGE],
>> +                             &iio_val);
>> +             ret = iio_read_channel_scale(adc_bat->channel[VOLTAGE],
>> +                             &scaleint, &scalepart);
>> +             if (ret < 0)
>> +                     return ret;
>> +             break;
>> +     case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +             ret = iio_read_channel_raw(adc_bat->channel[CURRENT],
>> +                             &iio_val);
>> +             ret = iio_read_channel_scale(adc_bat->channel[CURRENT],
>> +                             &scaleint, &scalepart);
>> +             if (ret < 0)
>> +                     return ret;
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     switch (ret) {
>> +     case IIO_VAL_INT:
>> +             result = iio_val * scaleint;
>> +             break;
>> +     case IIO_VAL_INT_PLUS_MICRO:
>> +             result = (s64)iio_val * (s64)scaleint +
>> +                     div_s64((s64)iio_val * (s64)scalepart, 1000000LL);
>> +             break;
>> +     case IIO_VAL_INT_PLUS_NANO:
>> +             result = (s64)iio_val * (s64)scaleint +
>> +                     div_s64((s64)iio_val * (s64)scalepart, 1000000000LL);
>> +             break;
>> +     }
>
> I think it's a good idea to factor the channel reading and scale conversion
> out into a helper function.
>
>> +
>> +     switch (psp) {
>> +     case POWER_SUPPLY_PROP_STATUS:
>> +             if (adc_bat->pdata.gpio_charge_finished < 0)
>
> gpio_is_valid
>
>> +                     val->intval = adc_bat->level == 100000 ?
>> +                             POWER_SUPPLY_STATUS_FULL : adc_bat->status;
>> +             else
>> +                     val->intval = adc_bat->status;
>> +             break;
> [...]
>> +     return ret;
>> +}
>> +
>
> [...]
>
>> +static void generic_adc_bat_work(struct work_struct *work)
>> +{
>> +     struct generic_adc_bat *adc_bat;
>> +     struct delayed_work *delayed_work;
>> +     int is_charged;
>> +     int is_plugged;
>> +
>> +     delayed_work = container_of(work,
>> +                             struct delayed_work, work);
>> +     adc_bat = container_of(delayed_work,
>> +                             struct generic_adc_bat, bat_work);
>> +
>> +     /* backup battery doesn't have current monitoring capability anyway */
>> +     is_plugged = power_supply_am_i_supplied(&adc_bat->psy);
>> +     adc_bat->cable_plugged = is_plugged;
>> +     if (is_plugged != adc_bat->was_plugged) {
>> +             adc_bat->was_plugged = is_plugged;
>> +             if (is_plugged)
>> +                     adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
>> +             else
>> +                     adc_bat->status = POWER_SUPPLY_STATUS_DISCHARGING;
>> +     } else {
>> +             if ((adc_bat->pdata.gpio_charge_finished >= 0) && is_plugged) {
>
> gpio_is_valid
>
>> +                     is_charged = charge_finished(adc_bat);
>> +                     if (is_charged)
>> +                             adc_bat->status = POWER_SUPPLY_STATUS_FULL;
>> +                     else
>> +                             adc_bat->status = POWER_SUPPLY_STATUS_CHARGING;
>> +             }
>> +     }
>> +
>> +     power_supply_changed(&adc_bat->psy);
>> +}
>
> [...]
>
>> +
>> +/*
>> + * compare the pdata->channel names with the predefined channels and
>> + * returns the index of the channel in channel_name array or -1 in the
>> + * case of not-found.
>> + */
>> +int channel_cmp(char *name)
>> +{
>> +     if (!strcmp(name, channel_name[VOLTAGE]))
>> +             return VOLTAGE;
>> +     else if (!strcmp(name, channel_name[CURRENT]))
>> +             return CURRENT;
>> +     else if (!strcmp(name, channel_name[POWER]))
>> +             return POWER;
>> +     else
>> +             return -1;
>> +}
>> +
>> +static int __devinit generic_adc_bat_probe(struct platform_device *pdev)
>> +{
>> +     struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
>> +     int ret, chan_index;
>> +
>> +     /* copying the battery name from platform data */
>> +     adc_bat.psy.name = pdata->battery_name;
>
> You should make a per device copy of adc_bat, otherwise there can only be
> one device at a time.
>
>> +
>> +     /* bootup default values for the battery */
>> +     adc_bat.volt_value = -1;
>> +     adc_bat.cur_value = -1;
>> +     adc_bat.cable_plugged = 0;
>> +     adc_bat.status = POWER_SUPPLY_STATUS_DISCHARGING;
>> +
>> +     /* calculate the total number of channels */
>> +     for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
>> +             ;
>> +
>> +     if (!chan_index) {
>> +             pr_err("atleast provide one channel\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* copying the static properties and allocating extra memory for holding
>> +      * the extra configurable properties received from platform data.
>> +     */
>> +     adc_bat.psy.properties = kzalloc(sizeof(bat_props)
>> +                                     + sizeof(int)*chan_index, GFP_KERNEL);
>> +     if (!adc_bat.psy.properties) {
>> +             ret = -ENOMEM;
>> +             goto first_mem_fail;
>> +     }
>> +     memcpy(adc_bat.psy.properties, bat_props, sizeof(bat_props));
>> +
>> +     /* allocating memory for iio channels */
>> +     adc_bat.channel = kzalloc(chan_index * sizeof(struct iio_channel),
>> +                                     GFP_KERNEL);
>> +     if (!adc_bat.channel) {
>> +             ret = -ENOMEM;
>> +             goto second_mem_fail;
>> +     }
>> +
>> +     /*
>> +      * getting channel from iio and copying the battery properties
>> +      * based on the channel set in the platform data.
>> +      */
>> +     for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++) {
>> +             int channel = channel_cmp(pdata->channel_name[chan_index]);
>> +             if (channel < 0) {
>> +                     ret = -EINVAL;
>> +                     goto second_mem_fail;
>> +             }
>> +
>> +             adc_bat.channel[chan_index] =
>> +             iio_channel_get(dev_name(&pdev->dev),
>> +                     pdata->channel_name[chan_index]);
>
> Just request the channel with the hardcoded channel names. There is no need
> to pass these in via platform data. If a channel is not found just skip it
> and continue with the next one. Only if 0 channels were found return an error.
>
>> +             if (IS_ERR(adc_bat.channel[chan_index])) {
>> +                     ret = PTR_ERR(adc_bat.channel[chan_index]);
>> +                     goto second_mem_fail;
>> +             }
>> +
>> +             memcpy(adc_bat.psy.properties+sizeof(bat_props),
>> +                     &dyn_props[channel], sizeof(dyn_props[channel]));
>
> You need to increase the offset for each new property.
>
>> +     }
>> +
>> +     ret = power_supply_register(&pdev->dev, &adc_bat.psy);
>> +     if (ret)
>> +             goto err_reg_fail;
>> +
>> +     INIT_DELAYED_WORK(&adc_bat.bat_work, generic_adc_bat_work);
>> +
>> +     if (gpio_is_valid(pdata->gpio_charge_finished)) {
>> +             ret = gpio_request(pdata->gpio_charge_finished, "charged");
>> +             if (ret)
>> +                     goto err_gpio;
>> +
>> +             ret = request_irq(gpio_to_irq(pdata->gpio_charge_finished),
>> +                             generic_adc_bat_charged,
>> +                             IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +                             "battery charged", &adc_bat);
>
> I'd make this request_any_context_irq, some IRQ expander use nested IRQs.
>
>> +             if (ret)
>> +                     goto err_gpio;
>> +     } else
>> +             goto err_gpio; /* let's bail out */
>> +
>> +     platform_set_drvdata(pdev, &adc_bat);
>> +     /* Schedule timer to check current status */
>> +     schedule_delayed_work(&adc_bat.bat_work,
>> +                     msecs_to_jiffies(0));
>> +     return 0;
>> +
>> +err_gpio:
>> +     power_supply_unregister(&adc_bat.psy);
>> +err_reg_fail:
>> +     for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
>> +             iio_channel_release(adc_bat.channel[chan_index]);
>> +     kfree(adc_bat.channel);
>> +second_mem_fail:
>> +     kfree(adc_bat.psy.properties);
>> +first_mem_fail:
>> +     return ret;
>> +}
>> +
>> +static int generic_adc_bat_remove(struct platform_device *pdev)
>> +{
>> +     int chan_index;
>> +     struct iio_battery_platform_data *pdata = pdev->dev.platform_data;
>> +     struct generic_adc_bat *adc_bat = to_generic_bat(pdata);
>> +
>> +     power_supply_unregister(&adc_bat->psy);
>> +
>> +     if (pdata->gpio_charge_finished >= 0) {
>
> gpio_is_valid
>
>> +             free_irq(gpio_to_irq(pdata->gpio_charge_finished), NULL);
>> +             gpio_free(pdata->gpio_charge_finished);
>> +     }
>> +
>> +     for (chan_index = 0; pdata->channel_name[chan_index]; chan_index++)
>> +             iio_channel_release(adc_bat->channel[chan_index]);
>> +     cancel_delayed_work(&adc_bat->bat_work);
>> +     return 0;
>> +}
>
Thanks Lars, all of your comments are valid and I will accordingly update.
I am waiting for some more review comments if there is any and will send
the updated code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/