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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
