Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221188 > unrolled thread

Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261  charger Andreas Dannenberg <dannenberg@ti.com> - 2015-09-09 04:30 +0200
    Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-09 06:20 +0200
      Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261  charger Andreas Dannenberg <dannenberg@ti.com> - 2015-09-09 19:40 +0200
        Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-10 01:50 +0200
          Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261  charger Laurentiu Palcu <laurentiu.palcu@intel.com> - 2015-09-10 17:00 +0200

#1221188 — Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

On Mon, Sep 07, 2015 at 12:57:56PM +0900, Krzysztof Kozlowski wrote:
> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
> >
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > Signed-off-by: Jennt TC <jenny.tc@intel.com>
> > ---
> >  .../devicetree/bindings/power/bq24261.txt          |   37 +
> >  drivers/power/Kconfig                              |    6 +
> >  drivers/power/Makefile                             |    1 +
> >  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
> >  include/linux/power/bq24261_charger.h              |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c
> >  create mode 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 0000000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> 
> Please split the bindings into separate patch (the first patch in patchset).
> 
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +    * "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> > +- ti,termination-current: integer, charge will be terminated when current in
> > +    constant-voltage phase drops below this value (in mA);
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> > +- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).
> 
> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover
> additional devices"
> http://www.spinics.net/lists/devicetree/msg92134.html
> 
> could you and Andreas figure out common bindings? Look at this:
> 
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,charge-current: integer, default charging current (in mA);
> 
> Different meaning and different units. This is madness! :)
> 
> In the same time you are adding TI-common bindings (not device
> specific, there is no prefix) so I would expect exactly the same
> bindings if it is possible.

Krzysztof, good observation! In bq2425x_charger.c (formerly known as
bq24257_charger.c :) that I worked on the unit used was uA. At that time
I did a quick check and there didn't seem to be a clear standard whether
to use the "micro" or "milli" units - different drivers use different
units. However there seems to be a tendency for the TI drivers to prefer
"milli" (bq2415x_charger.c, bq24735-charger.c)

Personally I think "milli" units are more appropriate for chargers since
they provide sufficient granularity and the numbers don't become too big
(try typing a voltage in the Volt-range in uV, it's very easy to get the
number of 0s wrong). However since the driver was already there I left
that aspect alone to preserve compatibility.

> > +
> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;
> 
> What is the requirement for thermal-sensing? Can it be enabled always?
> If yes, then this is not really a hardware property.
> 
> > +- ti,enable-user-write: boolean, if present driver will allow the user space
> > +    to control the charging current and voltage through sysfs;
> 
> This is not DT property. It does not describe hardware.

I take responsibility for this one :) In an earlier thread we were
discussing that it will be better to prevent sysfs write access to
"dangerous" charger parameters (charge voltage, current, ...) but I
argued that such access might still be desirable during development and
debug, and that a DT property could be used to allow such write access
(with the default being the charger properties being made read-only) -
this way giving the best of both worlds. However then when I updated the
bq24157_charger.c driver I ended up not really needing and implementing
this feature.

