| Message ID | 1394052739-8589-4-git-send-email-marek@goldelico.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko wrote: > Signed-off-by: Marek Belisko <marek@goldelico.com> > --- > .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt > > diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt > new file mode 100644 > index 0000000..fd0b6d2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt > @@ -0,0 +1,43 @@ > +twl4030_madc_battery > + > +Required properties: > + - compatible : "ti,twl4030-madc-battery" I'm rather confused by this; the code seems to have stripped away all the twl4030 dependencies and this now seems to be a generic ADC battery status driver. So why does this need to be specific to twl4030? > + - capacity : battery capacity in uAh I would make the units explicit in the name; "capacity" can be measured in many ways. > + - charging-calibration-data : list of voltage(mV):level(%) values > + for charging calibration (see example) > + - discharging-calibration-data : list of voltage(mV):level(%) values > + for discharging calibration (see example) Regardless of the prefixing of these property names, the specifics are unclear to me. Does this apply to vbat? Is vbat guaranteed to read in mV? Is any particular order required? The example seems to go be in descending percentage values. Is any particular interpolation expected on behalf of the OS, or is this just an informative set of measurements that hte OS can use as it sees fit? > + - io-channels: Should contain IIO channel specifiers > + for each element in io-channel-names. > +- io-channel-names: Should contain the following values: > + * "temp" - The ADC channel for temperature reading Which units? Where is this used? > + * "ichg" - The ADC channel for battery charging status How exactly is this used to detect the charging status? > + * "vbat" - The ADC channel to measure the battery voltage > + > +Example: > + madc-battery { > + compatible = "ti,twl4030-madc-battery"; > + capacity = <1200000>; > + charging-calibration-data = <4200 100 > + 4100 75 > + 4000 55 > + 3900 25 > + 3800 5 > + 3700 2 > + 3600 1 > + 3300 0>; Please bracket list entries individually (as you've done with io-channels). Thanks, Mark. > + > + discharging-calibration-data = <4200 100 > + 4100 95 > + 4000 70 > + 3800 50 > + 3700 10 > + 3600 5 > + 3300 0>; > + io-channels = <&twl_madc 1>, > + <&twl_madc 10>, > + <&twl_madc 12>; > + io-channel-names = "temp", > + "ichg", > + "vbat"; > + }; > -- > 1.8.3.2 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Mon, Sep 22, 2014 at 1:03 PM, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko wrote: >> Signed-off-by: Marek Belisko <marek@goldelico.com> >> --- >> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt >> >> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt >> new file mode 100644 >> index 0000000..fd0b6d2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt >> @@ -0,0 +1,43 @@ >> +twl4030_madc_battery >> + >> +Required properties: >> + - compatible : "ti,twl4030-madc-battery" > > I'm rather confused by this; the code seems to have stripped away all > the twl4030 dependencies and this now seems to be a generic ADC battery > status driver. So why does this need to be specific to twl4030? You are right. Also possibly driver name is incorrect :). Better name could be ti, madc-battery. > >> + - capacity : battery capacity in uAh > > I would make the units explicit in the name; "capacity" can be measured > in many ways. OK capacity_uAh should be fine I believe. > >> + - charging-calibration-data : list of voltage(mV):level(%) values >> + for charging calibration (see example) >> + - discharging-calibration-data : list of voltage(mV):level(%) values >> + for discharging calibration (see example) > > Regardless of the prefixing of these property names, the specifics are > unclear to me. Do you suggest to rename it completely or is enough to add "ti, " prefix? > > Does this apply to vbat? Yes bat is read and then interpolation algorithm is applied for various power properties. So mapping from voltage to percentage is defined in table. > > Is vbat guaranteed to read in mV? Yes. > > Is any particular order required? The example seems to go be in > descending percentage values. There is done sorting in driver so ordering it's not necessary. > > Is any particular interpolation expected on behalf of the OS, or is this > just an informative set of measurements that hte OS can use as it sees > fit? > >> + - io-channels: Should contain IIO channel specifiers >> + for each element in io-channel-names. >> +- io-channel-names: Should contain the following values: All 3 statements was copied from rx51-battery and documentation is the same. There is not unit for all channels just raw values which are then converted in driver. >> + * "temp" - The ADC channel for temperature reading > > Which units? Where is this used? > >> + * "ichg" - The ADC channel for battery charging status > > How exactly is this used to detect the charging status? > >> + * "vbat" - The ADC channel to measure the battery voltage >> + >> +Example: >> + madc-battery { >> + compatible = "ti,twl4030-madc-battery"; >> + capacity = <1200000>; >> + charging-calibration-data = <4200 100 >> + 4100 75 >> + 4000 55 >> + 3900 25 >> + 3800 5 >> + 3700 2 >> + 3600 1 >> + 3300 0>; > > Please bracket list entries individually (as you've done with > io-channels). OK. > > Thanks, > Mark. > >> + >> + discharging-calibration-data = <4200 100 >> + 4100 95 >> + 4000 70 >> + 3800 50 >> + 3700 10 >> + 3600 5 >> + 3300 0>; >> + io-channels = <&twl_madc 1>, >> + <&twl_madc 10>, >> + <&twl_madc 12>; >> + io-channel-names = "temp", >> + "ichg", >> + "vbat"; >> + }; >> -- >> 1.8.3.2 >> >> Thanks, Marek
On Mon, Sep 22, 2014 at 09:35:03PM +0100, Belisko Marek wrote: > Hi Mark, > > On Mon, Sep 22, 2014 at 1:03 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi, > > > > On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko wrote: > >> Signed-off-by: Marek Belisko <marek@goldelico.com> > >> --- > >> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++ > >> 1 file changed, 43 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt > >> > >> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt > >> new file mode 100644 > >> index 0000000..fd0b6d2 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt > >> @@ -0,0 +1,43 @@ > >> +twl4030_madc_battery > >> + > >> +Required properties: > >> + - compatible : "ti,twl4030-madc-battery" > > > > I'm rather confused by this; the code seems to have stripped away all > > the twl4030 dependencies and this now seems to be a generic ADC battery > > status driver. So why does this need to be specific to twl4030? > You are right. Also possibly driver name is incorrect :). Better name could be > ti, madc-battery. Ok. > > > >> + - capacity : battery capacity in uAh > > > > I would make the units explicit in the name; "capacity" can be measured > > in many ways. > OK capacity_uAh should be fine I believe. Well, "capacity-uah" to match naming conventions. > > > >> + - charging-calibration-data : list of voltage(mV):level(%) values > >> + for charging calibration (see example) > >> + - discharging-calibration-data : list of voltage(mV):level(%) values > >> + for discharging calibration (see example) > > > > Regardless of the prefixing of these property names, the specifics are > > unclear to me. > Do you suggest to rename it completely or is enough to add "ti, " prefix? I'm not suggesting prefixing. I'm suggesting that "calibration data" could mean a variety of things, so having a more specific name would be beneficial. > > Does this apply to vbat? > Yes bat is read and then interpolation algorithm is applied for > various power properties. > So mapping from voltage to percentage is defined in table. > > > > Is vbat guaranteed to read in mV? > Yes. > > > > Is any particular order required? The example seems to go be in > > descending percentage values. > There is done sorting in driver so ordering it's not necessary. > > > > Is any particular interpolation expected on behalf of the OS, or is this > > just an informative set of measurements that hte OS can use as it sees > > fit? > > > >> + - io-channels: Should contain IIO channel specifiers > >> + for each element in io-channel-names. > >> +- io-channel-names: Should contain the following values: > All 3 statements was copied from rx51-battery and documentation is the same. > There is not unit for all channels just raw values which are then > converted in driver. I don't understand. How do you figure out the mapping of those raw values to sensible units? Surely you need additional information to do so? Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Fri, Sep 26, 2014 at 3:39 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Sep 22, 2014 at 09:35:03PM +0100, Belisko Marek wrote: >> Hi Mark, >> >> On Mon, Sep 22, 2014 at 1:03 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> > Hi, >> > >> > On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko wrote: >> >> Signed-off-by: Marek Belisko <marek@goldelico.com> >> >> --- >> >> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++ >> >> 1 file changed, 43 insertions(+) >> >> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt >> >> >> >> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt >> >> new file mode 100644 >> >> index 0000000..fd0b6d2 >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt >> >> @@ -0,0 +1,43 @@ >> >> +twl4030_madc_battery >> >> + >> >> +Required properties: >> >> + - compatible : "ti,twl4030-madc-battery" >> > >> > I'm rather confused by this; the code seems to have stripped away all >> > the twl4030 dependencies and this now seems to be a generic ADC battery >> > status driver. So why does this need to be specific to twl4030? >> You are right. Also possibly driver name is incorrect :). Better name could be >> ti, madc-battery. > > Ok. > >> > >> >> + - capacity : battery capacity in uAh >> > >> > I would make the units explicit in the name; "capacity" can be measured >> > in many ways. >> OK capacity_uAh should be fine I believe. > > Well, "capacity-uah" to match naming conventions. OK. > >> > >> >> + - charging-calibration-data : list of voltage(mV):level(%) values >> >> + for charging calibration (see example) >> >> + - discharging-calibration-data : list of voltage(mV):level(%) values >> >> + for discharging calibration (see example) >> > >> > Regardless of the prefixing of these property names, the specifics are >> > unclear to me. >> Do you suggest to rename it completely or is enough to add "ti, " prefix? > > I'm not suggesting prefixing. I'm suggesting that "calibration data" > could mean a variety of things, so having a more specific name would be > beneficial. Charging and discharging data are used for some kind of interpolation within voltage interval. So we get volume from ADC and then convert it according charging/discharging table to some value which is then useded in various battery properties (status, charge_now, capacity, time_to_empty_now). So maybe battery-charging/discharging-calibration-data could be property name? > >> > Does this apply to vbat? >> Yes bat is read and then interpolation algorithm is applied for >> various power properties. >> So mapping from voltage to percentage is defined in table. >> > >> > Is vbat guaranteed to read in mV? >> Yes. >> > >> > Is any particular order required? The example seems to go be in >> > descending percentage values. >> There is done sorting in driver so ordering it's not necessary. >> > >> > Is any particular interpolation expected on behalf of the OS, or is this >> > just an informative set of measurements that hte OS can use as it sees >> > fit? >> > >> >> + - io-channels: Should contain IIO channel specifiers >> >> + for each element in io-channel-names. >> >> +- io-channel-names: Should contain the following values: >> All 3 statements was copied from rx51-battery and documentation is the same. >> There is not unit for all channels just raw values which are then >> converted in driver. > > I don't understand. How do you figure out the mapping of those raw > values to sensible units? Surely you need additional information to do > so? Yes mapping from raw values is done in driver. Like current read from iio channel_ichg is multiplied by 1000 or read from channel_temp is multiplied by 10 to have proper values for battery properties. > > Mark. BR, marek
diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt new file mode 100644 index 0000000..fd0b6d2 --- /dev/null +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt @@ -0,0 +1,43 @@ +twl4030_madc_battery + +Required properties: + - compatible : "ti,twl4030-madc-battery" + - capacity : battery capacity in uAh + - charging-calibration-data : list of voltage(mV):level(%) values + for charging calibration (see example) + - discharging-calibration-data : list of voltage(mV):level(%) values + for discharging calibration (see example) + - io-channels: Should contain IIO channel specifiers + for each element in io-channel-names. +- io-channel-names: Should contain the following values: + * "temp" - The ADC channel for temperature reading + * "ichg" - The ADC channel for battery charging status + * "vbat" - The ADC channel to measure the battery voltage + +Example: + madc-battery { + compatible = "ti,twl4030-madc-battery"; + capacity = <1200000>; + charging-calibration-data = <4200 100 + 4100 75 + 4000 55 + 3900 25 + 3800 5 + 3700 2 + 3600 1 + 3300 0>; + + discharging-calibration-data = <4200 100 + 4100 95 + 4000 70 + 3800 50 + 3700 10 + 3600 5 + 3300 0>; + io-channels = <&twl_madc 1>, + <&twl_madc 10>, + <&twl_madc 12>; + io-channel-names = "temp", + "ichg", + "vbat"; + };
Signed-off-by: Marek Belisko <marek@goldelico.com> --- .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt