Re: [PATCHv2] power_supply: Add support for Richtek rt9455 battery charger

[Krzysztof KozÅowski]

2015-05-05 0:22 GMT+09:00 Laurentiu Palcu <laurentiu.palcu@xxxxxxxxx>:
> Hi Anda,
>
> Can you please use a decent mail client to reply to emails? It was
> really hard for me to figure out which were my comments and which were
> yours... And I tested 3 different clients: mutt, evolution and
> outlook(the web app).
>
> More comments inline.
>
> laurentiu
>
> On Mon, May 04, 2015 at 05:12:07PM +0300, Nicolae, Anda-maria wrote:

(...)

>> > +
>> (...)
>> > +
>> > +static int rt9455_charger_set_voltage_max(struct rt9455_info *info,
>> > +                                       const union power_supply_propval *val)
>> > +{
>> > +     return rt9455_set_field_val(info, F_VMREG,
>> > +                                 rt9455_vmreg_values,
>> > +                                 ARRAY_SIZE(rt9455_vmreg_values),
>> > +                                 val->intval);
>> > +}
>> > +
>> > +static int rt9455_charger_set_property(struct power_supply *psy,
>> > +                                    enum power_supply_property psp,
>> > +                                    const union power_supply_propval *val)
>> > +{
>> > +     struct rt9455_info *info = power_supply_get_drvdata(psy);
>> > +
>> > +     switch (psp) {
>> > +     case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>> > +             return rt9455_charger_set_current(info, val);
>> > +     case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>> > +             return rt9455_charger_set_voltage(info, val);
>> Personally, I'm against having these properties writable. A user might
>> play with the charging voltage or current and then complain that the
>> battery does not charge at all, or it takes ages to charge. I'd leave
>> these to be set by manufacturer through DT/ACPI. But, it's just an
>> opinion.
>>
>> If the user really wants to damage his/her battery, he/she can charge
>> the values from DT/ACPI.

No, the end user does not change the DT/ACPI. It just runs some Linux
distro or Android phone. The userspace does not change DT either. So
allowing userspace to set this through regular sysfs API is rather
exceptional.

>> Also, numerous charger drivers expose these
>> properties as writable.  I prefer leaving them as they are.

I found only one mainline driver which exposes these properties as
writeable: bq24190_charger.c. Others do not. Many other drivers uses
power supply class only as a reading interface. The charger is
actually operated through respective regulator driver.

Best regards,
Krzysztof
--
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/