| Message ID | 1442003202-1430-2-git-send-email-dannenberg@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 12.09.2015 05:26, Andreas Dannenberg wrote: > Extend the bq24257 charger's device tree documentation to cover the > bq24250 and bq24251 devices as well feature additions. > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > --- > .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- > 1 file changed, 54 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt > index 5c9d394..4a88c96 100644 > --- a/Documentation/devicetree/bindings/power/bq24257.txt > +++ b/Documentation/devicetree/bindings/power/bq24257.txt > @@ -1,21 +1,70 @@ > -Binding for TI bq24257 Li-Ion Charger > +Binding for TI bq24250/bq24251/bq24257 Li-Ion Charger > > Required properties: > - compatible: Should contain one of the following: > + * "ti,bq24250" > + * "ti,bq24251" > * "ti,bq24257" > -- reg: integer, i2c address of the device. > +- reg: integer, i2c address of the device. > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can > + also be defined through the standard interrupt definition properties (see > + optional properties section below). Only use one method. > - ti,battery-regulation-voltage: integer, maximum charging voltage in uV. > -- ti,charge-current: integer, maximum charging current in uA. > -- ti,termination-current: integer, charge will be terminated when current in > - constant-voltage phase drops below this value (in uA). > +- ti,charge-current: integer, maximum charging current in uA. > +- ti,termination-current: integer, charge will be terminated when current in > + constant-voltage phase drops below this value (in uA). > + > +Optional properties: > +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin. > + This pin is not available on all devices however it should be used if > + possible as this is the recommended way to obtain the charger's input PG > + state. If this pin is not specified a software-based approach for PG > + detection is used. > +- ti,current-limit: The maximum current to be drawn from the charger's input > + (in uA). If this property is not specified a USB D+/D- signal based charger > + type detection is used (if available) and the input limit current is set > + accordingly. If the D+/D- based detection is not available on a given device > + a default of 500,000 is used (=500mA). > +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If > + not specified a default of 6,5000,000 (=6.5V) is used. > +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic > + power path management (in uV). If not specified a default of 4,360,000 > + (=4.36V) is used. > +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety > + timer is extended (slowed down) by a factor of two. Setting this property > + to 0 or not providing it will leave the safety timer at its default > + setting. Now I spotted the difference in this binding from previous emails. Previously you made it as a boolean property. Now it is a integer property for a boolean value. It is unusual. If you expect a need (or a possibility of need) of: 1. extending, 2. overriding, 3. using similar (extended) property in different drivers, then it should be a real value, like: 1. ti,safety-timer: integer, in seconds 2. ti,safety-timer-ratio/multiplier: integer, supported values are: 0 (use default) or 2 (slow down by factor of 2) It is unusual and not obvious to use a integer value for boolean strict property. With current solution you can actually only override it. Extending is possible but would look really weird, like: ti,safety-timer-2x-enable = <4>; /* Heh? 4x or (4*2)x ??? */ To add even more confusion: why this property has to be configured in Device Tree? How it is related to the hardware? It rather looks like a job for sysfs. Best regards, Krzysztof > +- interrupt-parent: Should be the phandle for the interrupt controller. Use in > + conjunction with "interrupts" and only in case "stat-gpios" is not used. > +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in > + conjunction with "interrupt-parent" and only in case "stat-gpios" is not > + used. > > Example: > > bq24257 { > compatible = "ti,bq24257"; > reg = <0x6a>; > + stat-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; > + pg-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; > > ti,battery-regulation-voltage = <4200000>; > ti,charge-current = <1000000>; > ti,termination-current = <50000>; > }; > + > +Example: > + > +bq24250 { > + compatible = "ti,bq24250"; > + reg = <0x6a>; > + interrupt-parent = <&gpio1>; > + interrupts = <16 IRQ_TYPE_EDGE_BOTH>; > + > + ti,battery-regulation-voltage = <4200000>; > + ti,charge-current = <500000>; > + ti,termination-current = <50000>; > + ti,current-limit = <900000>; > + ti,ovp-voltage = <9500000>; > + ti,in-dpm-voltage = <4440000>; > +}; > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 14, 2015 at 03:11:32PM +0900, Krzysztof Kozlowski wrote: > On 12.09.2015 05:26, Andreas Dannenberg wrote: > > Extend the bq24257 charger's device tree documentation to cover the > > bq24250 and bq24251 devices as well feature additions. > > > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > > --- > > .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- > > 1 file changed, 54 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt > > index 5c9d394..4a88c96 100644 > > --- a/Documentation/devicetree/bindings/power/bq24257.txt > > +++ b/Documentation/devicetree/bindings/power/bq24257.txt > > @@ -1,21 +1,70 @@ > > -Binding for TI bq24257 Li-Ion Charger > > +Binding for TI bq24250/bq24251/bq24257 Li-Ion Charger > > > > Required properties: > > - compatible: Should contain one of the following: > > + * "ti,bq24250" > > + * "ti,bq24251" > > * "ti,bq24257" > > -- reg: integer, i2c address of the device. > > +- reg: integer, i2c address of the device. > > +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can > > + also be defined through the standard interrupt definition properties (see > > + optional properties section below). Only use one method. > > - ti,battery-regulation-voltage: integer, maximum charging voltage in uV. > > -- ti,charge-current: integer, maximum charging current in uA. > > -- ti,termination-current: integer, charge will be terminated when current in > > - constant-voltage phase drops below this value (in uA). > > +- ti,charge-current: integer, maximum charging current in uA. > > +- ti,termination-current: integer, charge will be terminated when current in > > + constant-voltage phase drops below this value (in uA). > > + > > +Optional properties: > > +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin. > > + This pin is not available on all devices however it should be used if > > + possible as this is the recommended way to obtain the charger's input PG > > + state. If this pin is not specified a software-based approach for PG > > + detection is used. > > +- ti,current-limit: The maximum current to be drawn from the charger's input > > + (in uA). If this property is not specified a USB D+/D- signal based charger > > + type detection is used (if available) and the input limit current is set > > + accordingly. If the D+/D- based detection is not available on a given device > > + a default of 500,000 is used (=500mA). > > +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If > > + not specified a default of 6,5000,000 (=6.5V) is used. > > +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic > > + power path management (in uV). If not specified a default of 4,360,000 > > + (=4.36V) is used. > > +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety > > + timer is extended (slowed down) by a factor of two. Setting this property > > + to 0 or not providing it will leave the safety timer at its default > > + setting. > > Now I spotted the difference in this binding from previous emails. > Previously you made it as a boolean property. Now it is a integer > property for a boolean value. It is unusual. If you expect a need (or a > possibility of need) of: > 1. extending, > 2. overriding, > 3. using similar (extended) property in different drivers, > then it should be a real value, like: > > 1. ti,safety-timer: integer, in seconds Hi Krzysztof, I thought about this as well because the device actually allows configuring different safety timer intervals (currently not exposed) and it appears that the "2x enable" could be factored in there however it's not as easy as it seems. The "2x enable" is a dedicated HW feature that's only being used in some states of the devices and with that does not generally extend the safety timer under all conditions. > 2. ti,safety-timer-ratio/multiplier: integer, supported values are: 0 > (use default) or 2 (slow down by factor of 2) > > It is unusual and not obvious to use a integer value for boolean strict > property. With current solution you can actually only override it. Yes that was the sole reason behind using integer instead of boolean. The boolean "character" of this property was left intact because of the direct mapping to a device HW feature (see below). But I can certainly change this to ti,safety-timer-multiplier and use 0/2. This avoids the inaccuracy as explained above with just using ti,safety-timer (the corresponding underlying device property should be separate and could be exposed if ever needed) and still leaves room for this property to grow which may come in handy later. > Extending is possible but would look really weird, like: > ti,safety-timer-2x-enable = <4>; /* Heh? 4x or (4*2)x ??? */ > > To add even more confusion: why this property has to be configured in > Device Tree? How it is related to the hardware? It rather looks like a > job for sysfs. This property maps directly to a corresponding bit in the device register map so yes it is related to the HW and IMHO should be applicable to DT. The reason for not providing write access to that bit via sysfs is the same as this driver doesn't have write access to other "critical" properties such as charge current/voltages. Regards, -- Andreas Dannenberg Texas Instruments Inc > > Best regards, > Krzysztof > > > +- interrupt-parent: Should be the phandle for the interrupt controller. Use in > > + conjunction with "interrupts" and only in case "stat-gpios" is not used. > > +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in > > + conjunction with "interrupt-parent" and only in case "stat-gpios" is not > > + used. > > > > Example: > > > > bq24257 { > > compatible = "ti,bq24257"; > > reg = <0x6a>; > > + stat-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; > > + pg-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; > > > > ti,battery-regulation-voltage = <4200000>; > > ti,charge-current = <1000000>; > > ti,termination-current = <50000>; > > }; > > + > > +Example: > > + > > +bq24250 { > > + compatible = "ti,bq24250"; > > + reg = <0x6a>; > > + interrupt-parent = <&gpio1>; > > + interrupts = <16 IRQ_TYPE_EDGE_BOTH>; > > + > > + ti,battery-regulation-voltage = <4200000>; > > + ti,charge-current = <500000>; > > + ti,termination-current = <50000>; > > + ti,current-limit = <900000>; > > + ti,ovp-voltage = <9500000>; > > + ti,in-dpm-voltage = <4440000>; > > +}; > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14.09.2015 22:54, Andreas Dannenberg wrote: > On Mon, Sep 14, 2015 at 03:11:32PM +0900, Krzysztof Kozlowski wrote: >> On 12.09.2015 05:26, Andreas Dannenberg wrote: >>> Extend the bq24257 charger's device tree documentation to cover the >>> bq24250 and bq24251 devices as well feature additions. >>> >>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> >>> --- >>> .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- >>> 1 file changed, 54 insertions(+), 5 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt >>> index 5c9d394..4a88c96 100644 >>> --- a/Documentation/devicetree/bindings/power/bq24257.txt >>> +++ b/Documentation/devicetree/bindings/power/bq24257.txt >>> @@ -1,21 +1,70 @@ >>> -Binding for TI bq24257 Li-Ion Charger >>> +Binding for TI bq24250/bq24251/bq24257 Li-Ion Charger >>> >>> Required properties: >>> - compatible: Should contain one of the following: >>> + * "ti,bq24250" >>> + * "ti,bq24251" >>> * "ti,bq24257" >>> -- reg: integer, i2c address of the device. >>> +- reg: integer, i2c address of the device. >>> +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can >>> + also be defined through the standard interrupt definition properties (see >>> + optional properties section below). Only use one method. >>> - ti,battery-regulation-voltage: integer, maximum charging voltage in uV. >>> -- ti,charge-current: integer, maximum charging current in uA. >>> -- ti,termination-current: integer, charge will be terminated when current in >>> - constant-voltage phase drops below this value (in uA). >>> +- ti,charge-current: integer, maximum charging current in uA. >>> +- ti,termination-current: integer, charge will be terminated when current in >>> + constant-voltage phase drops below this value (in uA). >>> + >>> +Optional properties: >>> +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin. >>> + This pin is not available on all devices however it should be used if >>> + possible as this is the recommended way to obtain the charger's input PG >>> + state. If this pin is not specified a software-based approach for PG >>> + detection is used. >>> +- ti,current-limit: The maximum current to be drawn from the charger's input >>> + (in uA). If this property is not specified a USB D+/D- signal based charger >>> + type detection is used (if available) and the input limit current is set >>> + accordingly. If the D+/D- based detection is not available on a given device >>> + a default of 500,000 is used (=500mA). >>> +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If >>> + not specified a default of 6,5000,000 (=6.5V) is used. >>> +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic >>> + power path management (in uV). If not specified a default of 4,360,000 >>> + (=4.36V) is used. >>> +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety >>> + timer is extended (slowed down) by a factor of two. Setting this property >>> + to 0 or not providing it will leave the safety timer at its default >>> + setting. >> >> Now I spotted the difference in this binding from previous emails. >> Previously you made it as a boolean property. Now it is a integer >> property for a boolean value. It is unusual. If you expect a need (or a >> possibility of need) of: >> 1. extending, >> 2. overriding, >> 3. using similar (extended) property in different drivers, >> then it should be a real value, like: >> >> 1. ti,safety-timer: integer, in seconds > > Hi Krzysztof, > > I thought about this as well because the device actually allows > configuring different safety timer intervals (currently not exposed) and > it appears that the "2x enable" could be factored in there however it's > not as easy as it seems. The "2x enable" is a dedicated HW feature > that's only being used in some states of the devices and with that does > not generally extend the safety timer under all conditions. Right, I read also the datasheet which explains it. > >> 2. ti,safety-timer-ratio/multiplier: integer, supported values are: 0 >> (use default) or 2 (slow down by factor of 2) >> >> It is unusual and not obvious to use a integer value for boolean strict >> property. With current solution you can actually only override it. > > Yes that was the sole reason behind using integer instead of boolean. > The boolean "character" of this property was left intact because of the > direct mapping to a device HW feature (see below). DT serves a purpose of providing information allowing the kernel to use and configure the hardware. DT binding does not have to map device property. In the same time having a device property does not mean you create a DT binding. If such requirement of mapping property to a HW feature was valid then each driver would expose each register in DT. Sometimes platform data did this... but DT is not pdata. You're using such argument also below but this argument is not sufficient. Sufficient argument would be: this property is necessary to configure the hardware in different board configurations (so on different boards it will have different values). Example of device features not matching to DeviceTree: - configuration changing over time: /sys/class/power/ds2760-battery.*/charge_full - various timers: /sys/class/power_supply/max14577-charger/device/fast_charge_timer - various thresholds: /sys/class/power_supply/max77693-charger/device/top_off_threshold_current These are strictly HW features (they map to a bit in register) but they are not related to configuring device for specific board. They serve for user-space to use the devices in different ways. In this case - the 2XTMR_EN - I assume that for certain batteries or boards you want to extend the time of charging in case of special conditions? > But I can certainly > change this to ti,safety-timer-multiplier and use 0/2. This avoids the > inaccuracy as explained above with just using ti,safety-timer (the > corresponding underlying device property should be separate and could be > exposed if ever needed) and still leaves room for this property to grow > which may come in handy later. Hmmmm, the "2x" part of timer suggests to use a numerical value but actually the datasheet does the opposite. I would not expect different values... so your original idea makes sense (if DT is the place for it). Overriding could be achieved with /delete-property/ (I forgot about it...). > >> Extending is possible but would look really weird, like: >> ti,safety-timer-2x-enable = <4>; /* Heh? 4x or (4*2)x ??? */ > >> >> To add even more confusion: why this property has to be configured in >> Device Tree? How it is related to the hardware? It rather looks like a >> job for sysfs. > > This property maps directly to a corresponding bit in the device > register map so yes it is related to the HW and IMHO should be > applicable to DT. The reason for not providing write access to that bit > via sysfs is the same as this driver doesn't have write access to other > "critical" properties such as charge current/voltages. Responded to this above. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 15, 2015 at 04:56:52PM +0900, Krzysztof Kozlowski wrote: > On 14.09.2015 22:54, Andreas Dannenberg wrote: > > On Mon, Sep 14, 2015 at 03:11:32PM +0900, Krzysztof Kozlowski wrote: > >> On 12.09.2015 05:26, Andreas Dannenberg wrote: > >>> Extend the bq24257 charger's device tree documentation to cover the > >>> bq24250 and bq24251 devices as well feature additions. > >>> > >>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > >>> --- > >>> .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- > >>> 1 file changed, 54 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt > >>> index 5c9d394..4a88c96 100644 > >>> --- a/Documentation/devicetree/bindings/power/bq24257.txt > >>> +++ b/Documentation/devicetree/bindings/power/bq24257.txt > >>> @@ -1,21 +1,70 @@ > >>> -Binding for TI bq24257 Li-Ion Charger > >>> +Binding for TI bq24250/bq24251/bq24257 Li-Ion Charger > >>> > >>> Required properties: > >>> - compatible: Should contain one of the following: > >>> + * "ti,bq24250" > >>> + * "ti,bq24251" > >>> * "ti,bq24257" > >>> -- reg: integer, i2c address of the device. > >>> +- reg: integer, i2c address of the device. > >>> +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can > >>> + also be defined through the standard interrupt definition properties (see > >>> + optional properties section below). Only use one method. > >>> - ti,battery-regulation-voltage: integer, maximum charging voltage in uV. > >>> -- ti,charge-current: integer, maximum charging current in uA. > >>> -- ti,termination-current: integer, charge will be terminated when current in > >>> - constant-voltage phase drops below this value (in uA). > >>> +- ti,charge-current: integer, maximum charging current in uA. > >>> +- ti,termination-current: integer, charge will be terminated when current in > >>> + constant-voltage phase drops below this value (in uA). > >>> + > >>> +Optional properties: > >>> +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin. > >>> + This pin is not available on all devices however it should be used if > >>> + possible as this is the recommended way to obtain the charger's input PG > >>> + state. If this pin is not specified a software-based approach for PG > >>> + detection is used. > >>> +- ti,current-limit: The maximum current to be drawn from the charger's input > >>> + (in uA). If this property is not specified a USB D+/D- signal based charger > >>> + type detection is used (if available) and the input limit current is set > >>> + accordingly. If the D+/D- based detection is not available on a given device > >>> + a default of 500,000 is used (=500mA). > >>> +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If > >>> + not specified a default of 6,5000,000 (=6.5V) is used. > >>> +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic > >>> + power path management (in uV). If not specified a default of 4,360,000 > >>> + (=4.36V) is used. > >>> +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety > >>> + timer is extended (slowed down) by a factor of two. Setting this property > >>> + to 0 or not providing it will leave the safety timer at its default > >>> + setting. > >> > >> Now I spotted the difference in this binding from previous emails. > >> Previously you made it as a boolean property. Now it is a integer > >> property for a boolean value. It is unusual. If you expect a need (or a > >> possibility of need) of: > >> 1. extending, > >> 2. overriding, > >> 3. using similar (extended) property in different drivers, > >> then it should be a real value, like: > >> > >> 1. ti,safety-timer: integer, in seconds > > > > Hi Krzysztof, > > > > I thought about this as well because the device actually allows > > configuring different safety timer intervals (currently not exposed) and > > it appears that the "2x enable" could be factored in there however it's > > not as easy as it seems. The "2x enable" is a dedicated HW feature > > that's only being used in some states of the devices and with that does > > not generally extend the safety timer under all conditions. > > Right, I read also the datasheet which explains it. > > > > >> 2. ti,safety-timer-ratio/multiplier: integer, supported values are: 0 > >> (use default) or 2 (slow down by factor of 2) > >> > >> It is unusual and not obvious to use a integer value for boolean strict > >> property. With current solution you can actually only override it. > > > > Yes that was the sole reason behind using integer instead of boolean. > > The boolean "character" of this property was left intact because of the > > direct mapping to a device HW feature (see below). > > DT serves a purpose of providing information allowing the kernel to use > and configure the hardware. DT binding does not have to map device > property. In the same time having a device property does not mean you > create a DT binding. > > If such requirement of mapping property to a HW feature was valid then > each driver would expose each register in DT. Sometimes platform data > did this... but DT is not pdata. > You're using such argument also below but this argument is not sufficient. > > Sufficient argument would be: this property is necessary to configure > the hardware in different board configurations (so on different boards > it will have different values). > > Example of device features not matching to DeviceTree: > - configuration changing over time: > /sys/class/power/ds2760-battery.*/charge_full > > - various timers: > /sys/class/power_supply/max14577-charger/device/fast_charge_timer > > - various thresholds: > /sys/class/power_supply/max77693-charger/device/top_off_threshold_current > > These are strictly HW features (they map to a bit in register) but they > are not related to configuring device for specific board. They serve for > user-space to use the devices in different ways. > > In this case - the 2XTMR_EN - I assume that for certain batteries or > boards you want to extend the time of charging in case of special > conditions? FWIW, I don't see any real benefit in manually changing the 2X timer at all. The default setting is decent enough. The chip will automatically switch to 2X timer in certain conditions: for example, when the temperature threshold is exceeded and the charge current is automatically lowered. This makes sense since it'll take the battery more time to charge at a lower current. It would probably make more sense to add a DT property to change the 'Safety Timer Time Limit' device property (which can be 0.75hrs, 6hrs, 9hrs or totally disabled) than the 2XTMR. I omitted both when I originally added support for this chip since the 6hrs default safety timer seemed more than enough to charge a nowadays battery, even at 500mA (provided by a standard USB port). But, future batteries might (and probably will) have higher capacities and will take longer to charge. laurentiu -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-09-15 18:05 GMT+09:00 Laurentiu Palcu <laurentiu.palcu@intel.com>: > On Tue, Sep 15, 2015 at 04:56:52PM +0900, Krzysztof Kozlowski wrote: >> On 14.09.2015 22:54, Andreas Dannenberg wrote: >> > On Mon, Sep 14, 2015 at 03:11:32PM +0900, Krzysztof Kozlowski wrote: >> >> >> >> Now I spotted the difference in this binding from previous emails. >> >> Previously you made it as a boolean property. Now it is a integer >> >> property for a boolean value. It is unusual. If you expect a need (or a >> >> possibility of need) of: >> >> 1. extending, >> >> 2. overriding, >> >> 3. using similar (extended) property in different drivers, >> >> then it should be a real value, like: >> >> >> >> 1. ti,safety-timer: integer, in seconds >> > >> > Hi Krzysztof, >> > >> > I thought about this as well because the device actually allows >> > configuring different safety timer intervals (currently not exposed) and >> > it appears that the "2x enable" could be factored in there however it's >> > not as easy as it seems. The "2x enable" is a dedicated HW feature >> > that's only being used in some states of the devices and with that does >> > not generally extend the safety timer under all conditions. >> >> Right, I read also the datasheet which explains it. >> >> > >> >> 2. ti,safety-timer-ratio/multiplier: integer, supported values are: 0 >> >> (use default) or 2 (slow down by factor of 2) >> >> >> >> It is unusual and not obvious to use a integer value for boolean strict >> >> property. With current solution you can actually only override it. >> > >> > Yes that was the sole reason behind using integer instead of boolean. >> > The boolean "character" of this property was left intact because of the >> > direct mapping to a device HW feature (see below). >> >> DT serves a purpose of providing information allowing the kernel to use >> and configure the hardware. DT binding does not have to map device >> property. In the same time having a device property does not mean you >> create a DT binding. >> >> If such requirement of mapping property to a HW feature was valid then >> each driver would expose each register in DT. Sometimes platform data >> did this... but DT is not pdata. >> You're using such argument also below but this argument is not sufficient. >> >> Sufficient argument would be: this property is necessary to configure >> the hardware in different board configurations (so on different boards >> it will have different values). >> >> Example of device features not matching to DeviceTree: >> - configuration changing over time: >> /sys/class/power/ds2760-battery.*/charge_full >> >> - various timers: >> /sys/class/power_supply/max14577-charger/device/fast_charge_timer >> >> - various thresholds: >> /sys/class/power_supply/max77693-charger/device/top_off_threshold_current >> >> These are strictly HW features (they map to a bit in register) but they >> are not related to configuring device for specific board. They serve for >> user-space to use the devices in different ways. >> >> In this case - the 2XTMR_EN - I assume that for certain batteries or >> boards you want to extend the time of charging in case of special >> conditions? > > FWIW, I don't see any real benefit in manually changing the 2X timer at > all. The default setting is decent enough. The chip will automatically > switch to 2X timer in certain conditions: for example, when the > temperature threshold is exceeded and the charge current is > automatically lowered. This makes sense since it'll take the battery > more time to charge at a lower current. > > It would probably make more sense to add a DT property to change the > 'Safety Timer Time Limit' device property (which can be 0.75hrs, 6hrs, > 9hrs or totally disabled) than the 2XTMR. > > I omitted both when I originally added support for this chip since the > 6hrs default safety timer seemed more than enough to charge a nowadays > battery, even at 500mA (provided by a standard USB port). But, future > batteries might (and probably will) have higher capacities and will take > longer to charge. From my past mistakes I found that if binding is not necessary and it is not used (e.g. every DTS have the same value) then it will be easier not to add it. Bindings mean ABI so if you make a mistake then you will have to leave with it. Usually it is easier to add new bindings when they are needed. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 15, 2015 at 12:05:37PM +0300, Laurentiu Palcu wrote: > On Tue, Sep 15, 2015 at 04:56:52PM +0900, Krzysztof Kozlowski wrote: [...] > > In this case - the 2XTMR_EN - I assume that for certain batteries or > > boards you want to extend the time of charging in case of special > > conditions? > > FWIW, I don't see any real benefit in manually changing the 2X timer at > all. The default setting is decent enough. The chip will automatically > switch to 2X timer in certain conditions: for example, when the > temperature threshold is exceeded and the charge current is > automatically lowered. This makes sense since it'll take the battery > more time to charge at a lower current. Laurentiu, your comment just made me go double-check the applicable datasheets and realize that the timer 2x extension feature is by default enabled (not disabled as I originally thought) which is great. With that, we do have good coverage for the "special" charging conditions and don't need to expose access to this feature at this time. Will remove it. > It would probably make more sense to add a DT property to change the > 'Safety Timer Time Limit' device property (which can be 0.75hrs, 6hrs, > 9hrs or totally disabled) than the 2XTMR. > > I omitted both when I originally added support for this chip since the > 6hrs default safety timer seemed more than enough to charge a nowadays > battery, even at 500mA (provided by a standard USB port). But, future > batteries might (and probably will) have higher capacities and will take > longer to charge. Agreed, 6hrs should cover most use cases. And it'll be easy enough to add on this property when needed. Regards, -- Andreas Dannenberg Texas Instruments Inc -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt index 5c9d394..4a88c96 100644 --- a/Documentation/devicetree/bindings/power/bq24257.txt +++ b/Documentation/devicetree/bindings/power/bq24257.txt @@ -1,21 +1,70 @@ -Binding for TI bq24257 Li-Ion Charger +Binding for TI bq24250/bq24251/bq24257 Li-Ion Charger Required properties: - compatible: Should contain one of the following: + * "ti,bq24250" + * "ti,bq24251" * "ti,bq24257" -- reg: integer, i2c address of the device. +- reg: integer, i2c address of the device. +- stat-gpios: GPIO used for the devices STAT_IN pin. Alternatively the pin can + also be defined through the standard interrupt definition properties (see + optional properties section below). Only use one method. - ti,battery-regulation-voltage: integer, maximum charging voltage in uV. -- ti,charge-current: integer, maximum charging current in uA. -- ti,termination-current: integer, charge will be terminated when current in - constant-voltage phase drops below this value (in uA). +- ti,charge-current: integer, maximum charging current in uA. +- ti,termination-current: integer, charge will be terminated when current in + constant-voltage phase drops below this value (in uA). + +Optional properties: +- pg-gpios: GPIO used for connecting the bq2425x device PG (Power Good) pin. + This pin is not available on all devices however it should be used if + possible as this is the recommended way to obtain the charger's input PG + state. If this pin is not specified a software-based approach for PG + detection is used. +- ti,current-limit: The maximum current to be drawn from the charger's input + (in uA). If this property is not specified a USB D+/D- signal based charger + type detection is used (if available) and the input limit current is set + accordingly. If the D+/D- based detection is not available on a given device + a default of 500,000 is used (=500mA). +- ti,ovp-voltage: Configures the over voltage protection voltage (in uV). If + not specified a default of 6,5000,000 (=6.5V) is used. +- ti,in-dpm-voltage: Configures the threshold input voltage for the dynamic + power path management (in uV). If not specified a default of 4,360,000 + (=4.36V) is used. +- ti,safety-timer-2x-enable: If this property is set to 1 the device's safety + timer is extended (slowed down) by a factor of two. Setting this property + to 0 or not providing it will leave the safety timer at its default + setting. +- interrupt-parent: Should be the phandle for the interrupt controller. Use in + conjunction with "interrupts" and only in case "stat-gpios" is not used. +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in + conjunction with "interrupt-parent" and only in case "stat-gpios" is not + used. Example: bq24257 { compatible = "ti,bq24257"; reg = <0x6a>; + stat-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; + pg-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; ti,battery-regulation-voltage = <4200000>; ti,charge-current = <1000000>; ti,termination-current = <50000>; }; + +Example: + +bq24250 { + compatible = "ti,bq24250"; + reg = <0x6a>; + interrupt-parent = <&gpio1>; + interrupts = <16 IRQ_TYPE_EDGE_BOTH>; + + ti,battery-regulation-voltage = <4200000>; + ti,charge-current = <500000>; + ti,termination-current = <50000>; + ti,current-limit = <900000>; + ti,ovp-voltage = <9500000>; + ti,in-dpm-voltage = <4440000>; +};
Extend the bq24257 charger's device tree documentation to cover the bq24250 and bq24251 devices as well feature additions. Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> --- .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- 1 file changed, 54 insertions(+), 5 deletions(-)