Out of curiosity, if it against best-practice to expose non hardware
properties, how would one best address the above use case? Compile time
switch? A sysfs property that allows write-enabling the other sysfs
properties (doesn't sound right). Or...?

--
Andreas Dannenberg
Texas Instruments Inc

> Best regards,
> Krzysztof
> 
> > +
> > +Example:
> > +
> > +bq25890 {
> > +        compatible = "ti,bq24261
> > +        reg = <0x6b>;
> > +
> > +        ti,charge-current = <1000>;
> > +        ti,charge-voltage = <4200>;
> > +        ti,termination-current = <128>;
> > +        ti,max-charge-current = <3000>;
> > +        ti,max-charge-voltage = <4350>;
> > +        ti,min-charge-temperature = <0>;
> > +        ti,max-charge-temperature = <60>;
> > +
> > +        ti,thermal-sensing;
> > +        ti,enable-user-write;
> > +};
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index f8758d6..cd47d0d 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -396,6 +396,12 @@ config CHARGER_BQ24190
> >         help
> >           Say Y to enable support for the TI BQ24190 battery charger.
> >
> > +config CHARGER_BQ24261
> > +       tristate "TI BQ24261 charger driver"
> > +       depends on I2C && EXTCON
> > +       help
> > +         Say Y here to enable support for TI BQ24261 battery charger.
> > +
> >  config CHARGER_BQ24257
> >         tristate "TI BQ24257 battery charger driver"
> >         depends on I2C
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index 5752ce8..bec8409 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -59,6 +59,7 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> >  obj-$(CONFIG_CHARGER_MAX8998)  += max8998_charger.o
> >  obj-$(CONFIG_CHARGER_BQ2415X)  += bq2415x_charger.o
> >  obj-$(CONFIG_CHARGER_BQ24190)  += bq24190_charger.o
> > +obj-$(CONFIG_CHARGER_BQ24261)  += bq24261_charger.o
> >  obj-$(CONFIG_CHARGER_BQ24257)  += bq24257_charger.o
> >  obj-$(CONFIG_CHARGER_BQ24735)  += bq24735-charger.o
> >  obj-$(CONFIG_CHARGER_BQ25890)  += bq25890_charger.o
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221224 — Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

On 09.09.2015 11:26, Andreas Dannenberg wrote:
> On Mon, Sep 07, 2015 at 12:57:56PM +0900, Krzysztof Kozlowski wrote:
>> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
>>>
>>> Add new charger driver support for BQ24261 charger IC.
>>>
>>> BQ24261 charger driver relies on extcon notifications to get the
>>> charger cable type and based on that it will set the charging parameters.
>>>
>>> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>> Signed-off-by: Jennt TC <jenny.tc@intel.com>
>>> ---
>>>  .../devicetree/bindings/power/bq24261.txt          |   37 +
>>>  drivers/power/Kconfig                              |    6 +
>>>  drivers/power/Makefile                             |    1 +
>>>  drivers/power/bq24261_charger.c                    | 1208 ++++++++++++++++++++
>>>  include/linux/power/bq24261_charger.h              |   27 +
>>>  5 files changed, 1279 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/power/bq24261.txt
>>>  create mode 100644 drivers/power/bq24261_charger.c
>>>  create mode 100644 include/linux/power/bq24261_charger.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/bq24261.txt b/Documentation/devicetree/bindings/power/bq24261.txt
>>> new file mode 100644
>>> index 0000000..25fc5c4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/bq24261.txt
>>> @@ -0,0 +1,37 @@
>>> +Binding for TI bq24261 Li-Ion Charger
>>
>> Please split the bindings into separate patch (the first patch in patchset).
>>
>>> +
>>> +Required properties:
>>> +- compatible: Should contain one of the following:
>>> +    * "ti,bq24261"
>>> +- reg: integer, i2c address of the device.
>>> +- ti,charge-current: integer, default charging current (in mA);
>>> +- ti,charge-voltage: integer, default charging voltage (in mV);
>>> +- ti,termination-current: integer, charge will be terminated when current in
>>> +    constant-voltage phase drops below this value (in mA);
>>> +- ti,max-charge-current: integer, maximum charging current (in mA);
>>> +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
>>> +- ti,min-charge-temperature: integer, minimum charging temperature (in DegC);
>>> +- ti,max-charge-temperature: integer, maximum charging temperature (in DegC).
>>
>> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover
>> additional devices"
>> http://www.spinics.net/lists/devicetree/msg92134.html
>>
>> could you and Andreas figure out common bindings? Look at this:
>>
>> +- ti,charge-current: integer, maximum charging current in uA.
>> +- ti,charge-current: integer, default charging current (in mA);
>>
>> Different meaning and different units. This is madness! :)
>>
>> In the same time you are adding TI-common bindings (not device
>> specific, there is no prefix) so I would expect exactly the same
>> bindings if it is possible.
> 
> Krzysztof, good observation! In bq2425x_charger.c (formerly known as
> bq24257_charger.c :) that I worked on the unit used was uA. At that time
> I did a quick check and there didn't seem to be a clear standard whether
> to use the "micro" or "milli" units - different drivers use different
> units. However there seems to be a tendency for the TI drivers to prefer
> "milli" (bq2415x_charger.c, bq24735-charger.c)
> 
> Personally I think "milli" units are more appropriate for chargers since
> they provide sufficient granularity and the numbers don't become too big
> (try typing a voltage in the Volt-range in uV, it's very easy to get the
> number of 0s wrong). However since the driver was already there I left
> that aspect alone to preserve compatibility.

