| Message ID | 1441073435-12349-14-git-send-email-dannenberg@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
2015-09-01 11:10 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>: > Extend the bq24257 charger's device tree documentation to cover the > bq24250 and bq24257 devices as well as recent feature additions. > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > --- > .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- > 1 file changed, 55 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt > index 5c9d394..36b115c 100644 > --- a/Documentation/devicetree/bindings/power/bq24257.txt > +++ b/Documentation/devicetree/bindings/power/bq24257.txt > @@ -2,20 +2,71 @@ Binding for TI 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-gpio: 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. Preferred suffix should be "gpios": Documentation/devicetree/bindings/gpio/gpio.txt > +- pg-gpio: GPIO used for the device PG (Power Good) pin. This pin is not > + available on all devices and therefore only required on applicable devices. > + However it is also possible to explicitly disable the use of this pin > + through the optional property "ti,pg-gpio-disable" (see below) if desired. > + In this case the "pg-gpio" property is no longer required. > - 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: > +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables I thought "ilimit" is a typo but apparently not :) . The names of this property and "ti,in-limit-current" below are quite obfuscated. I am thinking about different names but first I would like to understand the feature: what do you mean by "automatic setting of the input current limit"? Like adjusting the limit of possible current to given charger type? > + the automatic setting of the input current limit. This property is also set > + implicitly on devices without charger type detection. If this property is > + provided the input current limit should be set manually through > + "ti,in-limit-current". > +- ti,in-limit-current: The maximum current to be drawn from the charger's input > + (in uA). Use this for devices that don't have charger type detection or if > + you have have set "ti,in-ilimit-autoset-disable". > +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV). > +- ti,vindpm-voltage: Configures the threshold input voltage for the dynamic > + power path management (in uV). I think the leading 'v' could be removed, so "ti,ovp-voltage" and "ti,in-dpm-voltage"? > +- ti,pg-gpio-disable: If this boolean property is provided a software-based > + approach for power good determination is used. Note that the PG-pin based > + approach is generally preferable. That's confusing. If someone does not want to use pg-gpio then he could just skip the "pg-gpio" property? > +- ti,timer-2x-enable: If this boolean property is provided the device's safety > + timer is extended by a factor of two. Boolean properties like this have disadvantage - they cannot be overridden. Also extending them is difficult (e.g. for some next device the timer could have value of 4). Usually it is better to have a value-based property with "0" meaning default or disabled. Also please look at existing bindings: $ git grep "ti," Documentation/devicetree/bindings/power Maybe you could reuse some? Each of these existing and new bindings are made generic (not device specific) so they should be reused. Best regards, Krzysztof > +- interrupt-parent: Should be the phandle for the interrupt controller. Use in > + conjunction with "interrupts" and only in case "stat-gpio" is not used. > +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in > + conjunction with "interrupt-parent" and only in case "stat-gpio" is not > + used. > > Example: > > bq24257 { > compatible = "ti,bq24257"; > reg = <0x6a>; > + stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>; > + pg-gpio = <&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,in-limit-current = <900000>; > + ti,vovp-voltage = <9500000>; > + ti,vindpm-voltage = <4440000>; > +}; > -- > 1.9.1 > -- 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
Hi Krzysztof, thanks for your feedback... comments below. On Wed, Sep 02, 2015 at 02:24:05PM +0900, Krzysztof Kozlowski wrote: > 2015-09-01 11:10 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>: > > --- a/Documentation/devicetree/bindings/power/bq24257.txt > > +++ b/Documentation/devicetree/bindings/power/bq24257.txt > > @@ -2,20 +2,71 @@ Binding for TI 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-gpio: 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. > > Preferred suffix should be "gpios": > Documentation/devicetree/bindings/gpio/gpio.txt Did not know this. Thanks for pointing to the src document. Will fix. > > +Optional properties: > > +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables > > I thought "ilimit" is a typo but apparently not :) . The names of this > property and "ti,in-limit-current" below are quite obfuscated. I am > thinking about different names but first I would like to understand > the feature: what do you mean by "automatic setting of the input > current limit"? Like adjusting the limit of possible current to given > charger type? Yes that's right. Some of the chargers can check the USB D+/D- lines to determine what kind of charger is attached (DCP, CDP, SDP, other) - see bq24257 device datasheet for more info: http://www.ti.com/product/bq24257 And this feature is being used in the original driver Laurentiu developed. However this detection mechanism (or any detection mechanism in general) may not always be appropriate hence the new property to disable it and manually provide an input current limit of how much can be drawn from the charger. Plus it's needed for devices that don't have the D+/D- detection feature. > > > + the automatic setting of the input current limit. This property is also set > > + implicitly on devices without charger type detection. If this property is > > + provided the input current limit should be set manually through > > + "ti,in-limit-current". > > +- ti,in-limit-current: The maximum current to be drawn from the charger's input > > + (in uA). Use this for devices that don't have charger type detection or if > > + you have have set "ti,in-ilimit-autoset-disable". > > +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV). > > +- ti,vindpm-voltage: Configures the threshold input voltage for the dynamic > > + power path management (in uV). > > I think the leading 'v' could be removed, so "ti,ovp-voltage" and > "ti,in-dpm-voltage"? Yes I was thinking about this but then went with something that closely reflects the device datasheets to maximize correlation. Removing the 'v' would make it look cleaner and more universal and I suppose it's not to far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP bit field in the devices register map. So let's change it. > > > +- ti,pg-gpio-disable: If this boolean property is provided a software-based > > + approach for power good determination is used. Note that the PG-pin based > > + approach is generally preferable. > > That's confusing. If someone does not want to use pg-gpio then he > could just skip the "pg-gpio" property? Yes that's right. Somebody might chose to do so because they don't want to sacrifice the extra pin. And I need to accommodate for devices that don't have a PG pin so the logic behind this had to be implemented anyways. I just made it accessible. What about I work on improving that explanation hopefully to make it less confusing. > > > +- ti,timer-2x-enable: If this boolean property is provided the device's safety > > + timer is extended by a factor of two. > > Boolean properties like this have disadvantage - they cannot be > overridden. Also extending them is difficult (e.g. for some next > device the timer could have value of 4). Usually it is better to have > a value-based property with "0" meaning default or disabled. When I looked through the Kernel headers I found the boolean property parse function and figured that's probably what I should be using. But as I was implementing it I had similar concerns to what you mentioned. If it's general practice to prefer the value based properties I'll be happy to change that. > > Also please look at existing bindings: > $ git grep "ti," Documentation/devicetree/bindings/power > Maybe you could reuse some? Each of these existing and new bindings > are made generic (not device specific) so they should be reused. Already spent time with existing bqXXXXX drivers to see if I can recycle properties. Will give it another run-through to make sure there is maximum re-use. Thanks, -- 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-gpio" is not used. > > +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in > > + conjunction with "interrupt-parent" and only in case "stat-gpio" is not > > + used. > > > > Example: > > > > bq24257 { > > compatible = "ti,bq24257"; > > reg = <0x6a>; > > + stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>; > > + pg-gpio = <&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,in-limit-current = <900000>; > > + ti,vovp-voltage = <9500000>; > > + ti,vindpm-voltage = <4440000>; > > +}; > > -- > > 1.9.1 > > -- 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 02.09.2015 23:03, Andreas Dannenberg wrote: > Hi Krzysztof, thanks for your feedback... comments below. > > On Wed, Sep 02, 2015 at 02:24:05PM +0900, Krzysztof Kozlowski wrote: >> 2015-09-01 11:10 GMT+09:00 Andreas Dannenberg <dannenberg@ti.com>: >>> --- a/Documentation/devicetree/bindings/power/bq24257.txt >>> +++ b/Documentation/devicetree/bindings/power/bq24257.txt >>> @@ -2,20 +2,71 @@ Binding for TI 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-gpio: 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. >> >> Preferred suffix should be "gpios": >> Documentation/devicetree/bindings/gpio/gpio.txt > > Did not know this. Thanks for pointing to the src document. Will fix. > >>> +Optional properties: >>> +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables >> >> I thought "ilimit" is a typo but apparently not :) . The names of this >> property and "ti,in-limit-current" below are quite obfuscated. I am >> thinking about different names but first I would like to understand >> the feature: what do you mean by "automatic setting of the input >> current limit"? Like adjusting the limit of possible current to given >> charger type? > > Yes that's right. Some of the chargers can check the USB D+/D- lines to > determine what kind of charger is attached (DCP, CDP, SDP, other) - see > bq24257 device datasheet for more info: > http://www.ti.com/product/bq24257 > > And this feature is being used in the original driver Laurentiu > developed. However this detection mechanism (or any detection mechanism > in general) may not always be appropriate hence the new property to > disable it and manually provide an input current limit of how much can > be drawn from the charger. Plus it's needed for devices that don't have > the D+/D- detection feature. Thanks for explanation. How about leaving only "ti,in-limit-current" (get rid of "ti,in-ilimit-autoset-disable"). If it is present then the autodetection/autoset would be disabled. > >> >>> + the automatic setting of the input current limit. This property is also set >>> + implicitly on devices without charger type detection. If this property is >>> + provided the input current limit should be set manually through >>> + "ti,in-limit-current". >>> +- ti,in-limit-current: The maximum current to be drawn from the charger's input >>> + (in uA). Use this for devices that don't have charger type detection or if >>> + you have have set "ti,in-ilimit-autoset-disable". >>> +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV). >>> +- ti,vindpm-voltage: Configures the threshold input voltage for the dynamic >>> + power path management (in uV). >> >> I think the leading 'v' could be removed, so "ti,ovp-voltage" and >> "ti,in-dpm-voltage"? > > Yes I was thinking about this but then went with something that closely > reflects the device datasheets to maximize correlation. Removing the 'v' > would make it look cleaner and more universal and I suppose it's not to > far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP > bit field in the devices register map. So let's change it. Thanks. Bindings don't have to follow naming convention of company's datasheet. > >> >>> +- ti,pg-gpio-disable: If this boolean property is provided a software-based >>> + approach for power good determination is used. Note that the PG-pin based >>> + approach is generally preferable. >> >> That's confusing. If someone does not want to use pg-gpio then he >> could just skip the "pg-gpio" property? > > Yes that's right. Somebody might chose to do so because they don't want > to sacrifice the extra pin. And I need to accommodate for devices that > don't have a PG pin so the logic behind this had to be implemented > anyways. I just made it accessible. What about I work on improving that > explanation hopefully to make it less confusing. I still don't get why you need extra "ti,pg-gpio-disable" property. It could work like this: 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional. 2. If it is present, use it. 3. If it is not present, use software based approach (the same as setting "ti,pg-gpio-disable" previously). Would that work? >> >>> +- ti,timer-2x-enable: If this boolean property is provided the device's safety >>> + timer is extended by a factor of two. >> >> Boolean properties like this have disadvantage - they cannot be >> overridden. Also extending them is difficult (e.g. for some next >> device the timer could have value of 4). Usually it is better to have >> a value-based property with "0" meaning default or disabled. > > When I looked through the Kernel headers I found the boolean property > parse function and figured that's probably what I should be using. But > as I was implementing it I had similar concerns to what you mentioned. > If it's general practice to prefer the value based properties I'll be > happy to change that. Usually the boolean is a good choice but here you used it for a numerical property. You are preparing a generic TI bindings so it is wise to think about other future drivers. Would they have other values for timer factor? Would you want to set it in common board DTSI and override it in some one specific child DTS? Best regards, Krzysztof > >> >> Also please look at existing bindings: >> $ git grep "ti," Documentation/devicetree/bindings/power >> Maybe you could reuse some? Each of these existing and new bindings >> are made generic (not device specific) so they should be reused. > > Already spent time with existing bqXXXXX drivers to see if I can recycle > properties. Will give it another run-through to make sure there is > maximum re-use. > > Thanks, > > -- > 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-gpio" is not used. >>> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in >>> + conjunction with "interrupt-parent" and only in case "stat-gpio" is not >>> + used. >>> >>> Example: >>> >>> bq24257 { >>> compatible = "ti,bq24257"; >>> reg = <0x6a>; >>> + stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>; >>> + pg-gpio = <&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,in-limit-current = <900000>; >>> + ti,vovp-voltage = <9500000>; >>> + ti,vindpm-voltage = <4440000>; >>> +}; >>> -- >>> 1.9.1 >>> > -- 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 Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote: > > Yes that's right. Some of the chargers can check the USB D+/D- lines to > > determine what kind of charger is attached (DCP, CDP, SDP, other) - see > > bq24257 device datasheet for more info: > > http://www.ti.com/product/bq24257 > > > > And this feature is being used in the original driver Laurentiu > > developed. However this detection mechanism (or any detection mechanism > > in general) may not always be appropriate hence the new property to > > disable it and manually provide an input current limit of how much can > > be drawn from the charger. Plus it's needed for devices that don't have > > the D+/D- detection feature. > > Thanks for explanation. How about leaving only "ti,in-limit-current" > (get rid of "ti,in-ilimit-autoset-disable"). If it is present then the > autodetection/autoset would be disabled. I had it separated out to make the DT declarations more explicit (so one would immediately "see" that autoset is disabled) but other than that there is no reason why this couldn't get folded together. Will change. > > Yes I was thinking about this but then went with something that closely > > reflects the device datasheets to maximize correlation. Removing the 'v' > > would make it look cleaner and more universal and I suppose it's not to > > far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP > > bit field in the devices register map. So let's change it. > > Thanks. Bindings don't have to follow naming convention of company's > datasheet. Ok. > > Yes that's right. Somebody might chose to do so because they don't want > > to sacrifice the extra pin. And I need to accommodate for devices that > > don't have a PG pin so the logic behind this had to be implemented > > anyways. I just made it accessible. What about I work on improving that > > explanation hopefully to make it less confusing. > > I still don't get why you need extra "ti,pg-gpio-disable" property. It > could work like this: > 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional. > 2. If it is present, use it. > 3. If it is not present, use software based approach (the same as > setting "ti,pg-gpio-disable" previously). > > Would that work? Similar to the earlier comment - the idea was to have it more explicit. Plus, the original driver would hard-fail when the "pg-gpio" pin is not provided (since it was essential for that driver's ability to function). If I change to automatically fallback to the SW solution such an error case would in theory not be 100% backward compatible. I could however more clearly indicate/log which method is being used (dev_info) so somebody rummaging through the logs would still see it. Will simplify. > > When I looked through the Kernel headers I found the boolean property > > parse function and figured that's probably what I should be using. But > > as I was implementing it I had similar concerns to what you mentioned. > > If it's general practice to prefer the value based properties I'll be > > happy to change that. > > Usually the boolean is a good choice but here you used it for a > numerical property. You are preparing a generic TI bindings so it is > wise to think about other future drivers. Would they have other values > for timer factor? Would you want to set it in common board DTSI and > override it in some one specific child DTS? Thanks again for the additional comments and the pointers. Regards, -- Andreas Dannenberg Texas Instruments Inc > > Best regards, > Krzysztof > > > > >> > >> Also please look at existing bindings: > >> $ git grep "ti," Documentation/devicetree/bindings/power > >> Maybe you could reuse some? Each of these existing and new bindings > >> are made generic (not device specific) so they should be reused. > > > > Already spent time with existing bqXXXXX drivers to see if I can recycle > > properties. Will give it another run-through to make sure there is > > maximum re-use. > > > > Thanks, > > > > -- > > 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-gpio" is not used. > >>> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in > >>> + conjunction with "interrupt-parent" and only in case "stat-gpio" is not > >>> + used. > >>> > >>> Example: > >>> > >>> bq24257 { > >>> compatible = "ti,bq24257"; > >>> reg = <0x6a>; > >>> + stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>; > >>> + pg-gpio = <&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,in-limit-current = <900000>; > >>> + ti,vovp-voltage = <9500000>; > >>> + ti,vindpm-voltage = <4440000>; > >>> +}; > >>> -- > >>> 1.9.1 > >>> > > > -- 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 03.09.2015 10:47, Andreas Dannenberg wrote: > On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote: >>> Yes that's right. Some of the chargers can check the USB D+/D- lines to >>> determine what kind of charger is attached (DCP, CDP, SDP, other) - see >>> bq24257 device datasheet for more info: >>> http://www.ti.com/product/bq24257 >>> >>> And this feature is being used in the original driver Laurentiu >>> developed. However this detection mechanism (or any detection mechanism >>> in general) may not always be appropriate hence the new property to >>> disable it and manually provide an input current limit of how much can >>> be drawn from the charger. Plus it's needed for devices that don't have >>> the D+/D- detection feature. >> >> Thanks for explanation. How about leaving only "ti,in-limit-current" >> (get rid of "ti,in-ilimit-autoset-disable"). If it is present then the >> autodetection/autoset would be disabled. > > I had it separated out to make the DT declarations more explicit (so one > would immediately "see" that autoset is disabled) but other than that > there is no reason why this couldn't get folded together. Will change. > >>> Yes I was thinking about this but then went with something that closely >>> reflects the device datasheets to maximize correlation. Removing the 'v' >>> would make it look cleaner and more universal and I suppose it's not to >>> far of a stretch for someone to associate "ti,ovp-voltag" with the VOVP >>> bit field in the devices register map. So let's change it. >> >> Thanks. Bindings don't have to follow naming convention of company's >> datasheet. > > Ok. > >>> Yes that's right. Somebody might chose to do so because they don't want >>> to sacrifice the extra pin. And I need to accommodate for devices that >>> don't have a PG pin so the logic behind this had to be implemented >>> anyways. I just made it accessible. What about I work on improving that >>> explanation hopefully to make it less confusing. >> >> I still don't get why you need extra "ti,pg-gpio-disable" property. It >> could work like this: >> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional. >> 2. If it is present, use it. >> 3. If it is not present, use software based approach (the same as >> setting "ti,pg-gpio-disable" previously). >> >> Would that work? > > Similar to the earlier comment - the idea was to have it more explicit. > Plus, the original driver would hard-fail when the "pg-gpio" pin is not > provided (since it was essential for that driver's ability to function). > If I change to automatically fallback to the SW solution such an error > case would in theory not be 100% backward compatible. I could however > more clearly indicate/log which method is being used (dev_info) so > somebody rummaging through the logs would still see it. Will simplify. Right, the kernel's backward compatibility has to be preserved. However for bindings I am not sure. Let's assume booting on device with bq24257. 1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print error (probe would fail). 2. New kernel booted in the same situation (1) would assume GPIOs have to be disabled. 3. Old kernel booted with your previous solution (DTB containing both "pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the "disable" property. 4. New kernel in the same situation (3) would disable GPIOs. There is a difference between (1) and (3). New DTB is not backward compatible with old kernels for existing bq24257 devices. Am I understanding this correctly? Best regards, Krzysztof > >>> When I looked through the Kernel headers I found the boolean property >>> parse function and figured that's probably what I should be using. But >>> as I was implementing it I had similar concerns to what you mentioned. >>> If it's general practice to prefer the value based properties I'll be >>> happy to change that. >> >> Usually the boolean is a good choice but here you used it for a >> numerical property. You are preparing a generic TI bindings so it is >> wise to think about other future drivers. Would they have other values >> for timer factor? Would you want to set it in common board DTSI and >> override it in some one specific child DTS? > > Thanks again for the additional comments and the pointers. > > Regards, > > -- > Andreas Dannenberg > Texas Instruments Inc > > >> >> Best regards, >> Krzysztof >> >>> >>>> >>>> Also please look at existing bindings: >>>> $ git grep "ti," Documentation/devicetree/bindings/power >>>> Maybe you could reuse some? Each of these existing and new bindings >>>> are made generic (not device specific) so they should be reused. >>> >>> Already spent time with existing bqXXXXX drivers to see if I can recycle >>> properties. Will give it another run-through to make sure there is >>> maximum re-use. >>> >>> Thanks, >>> >>> -- >>> 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-gpio" is not used. >>>>> +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in >>>>> + conjunction with "interrupt-parent" and only in case "stat-gpio" is not >>>>> + used. >>>>> >>>>> Example: >>>>> >>>>> bq24257 { >>>>> compatible = "ti,bq24257"; >>>>> reg = <0x6a>; >>>>> + stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>; >>>>> + pg-gpio = <&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,in-limit-current = <900000>; >>>>> + ti,vovp-voltage = <9500000>; >>>>> + ti,vindpm-voltage = <4440000>; >>>>> +}; >>>>> -- >>>>> 1.9.1 >>>>> >>> >> > -- 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 Thu, Sep 03, 2015 at 10:57:12AM +0900, Krzysztof Kozlowski wrote: > On 03.09.2015 10:47, Andreas Dannenberg wrote: > > On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote: > >> I still don't get why you need extra "ti,pg-gpio-disable" property. It > >> could work like this: > >> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional. > >> 2. If it is present, use it. > >> 3. If it is not present, use software based approach (the same as > >> setting "ti,pg-gpio-disable" previously). > >> > >> Would that work? > > > > Similar to the earlier comment - the idea was to have it more explicit. > > Plus, the original driver would hard-fail when the "pg-gpio" pin is not > > provided (since it was essential for that driver's ability to function). > > If I change to automatically fallback to the SW solution such an error > > case would in theory not be 100% backward compatible. I could however > > more clearly indicate/log which method is being used (dev_info) so > > somebody rummaging through the logs would still see it. Will simplify. > > Right, the kernel's backward compatibility has to be preserved. However > for bindings I am not sure. Hi Krzysztof. Please see pages 3-5 in the below PDF, that's what I had in mind. Maybe I mis-interpreted it or there is newer information. http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-dt-as-stable-abi-fairy-tale.pdf With the proposed changes, while the existing bindings (strings) would stay the same, the failure behavior in one specific use case would be slightly different. The question is, where should we draw the line for compatibility? I would argue in this case the general usage is not affected so it should be OK making the change/simplification you propose (getting rid of "ti,pg-gpio-disable"). > Let's assume booting on device with bq24257. > 1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print > error (probe would fail). > 2. New kernel booted in the same situation (1) would assume GPIOs have > to be disabled. > > 3. Old kernel booted with your previous solution (DTB containing both > "pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the > "disable" property. > 4. New kernel in the same situation (3) would disable GPIOs. > > There is a difference between (1) and (3). New DTB is not backward > compatible with old kernels for existing bq24257 devices. > > Am I understanding this correctly? Well almost. I think the difference between case 1 and 3 isn't really an issue from a use case point of view. The case I was trying to describe is as follows (let's call it case 5): 5. An old DTB not containing "pg-gpio" (or with that property being misconfigured) is booted with the new Kernel. The boot will succeed as the SW approach for PG detection gets invoked automatically. The difference now is between (1) and (5). If in (5) somebody specifies "pg-gpio" they would want to explicitly use this pin and be notified if the respective setup fails. My proposal earlier for this was to output through dev_info() during driver probe whether an actual GPIO pin is being used for power-good detection or whether the fall-back SW algorithm has been invoked. This way, at least there is a notification. Thanks and Regards, -- Andreas Dannenberg Texas Instruments Inc > > 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 04.09.2015 01:09, Andreas Dannenberg wrote: > On Thu, Sep 03, 2015 at 10:57:12AM +0900, Krzysztof Kozlowski wrote: >> On 03.09.2015 10:47, Andreas Dannenberg wrote: >>> On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote: >>>> I still don't get why you need extra "ti,pg-gpio-disable" property. It >>>> could work like this: >>>> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional. >>>> 2. If it is present, use it. >>>> 3. If it is not present, use software based approach (the same as >>>> setting "ti,pg-gpio-disable" previously). >>>> >>>> Would that work? >>> >>> Similar to the earlier comment - the idea was to have it more explicit. >>> Plus, the original driver would hard-fail when the "pg-gpio" pin is not >>> provided (since it was essential for that driver's ability to function). >>> If I change to automatically fallback to the SW solution such an error >>> case would in theory not be 100% backward compatible. I could however >>> more clearly indicate/log which method is being used (dev_info) so >>> somebody rummaging through the logs would still see it. Will simplify. >> >> Right, the kernel's backward compatibility has to be preserved. However >> for bindings I am not sure. > > Hi Krzysztof. Please see pages 3-5 in the below PDF, that's what I had > in mind. Maybe I mis-interpreted it or there is newer information. > http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-dt-as-stable-abi-fairy-tale.pdf I think our case is not covered there. I am talking about backward compatibility of DTB with older kernels, not about backward compatibility of kernel. AFAIR this is not a requirement so you can change the behaviour of DTB (backward compatibility of kernel is fully preserved). > > With the proposed changes, while the existing bindings (strings) would > stay the same, the failure behavior in one specific use case would be > slightly different. The question is, where should we draw the line for > compatibility? I would argue in this case the general usage is not > affected so it should be OK making the change/simplification you propose > (getting rid of "ti,pg-gpio-disable"). If we don't care about using newer DTB with older kernel then there is no compatibility line to draw. You can safely change the behaviour. > >> Let's assume booting on device with bq24257. >> 1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print >> error (probe would fail). >> 2. New kernel booted in the same situation (1) would assume GPIOs have >> to be disabled. >> >> 3. Old kernel booted with your previous solution (DTB containing both >> "pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the >> "disable" property. >> 4. New kernel in the same situation (3) would disable GPIOs. >> >> There is a difference between (1) and (3). New DTB is not backward >> compatible with old kernels for existing bq24257 devices. >> >> Am I understanding this correctly? > > Well almost. I think the difference between case 1 and 3 isn't really an > issue from a use case point of view. The case I was trying to describe > is as follows (let's call it case 5): > > 5. An old DTB not containing "pg-gpio" (or with that property being > misconfigured) is booted with the new Kernel. The boot will succeed as > the SW approach for PG detection gets invoked automatically. > > The difference now is between (1) and (5). If in (5) somebody specifies > "pg-gpio" they would want to explicitly use this pin and be notified if > the respective setup fails. My proposal earlier for this was to output > through dev_info() during driver probe whether an actual GPIO pin is > being used for power-good detection or whether the fall-back SW > algorithm has been invoked. This way, at least there is a notification. I think I understand. IMHO if pg-gpio is specified and it fails (like GPIO is invalid) the kernel should not switch to SW mode. This should be all-or-nothing and described in a specific way: iff pg-gpio is missing then SW mode would be used because that was the intention of developer. 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
diff --git a/Documentation/devicetree/bindings/power/bq24257.txt b/Documentation/devicetree/bindings/power/bq24257.txt index 5c9d394..36b115c 100644 --- a/Documentation/devicetree/bindings/power/bq24257.txt +++ b/Documentation/devicetree/bindings/power/bq24257.txt @@ -2,20 +2,71 @@ Binding for TI 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-gpio: 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. +- pg-gpio: GPIO used for the device PG (Power Good) pin. This pin is not + available on all devices and therefore only required on applicable devices. + However it is also possible to explicitly disable the use of this pin + through the optional property "ti,pg-gpio-disable" (see below) if desired. + In this case the "pg-gpio" property is no longer required. - 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: +- ti,in-ilimit-autoset-disable: If this boolean property is present it disables + the automatic setting of the input current limit. This property is also set + implicitly on devices without charger type detection. If this property is + provided the input current limit should be set manually through + "ti,in-limit-current". +- ti,in-limit-current: The maximum current to be drawn from the charger's input + (in uA). Use this for devices that don't have charger type detection or if + you have have set "ti,in-ilimit-autoset-disable". +- ti,vovp-voltage: Configures the over voltage protection voltage (in uV). +- ti,vindpm-voltage: Configures the threshold input voltage for the dynamic + power path management (in uV). +- ti,pg-gpio-disable: If this boolean property is provided a software-based + approach for power good determination is used. Note that the PG-pin based + approach is generally preferable. +- ti,timer-2x-enable: If this boolean property is provided the device's safety + timer is extended by a factor of two. +- interrupt-parent: Should be the phandle for the interrupt controller. Use in + conjunction with "interrupts" and only in case "stat-gpio" is not used. +- interrupts: Interrupt mapping for GPIO IRQ (configure for both edges). Use in + conjunction with "interrupt-parent" and only in case "stat-gpio" is not + used. Example: bq24257 { compatible = "ti,bq24257"; reg = <0x6a>; + stat-gpio = <&gpio1 16 GPIO_ACTIVE_HIGH>; + pg-gpio = <&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,in-limit-current = <900000>; + ti,vovp-voltage = <9500000>; + ti,vindpm-voltage = <4440000>; +};
Extend the bq24257 charger's device tree documentation to cover the bq24250 and bq24257 devices as well as recent feature additions. Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> --- .../devicetree/bindings/power/bq24257.txt | 59 ++++++++++++++++++++-- 1 file changed, 55 insertions(+), 4 deletions(-)