I am fine with both units but milli indeed seems easier to judge by fast
looking and less error-prone. Whatever you choose - choose the same one. :)


>>> +
>>> +Optional properties:
>>> +- ti,thermal-sensing: boolean, if present thermal regulation will be enabled;
>>
>> What is the requirement for thermal-sensing? Can it be enabled always?
>> If yes, then this is not really a hardware property.
>>
>>> +- ti,enable-user-write: boolean, if present driver will allow the user space
>>> +    to control the charging current and voltage through sysfs;
>>
>> This is not DT property. It does not describe hardware.
> 
> I take responsibility for this one :) In an earlier thread we were
> discussing that it will be better to prevent sysfs write access to
> "dangerous" charger parameters (charge voltage, current, ...) but I
> argued that such access might still be desirable during development and
> debug, and that a DT property could be used to allow such write access
> (with the default being the charger properties being made read-only) -
> this way giving the best of both worlds. However then when I updated the
> bq24157_charger.c driver I ended up not really needing and implementing
> this feature.
> 
> Out of curiosity, if it against best-practice to expose non hardware
> properties, how would one best address the above use case? Compile time
> switch? A sysfs property that allows write-enabling the other sysfs
> properties (doesn't sound right). Or...?

Device Tree describes only the hardware configuration. This means that
it does not fully replaces platform data. Definitely DT should not be
used to configure debug options.

AFAIU, you have some board (always the same type of board) and:
1. during development you want to enable certain sysfs dangerous
operation/feature,
2. for production use you want to disable this feature?

This means that the "feature" would be present in mainline kernel always
so it is not possible to disallow the use of it. For example if this was
implemented in DT, the user could replace a DT with different one.

Assuming that this feature should be in mainline kernel at all, then
probably compile time option is the best way. Some subsystems don't
offer dangerous operations (e.g. setting voltage for regulators or force
suspending devices) but they provide certain development drivers which
expose such interfaces. Such cases should be rather done per subsystem,
not per device.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221591

On Wed, Sep 09, 2015 at 01:17:11PM +0900, Krzysztof Kozlowski wrote:
> On 09.09.2015 11:26, Andreas Dannenberg wrote:
> > Krzysztof, good observation! In bq2425x_charger.c (formerly known as
> > bq24257_charger.c :) that I worked on the unit used was uA. At that time
> > I did a quick check and there didn't seem to be a clear standard whether
> > to use the "micro" or "milli" units - different drivers use different
> > units. However there seems to be a tendency for the TI drivers to prefer
> > "milli" (bq2415x_charger.c, bq24735-charger.c)
> > 
> > Personally I think "milli" units are more appropriate for chargers since
> > they provide sufficient granularity and the numbers don't become too big
> > (try typing a voltage in the Volt-range in uV, it's very easy to get the
> > number of 0s wrong). However since the driver was already there I left
> > that aspect alone to preserve compatibility.
> 
> I am fine with both units but milli indeed seems easier to judge by fast
> looking and less error-prone. Whatever you choose - choose the same one. :)

Ok sounds good. If so, I could go ahead and change the units in the
bq2425x_charger.c over to mA and mV? It would be a bit labor some and I
also want to see what Laurentiu thinks but this way we could have most
of those TI charger drivers use the same units (the new bq24261 driver
Ram posted also uses mA/mV). Except bq25890_charger.c.... that would
still use uA/uV....

Laurentiu -- what made you chose the "micro" units for bq24257_charger.c
and bq25890_charger.c?

Thanks,

--
Andreas Dannenberg
Texas Instruments Inc

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221800 — Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

On 10.09.2015 02:31, Andreas Dannenberg wrote:
> On Wed, Sep 09, 2015 at 01:17:11PM +0900, Krzysztof Kozlowski wrote:
>> On 09.09.2015 11:26, Andreas Dannenberg wrote:
>>> Krzysztof, good observation! In bq2425x_charger.c (formerly known as
>>> bq24257_charger.c :) that I worked on the unit used was uA. At that time
>>> I did a quick check and there didn't seem to be a clear standard whether
>>> to use the "micro" or "milli" units - different drivers use different
>>> units. However there seems to be a tendency for the TI drivers to prefer
>>> "milli" (bq2415x_charger.c, bq24735-charger.c)
>>>
>>> Personally I think "milli" units are more appropriate for chargers since
>>> they provide sufficient granularity and the numbers don't become too big
>>> (try typing a voltage in the Volt-range in uV, it's very easy to get the
>>> number of 0s wrong). However since the driver was already there I left
>>> that aspect alone to preserve compatibility.
>>
>> I am fine with both units but milli indeed seems easier to judge by fast
>> looking and less error-prone. Whatever you choose - choose the same one. :)
> 
> Ok sounds good. If so, I could go ahead and change the units in the
> bq2425x_charger.c over to mA and mV?

Wait, these are existing bindings (for bq24257). You cannot change
existing binding.

Best regards,
Krzysztof

> It would be a bit labor some and I
> also want to see what Laurentiu thinks but this way we could have most
> of those TI charger drivers use the same units (the new bq24261 driver
> Ram posted also uses mA/mV). Except bq25890_charger.c.... that would
> still use uA/uV....
> 
> Laurentiu -- what made you chose the "micro" units for bq24257_charger.c
> and bq25890_charger.c?



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222246

On Thu, Sep 10, 2015 at 08:49:26AM +0900, Krzysztof Kozlowski wrote:
> On 10.09.2015 02:31, Andreas Dannenberg wrote:
> > On Wed, Sep 09, 2015 at 01:17:11PM +0900, Krzysztof Kozlowski wrote:
> >> On 09.09.2015 11:26, Andreas Dannenberg wrote:
> >>> Krzysztof, good observation! In bq2425x_charger.c (formerly known as
> >>> bq24257_charger.c :) that I worked on the unit used was uA. At that time
> >>> I did a quick check and there didn't seem to be a clear standard whether
> >>> to use the "micro" or "milli" units - different drivers use different
> >>> units. However there seems to be a tendency for the TI drivers to prefer
> >>> "milli" (bq2415x_charger.c, bq24735-charger.c)
> >>>
> >>> Personally I think "milli" units are more appropriate for chargers since
> >>> they provide sufficient granularity and the numbers don't become too big
> >>> (try typing a voltage in the Volt-range in uV, it's very easy to get the
> >>> number of 0s wrong). However since the driver was already there I left
> >>> that aspect alone to preserve compatibility.
> >>
> >> I am fine with both units but milli indeed seems easier to judge by fast
> >> looking and less error-prone. Whatever you choose - choose the same one. :)
> > 
> > Ok sounds good. If so, I could go ahead and change the units in the
> > bq2425x_charger.c over to mA and mV?
> 
> Wait, these are existing bindings (for bq24257). You cannot change
> existing binding.
> 
> Best regards,
> Krzysztof
> 
> > It would be a bit labor some and I
> > also want to see what Laurentiu thinks but this way we could have most
> > of those TI charger drivers use the same units (the new bq24261 driver
> > Ram posted also uses mA/mV). Except bq25890_charger.c.... that would
> > still use uA/uV....
> > 
> > Laurentiu -- what made you chose the "micro" units for bq24257_charger.c
> > and bq25890_charger.c?
When I started writing the BQ24257 driver, I had a look on what existed
already. Somehow, I don't know why, I settled on bq24190_charger which
has all the units in uA/uV. I thought it's the de-facto standard for
charger drivers... :/

For bindings, I looked at the existing TI chips bindings and chose the
same binding names, where possible. But for some dumb reason, I missed
that the units already used where mainly in mA/mV and, honestly, I
didn't even suspect units must be consistent from device to device. What
happens if a chip needs a more fine grained setting? I agree on binding
name consistency though: ti,charge-current (for example) should mean the
same for all TI chargers.

That said, is not much we can do on the mA/uA or mV/uV front... At least
we can agree on using the same binding names.

laurentiu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web