[PATCH 0/2] iio: adc: axp20x: add support for NTC thermistor

[PATCH 0/2] iio: adc: axp20x: add support for NTC thermistor

[Evgeny Boger]

Most AXPxxx-based reference designs place a 10k NTC thermistor on a
TS pin. When appropriately configured, AXP PMICs will inject fixed
current (80uA by default) into TS pin and measure the voltage across a
thermistor. The PMIC itself will by default compare this voltage with
predefined thresholds  and disable battery charging whenever
the battery is too hot or too cold.

Alternatively, the TS pin can be configured as general-purpose
ADC input. This mode is not supported by the driver.
    
This patch series allows reading the voltage on the TS pin. The voltage
can be then either processed by userspace or used by kernel consumer like
hwmon ntc thermistor driver.


Evgeny Boger (2):
  iio:adc:axp20x: add support for NTC thermistor
  dt-bindings: iio: adc: document TS voltage in AXP PMICs

 .../bindings/iio/adc/x-powers,axp209-adc.yaml |  3 ++
 drivers/iio/adc/axp20x_adc.c                  | 45 ++++++++++++++++---
 2 files changed, 42 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] iio:adc:axp20x: add support for NTC thermistor

[Evgeny Boger]

Most AXPxxx-based reference designs place a 10k NTC thermistor on a
TS pin. When appropriately configured, AXP PMICs will inject fixed
current (80uA by default) into TS pin and measure the voltage across a
thermistor. The PMIC itself will by default compare this voltage with
predefined thresholds  and disable battery charging whenever
the battery is too hot or too cold.

Alternatively, the TS pin can be configured as general-purpose
ADC input. This mode is not supported by the driver.

This patch allows reading the voltage on the TS pin. It can be then
either processed by userspace or used by kernel consumer like hwmon
ntc thermistor driver.

Signed-off-by: Evgeny Boger <boger@wirenboard.com>
---
 drivers/iio/adc/axp20x_adc.c | 45 +++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index 3e0c0233b431..12d469a52cea 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -186,6 +186,8 @@ static const struct iio_chan_spec axp20x_adc_channels[] = {
 			   AXP20X_BATT_CHRG_I_H),
 	AXP20X_ADC_CHANNEL(AXP20X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
 			   AXP20X_BATT_DISCHRG_I_H),
+	AXP20X_ADC_CHANNEL(AXP20X_TS_IN, "ts_v", IIO_VOLTAGE,
+			   AXP20X_TS_IN_H),
 };
 
 static const struct iio_chan_spec axp22x_adc_channels[] = {
@@ -203,6 +205,8 @@ static const struct iio_chan_spec axp22x_adc_channels[] = {
 			   AXP20X_BATT_CHRG_I_H),
 	AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
 			   AXP20X_BATT_DISCHRG_I_H),
+	AXP20X_ADC_CHANNEL(AXP22X_TS_IN, "ts_v", IIO_VOLTAGE,
+			   AXP22X_TS_ADC_H),
 };
 
 static const struct iio_chan_spec axp813_adc_channels[] = {
@@ -222,6 +226,8 @@ static const struct iio_chan_spec axp813_adc_channels[] = {
 			   AXP20X_BATT_CHRG_I_H),
 	AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
 			   AXP20X_BATT_DISCHRG_I_H),
+	AXP20X_ADC_CHANNEL(AXP813_TS_IN, "ts_v", IIO_VOLTAGE,
+			   AXP288_TS_ADC_H),
 };
 
 static int axp20x_adc_raw(struct iio_dev *indio_dev,
@@ -307,11 +313,36 @@ static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
 		*val2 = 400000;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case AXP20X_TS_IN:
+		/* 0.8 mV per LSB */
+		*val = 0;
+		*val2 = 800000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
 	default:
 		return -EINVAL;
 	}
 }
 
+static int axp22x_adc_scale_voltage(int channel, int *val, int *val2)
+{
+	switch (channel) {
+	case AXP22X_BATT_V:
+		/* 1.1 mV per LSB */
+		*val = 1;
+		*val2 = 100000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case AXP22X_TS_IN:
+		/* 0.8 mV per LSB */
+		*val = 0;
+		*val2 = 800000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
 static int axp813_adc_scale_voltage(int channel, int *val, int *val2)
 {
 	switch (channel) {
@@ -325,6 +356,12 @@ static int axp813_adc_scale_voltage(int channel, int *val, int *val2)
 		*val2 = 100000;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case AXP813_TS_IN:
+		/* 0.8 mV per LSB */
+		*val = 0;
+		*val2 = 800000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
 	default:
 		return -EINVAL;
 	}
@@ -378,12 +415,7 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
 {
 	switch (chan->type) {
 	case IIO_VOLTAGE:
-		if (chan->channel != AXP22X_BATT_V)
-			return -EINVAL;
-
-		*val = 1;
-		*val2 = 100000;
-		return IIO_VAL_INT_PLUS_MICRO;
+		return axp22x_adc_scale_voltage(chan->channel, val, val2);
 
 	case IIO_CURRENT:
 		*val = 0;
@@ -488,6 +520,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
 {
 	switch (mask) {
 	case IIO_CHAN_INFO_OFFSET:
+		/* For PMIC temp only */
 		*val = -2677;
 		return IIO_VAL_INT;
 
-- 
2.25.1

[PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Evgeny Boger]

Most AXPxxx-based reference designs place a 10k NTC thermistor on a
TS pin. axp20x IIO driver now report the voltage of this pin via
additional IIO channel. Add new "ts_v" channel to the channel description.

Signed-off-by: Evgeny Boger <boger@wirenboard.com>
---
 .../devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml b/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml
index e759a5da708d..d6d3d8590171 100644
--- a/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml
@@ -27,6 +27,7 @@ description: |
    8 | batt_v
    9 | batt_chrg_i
   10 | batt_dischrg_i
+  11 | ts_v
 
   AXP22x
   ------
@@ -34,6 +35,7 @@ description: |
    1 | batt_v
    2 | batt_chrg_i
    3 | batt_dischrg_i
+   4 | ts_v
 
   AXP813
   ------
@@ -42,6 +44,7 @@ description: |
    2 | batt_v
    3 | batt_chrg_i
    4 | batt_dischrg_i
+   5 | ts_v
 
 
 properties:
-- 
2.25.1

Re: [PATCH 1/2] iio:adc:axp20x: add support for NTC thermistor

[Maxime Ripard]

On Thu, Nov 18, 2021 at 05:12:32PM +0300, Evgeny Boger wrote:
> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
> TS pin. When appropriately configured, AXP PMICs will inject fixed
> current (80uA by default) into TS pin and measure the voltage across a
> thermistor. The PMIC itself will by default compare this voltage with
> predefined thresholds  and disable battery charging whenever
> the battery is too hot or too cold.
> 
> Alternatively, the TS pin can be configured as general-purpose
> ADC input. This mode is not supported by the driver.
> 
> This patch allows reading the voltage on the TS pin. It can be then
> either processed by userspace or used by kernel consumer like hwmon
> ntc thermistor driver.
> 
> Signed-off-by: Evgeny Boger <boger@wirenboard.com>

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Maxime Ripard]

On Thu, Nov 18, 2021 at 05:12:33PM +0300, Evgeny Boger wrote:
> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
> TS pin. axp20x IIO driver now report the voltage of this pin via
> additional IIO channel. Add new "ts_v" channel to the channel description.
> 
> Signed-off-by: Evgeny Boger <boger@wirenboard.com>

Would it make sense to put the resistance in the DT as well or is it
made mandatory by Allwinner?

Maxime

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Evgeny Boger]

22.11.2021 13:49, Maxime Ripard пишет:
> On Thu, Nov 18, 2021 at 05:12:33PM +0300, Evgeny Boger wrote:
>> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
>> TS pin. axp20x IIO driver now report the voltage of this pin via
>> additional IIO channel. Add new "ts_v" channel to the channel description.
>>
>> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> Would it make sense to put the resistance in the DT as well or is it
> made mandatory by Allwinner?
>
> Maxime
Well, I don't think so. Basically, by default AXP20x injects 80uA current
into the TS pin and measure the voltage. Then, there are voltage thresholds
to stop charging if the battery is too hot or too cold. The default 
thresholds
were calculated by the manufacturer for default 10k resistance and 80uA 
current.
Finally, if TS pin isshorted to GND, the AXP2xx will detect it and won't 
shut

down charging.
Note that AXP2xx doesn't convert the measured voltage to temperature.

So while it's possible to use AXP2xx with resistance other than 10k, it will
require us to override these protection thresholds. Moreover, if one 
want to
put the actual resistance in DT, then the driver would need to calculate 
these
protection thresholds based on NTC parameters and injection current.
I think we better keep things simple and let DT followthe hardware, 
which only
operates in terms of voltage, not temperature and resistance.

-- 

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Samuel Holland]

On 11/22/21 5:17 AM, Evgeny Boger wrote:
> 22.11.2021 13:49, Maxime Ripard пишет:
>> On Thu, Nov 18, 2021 at 05:12:33PM +0300, Evgeny Boger wrote:
>>> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
>>> TS pin. axp20x IIO driver now report the voltage of this pin via
>>> additional IIO channel. Add new "ts_v" channel to the channel
>>> description.
>>>
>>> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
>> Would it make sense to put the resistance in the DT as well or is it
>> made mandatory by Allwinner?
>>
>> Maxime
> Well, I don't think so. Basically, by default AXP20x injects 80uA 
> current into the TS pin and measure the voltage. Then, there are 
> voltage thresholds to stop charging if the battery is too hot or too 
> cold. The default thresholds were calculated by the manufacturer for 
> default 10k resistance and 80uA current. Finally, if TS pin is
> shorted to GND, the AXP2xx will detect it and won't shut down 
> charging. Note that AXP2xx doesn't convert the measured voltage to 
> temperature.

Agreed, since the ADC driver only works with voltages, the resistance is
not relevant to it, so a resistance property does not belong here.

> So while it's possible to use AXP2xx with resistance other than 10k, 
> it will require us to override these protection thresholds.
> Moreover, if one want to put the actual resistance in DT, then the
> driver would need to calculate these protection thresholds based on
> NTC parameters and injection current.
That means we do need a resistance property for the battery charger
driver, because it does need to calculate temperature.

Regardless of the reference design, the resistance is variable in
practice. At least some early v1.0 PinePhones shipped with batteries
containing a 3 kOhm NTC. And the battery is removable, with an
off-the-shelf form factor, so users could install aftermarket batteries
with any NTC resistance.

Right now, people with these batteries are disabling the TS; otherwise
the PMIC refuses to charge them. It would be good to re-enable the TS by
coming up with the proper voltages for the min/max thresholds. And there
are power supply properties we can use to expose the current temperature
and those thresholds to userspace (at least as read-only).

Regards,
Samuel

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Evgeny Boger]

22.11.2021 14:35, Samuel Holland пишет:
> On 11/22/21 5:17 AM, Evgeny Boger wrote:
>> 22.11.2021 13:49, Maxime Ripard пишет:
>>> On Thu, Nov 18, 2021 at 05:12:33PM +0300, Evgeny Boger wrote:
>>>> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
>>>> TS pin. axp20x IIO driver now report the voltage of this pin via
>>>> additional IIO channel. Add new "ts_v" channel to the channel
>>>> description.
>>>>
>>>> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
>>> Would it make sense to put the resistance in the DT as well or is it
>>> made mandatory by Allwinner?
>>>
>>> Maxime
>> Well, I don't think so. Basically, by default AXP20x injects 80uA
>> current into the TS pin and measure the voltage. Then, there are
>> voltage thresholds to stop charging if the battery is too hot or too
>> cold. The default thresholds were calculated by the manufacturer for
>> default 10k resistance and 80uA current. Finally, if TS pin is
>> shorted to GND, the AXP2xx will detect it and won't shut down
>> charging. Note that AXP2xx doesn't convert the measured voltage to
>> temperature.
> Agreed, since the ADC driver only works with voltages, the resistance is
> not relevant to it, so a resistance property does not belong here.
>
>> So while it's possible to use AXP2xx with resistance other than 10k,
>> it will require us to override these protection thresholds.
>> Moreover, if one want to put the actual resistance in DT, then the
>> driver would need to calculate these protection thresholds based on
>> NTC parameters and injection current.
> That means we do need a resistance property for the battery charger
> driver, because it does need to calculate temperature.
Right now the charger driver just doesn't touch the default voltage 
thresholds at all.
> Regardless of the reference design, the resistance is variable in
> practice. At least some early v1.0 PinePhones shipped with batteries
> containing a 3 kOhm NTC. And the battery is removable, with an
> off-the-shelf form factor, so users could install aftermarket batteries
> with any NTC resistance.
I think we can easily expose *voltage* thresholds as DT properties.

>
> Right now, people with these batteries are disabling the TS; otherwise
> the PMIC refuses to charge them. It would be good to re-enable the TS by
> coming up with the proper voltages for the min/max thresholds. And there
> are power supply properties we can use to expose the current temperature
> and those thresholds to userspace (at least as read-only).
So my idea was to convert voltage to temperature elsewhere. Again, I 
personally
think that it makes sense, because the hardware itself (including 
charger) doesn't
deal with temperature at all, only with threshold voltages and injection 
current.

For instance, in  our board we now use hwmon NTC driver, like this:

     /* Huge pullup voltage is here to emulate constant current */
     bat-temp {
         compatible = "murata,ncp15xh103";
         pullup-uv = <1000000000>; // 1E9 uV
         pullup-ohm = <12500000>; // pullup_uv/80
         pulldown-ohm = <0>;
         io-channels = <&axp_adc 4>;
     };

As far as I know, there are IIO AFE patch set under review, which also add
voltage to temperature conversion.

If we indeed want the axp20x charger driver to convert voltage to 
temperature
(and vice versa, for computing charging thresholds), then all that logic 
will need to
be reimplemented in driver. Note that there wouldn't be a single 
"resistance" property,
instead there are huge resistance vs voltage tables which are slightly 
different for
different vendors.

In principle, having temperature reported by power supply driver would 
be great. But
I don't see how it can be implemented without a massive effort.


>
> Regards,
> Samuel


-- 
С уважением,
     Евгений Богер / Evgeny Boger
     CTO, Wiren Board Team
     https://wirenboard.com/ru
     +7 495 150 66 19 (# 33)

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Rob Herring]

On Thu, 18 Nov 2021 17:12:33 +0300, Evgeny Boger wrote:
> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
> TS pin. axp20x IIO driver now report the voltage of this pin via
> additional IIO channel. Add new "ts_v" channel to the channel description.
> 
> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> ---
>  .../devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml       | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Evgeny Boger]

(added linux-pm@ list and maintainers)


Actually, on second though, I think it might be doable to add voltage to 
temperature conversion to this driver.

I think since the NTC thermistor belongs to the battery, not charger, 
the thermistor should be described in monitored battery node.
So I propose to extend battery node (power/supply/battery.yaml) by 
adding something like:

thermistor-resistance-temp-table = <25 10000>, <35 6530>, ...;

This driver will then interpolate between points to report temperature.

We can also adjust PMIC voltage thresholds based on this table and 
"alert-celsius" property already described in battery.yaml.

I think the driver should report raw TS voltage as well, because the TS 
pin can also be used as general-purpose ADC pin.




22.11.2021 14:35, Samuel Holland пишет:
> On 11/22/21 5:17 AM, Evgeny Boger wrote:
>> 22.11.2021 13:49, Maxime Ripard пишет:
>>> On Thu, Nov 18, 2021 at 05:12:33PM +0300, Evgeny Boger wrote:
>>>> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
>>>> TS pin. axp20x IIO driver now report the voltage of this pin via
>>>> additional IIO channel. Add new "ts_v" channel to the channel
>>>> description.
>>>>
>>>> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
>>> Would it make sense to put the resistance in the DT as well or is it
>>> made mandatory by Allwinner?
>>>
>>> Maxime
>> Well, I don't think so. Basically, by default AXP20x injects 80uA
>> current into the TS pin and measure the voltage. Then, there are
>> voltage thresholds to stop charging if the battery is too hot or too
>> cold. The default thresholds were calculated by the manufacturer for
>> default 10k resistance and 80uA current. Finally, if TS pin is
>> shorted to GND, the AXP2xx will detect it and won't shut down
>> charging. Note that AXP2xx doesn't convert the measured voltage to
>> temperature.
> Agreed, since the ADC driver only works with voltages, the resistance is
> not relevant to it, so a resistance property does not belong here.
>
>> So while it's possible to use AXP2xx with resistance other than 10k,
>> it will require us to override these protection thresholds.
>> Moreover, if one want to put the actual resistance in DT, then the
>> driver would need to calculate these protection thresholds based on
>> NTC parameters and injection current.
> That means we do need a resistance property for the battery charger
> driver, because it does need to calculate temperature.
>
> Regardless of the reference design, the resistance is variable in
> practice. At least some early v1.0 PinePhones shipped with batteries
> containing a 3 kOhm NTC. And the battery is removable, with an
> off-the-shelf form factor, so users could install aftermarket batteries
> with any NTC resistance.
>
> Right now, people with these batteries are disabling the TS; otherwise
> the PMIC refuses to charge them. It would be good to re-enable the TS by
> coming up with the proper voltages for the min/max thresholds. And there
> are power supply properties we can use to expose the current temperature
> and those thresholds to userspace (at least as read-only).
>
> Regards,
> Samuel

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Quentin Schulz]

Hi Evgeny,

On Tue, Nov 30, 2021 at 02:58:23AM +0300, Evgeny Boger wrote:
> (added linux-pm@ list and maintainers)
> 
> 
> Actually, on second though, I think it might be doable to add voltage to
> temperature conversion to this driver.
> 
> I think since the NTC thermistor belongs to the battery, not charger, the
> thermistor should be described in monitored battery node.

Then we would duplicate the code in ntc thermistor driver and battery
subsystem (or simple-battery driver I assume), instead of reusing the
ntc thermistor driver.

> So I propose to extend battery node (power/supply/battery.yaml) by adding
> something like:
> 
> thermistor-resistance-temp-table = <25 10000>, <35 6530>, ...;
> 
> This driver will then interpolate between points to report temperature.
> 

Not sure this makes sense, that's the point of the ntc thermistor driver
which does all of this already AFAICT.

The battery node already supports operating-range-celsius and
alert-celsius. This driver should read that and then ask the ntc
thermistor driver what's the voltage of the thermistor associated with
this temperature and then set the register to this value.

What's missing in the ntc thermistor driver and/or its subsystem is the
ability to request a specific temperature to voltage conversion,
unrelated to the current value of the NTC thermistor.

In my head I picture the following:

battery node ----------> axp -------------------------------> ntc
             max/min °C		request °C to V for max/min
			     <-------------------------------
				V for max/min °C

The issue I see in this is that the axp needs to have a phandle to the
ntc thermistor... which does not make much sense from DTS point of view
IMO.

Now.. we could also have the following instead by extending the battery
subsystem:

		battery node --------------> axp
				max/min °C
ntc <----------		     <-------------
	request °C to V		request °C to V
    ---------->		     -------------->
	V for max/min °C	V for max/min °C

which means the battery driver/susbystem would have a phandle to the ntc
thermistor and proxy the °C to V conversionr equest from axp to ntc and
the answer back to the axp.

Obviously, ntc would still be a consumer of axp ts iio channel to
report the battery temp to userspace.

> We can also adjust PMIC voltage thresholds based on this table and
> "alert-celsius" property already described in battery.yaml.
> 
> I think the driver should report raw TS voltage as well, because the TS pin
> can also be used as general-purpose ADC pin.
> 

Raw TS voltage is still reported with your first patch so that's fine.

We'd need to create a pinctrl driver for the AXP to handle the few pins
with multiple fonctions, but it's outside of the scope of this patch
series :)

Regarding the injected current, I don't have enough knowledge in
electronics to understand what exactly would happen to the NTC
thermistor here since it seems the current is not used anywhere in the
formula to get the ohm of the resistor which is then converted to the
actual temperature. At least in the ntc thermistor driver. Also not sure
where this injected current should be declared in the device tree, is it
a limitation of the NTC thermistor (some kind of operating range?) which
should be propagated to the AXP to make it inject more/less current
depending on the NTC thermistor?

This patch is fine in itself since it's required for doing anything more
that is currently discussed in this thread. We can continue discussing
this but I don't think it is preventing this patch to continue its
normal life and be merged :)

Reviewed-by: Quentin Schulz <foss+kernel@0leil.net>

Cheers,
Quentin

Re: [PATCH 1/2] iio:adc:axp20x: add support for NTC thermistor

[Quentin Schulz]

Hi Evgeny,

On Thu, Nov 18, 2021 at 05:12:32PM +0300, Evgeny Boger wrote:
> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
> TS pin. When appropriately configured, AXP PMICs will inject fixed
> current (80uA by default) into TS pin and measure the voltage across a
> thermistor. The PMIC itself will by default compare this voltage with
> predefined thresholds  and disable battery charging whenever

They actually are configurable, it's just that we don't have the means
to configure it currently from the kernel.

"In the diagram above, VTH/VTL refers to the high temperature threshold
and low temperature threshold, which is programmable via registers
REG38H/39H/3CH/3DH respectively." in the AXP209 datasheet, section
"Battery temperature detection".

> the battery is too hot or too cold.
> 
> Alternatively, the TS pin can be configured as general-purpose
> ADC input. This mode is not supported by the driver.
> 
> This patch allows reading the voltage on the TS pin. It can be then
> either processed by userspace or used by kernel consumer like hwmon
> ntc thermistor driver.
> 
> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> ---
>  drivers/iio/adc/axp20x_adc.c | 45 +++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 3e0c0233b431..12d469a52cea 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
[...]
> +static int axp22x_adc_scale_voltage(int channel, int *val, int *val2)
> +{
> +	switch (channel) {
> +	case AXP22X_BATT_V:
> +		/* 1.1 mV per LSB */
> +		*val = 1;
> +		*val2 = 100000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP22X_TS_IN:
> +		/* 0.8 mV per LSB */
> +		*val = 0;
> +		*val2 = 800000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
[...]
> @@ -378,12 +415,7 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
>  {
>  	switch (chan->type) {
>  	case IIO_VOLTAGE:
> -		if (chan->channel != AXP22X_BATT_V)
> -			return -EINVAL;
> -
> -		*val = 1;
> -		*val2 = 100000;
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		return axp22x_adc_scale_voltage(chan->channel, val, val2);
>  

I would actually have made the move to axp22x_adc_scale_voltage function
for AXP22X_BATT_V channel separate from this commit because I was a bit
confused in the original review why suddenly there was an addition for
AXP22X_BATT_V while this patch is about AXP22X_TS_IN.

If maintainers are ok with this, I don't mind.

I don't have any HW to test this but the changes make sense to me:
Reviewed-by: Quentin Schulz <foss+kernel@0leil.net>

Thanks!
Quentin

Re: [PATCH 1/2] iio:adc:axp20x: add support for NTC thermistor

[Quentin Schulz]

Hi Evgeny,

On Thu, Nov 18, 2021 at 05:12:32PM +0300, Evgeny Boger wrote:
> Most AXPxxx-based reference designs place a 10k NTC thermistor on a
> TS pin. When appropriately configured, AXP PMICs will inject fixed
> current (80uA by default) into TS pin and measure the voltage across a
> thermistor. The PMIC itself will by default compare this voltage with
> predefined thresholds  and disable battery charging whenever

They actually are configurable, we just don't have the knobs for this in
the kernel.

"In the diagram above, VTH/VTL refers to the high temperature threshold
and low temperature threshold, which is programmable via registers
REG38H/39H/3CH/3DH respectively. " in AXP209 datasheet, section
"Battery temperature detection".

> the battery is too hot or too cold.
> 
> Alternatively, the TS pin can be configured as general-purpose
> ADC input. This mode is not supported by the driver.
> 
> This patch allows reading the voltage on the TS pin. It can be then
> either processed by userspace or used by kernel consumer like hwmon
> ntc thermistor driver.
> 
> Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> ---
>  drivers/iio/adc/axp20x_adc.c | 45 +++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 3e0c0233b431..12d469a52cea 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
[...]
> +static int axp22x_adc_scale_voltage(int channel, int *val, int *val2)
> +{
> +	switch (channel) {
> +	case AXP22X_BATT_V:
> +		/* 1.1 mV per LSB */
> +		*val = 1;
> +		*val2 = 100000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP22X_TS_IN:
> +		/* 0.8 mV per LSB */
> +		*val = 0;
> +		*val2 = 800000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
>  static int axp813_adc_scale_voltage(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
[...]
> @@ -378,12 +415,7 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
>  {
>  	switch (chan->type) {
>  	case IIO_VOLTAGE:
> -		if (chan->channel != AXP22X_BATT_V)
> -			return -EINVAL;
> -
> -		*val = 1;
> -		*val2 = 100000;
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		return axp22x_adc_scale_voltage(chan->channel, val, val2);
>  

I would have personally split the move to a separate
axp22x_adc_scale_voltage function in a separate commit. I was a bit
confused at first why in the diff above there was a AXP22X_BATT_V
addition since this commit is about AXP22X_TS_IN.

If the maintainers are ok with it, I don't mind too much.

I don't have the HW to test this, but changes look ok.

Reviewed-by: Quentin Schulz <foss+kernel@0leil.net>

Thanks!
Quentin

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Quentin Schulz]

Hi all,

On Tue, Nov 30, 2021 at 02:58:23AM +0300, Evgeny Boger wrote:
> (added linux-pm@ list and maintainers)
> 
> 
> Actually, on second though, I think it might be doable to add voltage to
> temperature conversion to this driver.
> 
> I think since the NTC thermistor belongs to the battery, not charger, the
> thermistor should be described in monitored battery node.
> So I propose to extend battery node (power/supply/battery.yaml) by adding
> something like:
> 
> thermistor-resistance-temp-table = <25 10000>, <35 6530>, ...;
> 
> This driver will then interpolate between points to report temperature.
> 

I disagree, I think it does not make much sense. This is already done by
the NTC thermistor driver.
The battery "subsystem" already provides operating-range-celsius and
alert-celsius properties for that.
Since the battery is linked to the AXP, all we need is to be able to ask
the NTC thermistor driver to do the conversion from temperature to
voltage of the two voltage values we get from the battery and use the
result as threshold in the AXP registers.
I wouldn't want to have the extrapolation done in two different places.

I can see two ways of specifying that interation:

battery -------------------> axp --------------------> ntc
	min/max °C			request °C to V
				 <--------------------
					response V

This however would require a phandle in the AXP to the NTC thermistor
driver and I don't feel like it's that good of an idea?

Another way would be to use the battery as a proxy for the voltage
request to ntc.

		     battery --------------------> axp
				min/max °C
ntc <--------------- 	     <--------------------
	request °C to V		request °C to V
    --------------->	     --------------------->
	response V		response V

This would require a phandle to the ntc thermistor in the battery node,
which kind of makes sense to me. And since the AXP already has knowledge
of the battery, it can request the appropriate value to the battery
which then proxies it to and back from the ntc.

Forgive me for my poor ASCII drawing skills :) hopefully it's good
enough to convey my thoughts.

> We can also adjust PMIC voltage thresholds based on this table and
> "alert-celsius" property already described in battery.yaml.
> 
> I think the driver should report raw TS voltage as well, because the TS pin
> can also be used as general-purpose ADC pin.
> 

Since the ntc anyway needs this raw TS voltage and that patch does that,
I think it's fine. Specifically, re-using this pin as a general-purpose
ADC won't impact the current patchset.

What we'll need is to have a pinctrl driver for the few pins in the AXP
which have multiple functions. But that's outside of the scope of this
patchset.

Regarding the injected current, I don't have enough knowledge in
electronics to understand how this will change things in the thermistor
since in the NTC thermistor driver there's no logic related to the
actual current being injected. Maybe it is related to some operating
value required by the NTC? I can't say unfortunately.

We can continue this discussion but I don't think this should block this
patch as I don't see the outcome of this discussion change anything in
this patchset.

Reviewed-by: Quentin Schulz <foss+kernel@0leil.net>

Thanks!
Quentin

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Evgeny Boger]

Hi Quentin,

thank you for the feedback!

01.12.2021 14:02, Quentin Schulz пишет:
> Hi all,
>
> On Tue, Nov 30, 2021 at 02:58:23AM +0300, Evgeny Boger wrote:
>> (added linux-pm@ list and maintainers)
>>
>>
>> Actually, on second though, I think it might be doable to add voltage to
>> temperature conversion to this driver.
>>
>> I think since the NTC thermistor belongs to the battery, not charger, the
>> thermistor should be described in monitored battery node.
>> So I propose to extend battery node (power/supply/battery.yaml) by adding
>> something like:
>>
>> thermistor-resistance-temp-table = <25 10000>, <35 6530>, ...;
>>
>> This driver will then interpolate between points to report temperature.
>>
> I disagree, I think it does not make much sense. This is already done by
> the NTC thermistor driver.
> The battery "subsystem" already provides operating-range-celsius and
> alert-celsius properties for that.
> Since the battery is linked to the AXP, all we need is to be able to ask
> the NTC thermistor driver to do the conversion from temperature to
> voltage of the two voltage values we get from the battery and use the
> result as threshold in the AXP registers.
> I wouldn't want to have the extrapolation done in two different places.
>
> I can see two ways of specifying that interation:
>
> battery -------------------> axp --------------------> ntc
> 	min/max °C			request °C to V
> 				 <--------------------
> 					response V
>
> This however would require a phandle in the AXP to the NTC thermistor
> driver and I don't feel like it's that good of an idea?
>
> Another way would be to use the battery as a proxy for the voltage
> request to ntc.
>
> 		     battery --------------------> axp
> 				min/max °C
> ntc <--------------- 	     <--------------------
> 	request °C to V		request °C to V
>      --------------->	     --------------------->
> 	response V		response V
>
> This would require a phandle to the ntc thermistor in the battery node,
> which kind of makes sense to me. And since the AXP already has knowledge
> of the battery, it can request the appropriate value to the battery
> which then proxies it to and back from the ntc.
>
> Forgive me for my poor ASCII drawing skills :) hopefully it's good
> enough to convey my thoughts.
I see quite a few problems with NTC driver approach.

The problem is, I don't know any suitable subsystem for that. NTC is not 
a subsystem,
NTC in kernel is a mere hwmon driver, and also is quite an old one.

Also, we already have iio-afe, which, in a sense, already does pretty 
much the same as NTC
hwmon driver. Maybe using iio-afe is the better idea?
But then, I think that's a very complicated interaction for a simple 
interpolation between points.

Another thing is, in our design we ended up using not a simple 10k NTC 
thermistor, but a 10k NTC is series with fixed 2.2k.
The reason why it's needed is that AXP NTC voltage thresholds are fixed 
at startup time, and if we somehow have to deal
with default thresholds to get different behaviour.  So the 
resistance-temperature curve in our case is different from any standard
NTC. Speaking of "standard" NTC, our supplier has like 15 different 
models for *each* resistance, which slightly differ in
resistance-temperature curve. Adding them all into a driver would be 
strange.

Personally, I think better approach with NTCs is to place the 
resistance-temperature tables for bunch of models to .dtsi
files, describe the thermistor node in DT and then make all drivers 
(hwmon NTC, iio-afe, this one) to use this data in the same way
it's done with monitored-battery node.


>
>> We can also adjust PMIC voltage thresholds based on this table and
>> "alert-celsius" property already described in battery.yaml.
>>
>> I think the driver should report raw TS voltage as well, because the TS pin
>> can also be used as general-purpose ADC pin.
>>
> Since the ntc anyway needs this raw TS voltage and that patch does that,
> I think it's fine. Specifically, re-using this pin as a general-purpose
> ADC won't impact the current patchset.
>
> What we'll need is to have a pinctrl driver for the few pins in the AXP
> which have multiple functions. But that's outside of the scope of this
> patchset.
Should it really be pinctrl, though? Unfortunately the choice will alter 
other
functions as well. Say, if we use TS pin in GPADC mode, we'll have to 
disable
temperature thresholds and current injection.
>
> Regarding the injected current, I don't have enough knowledge in
> electronics to understand how this will change things in the thermistor
> since in the NTC thermistor driver there's no logic related to the
> actual current being injected. Maybe it is related to some operating
> value required by the NTC? I can't say unfortunately.
It's basically Ohm's law, so it's not related to the NTC thermistor itself,
but more to the voltage across NTC that the AXP can measure.
Say, if maximum measurable voltage is 3.3V, than the maximum measurable 
resistance
at the given current would be 3.3V/80uA = 41 kOhm. In case of 10k NTC 
it's about -5C or so.

But again, one can't really alter startup voltage thresholds of the AXP. 
And also, regardless of
settings, at least AXP221s will completely disable TS-based protection 
if voltage on TS pin is below 0.2V.
So at the end, unfortunately, there are not so many options when it 
comes to the thermistor and the injection current.
>
> We can continue this discussion but I don't think this should block this
> patch as I don't see the outcome of this discussion change anything in
> this patchset.
>
> Reviewed-by: Quentin Schulz <foss+kernel@0leil.net>
>
> Thanks!
> Quentin

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 6174 bytes --]

Hi,

On Wed, Dec 01, 2021 at 06:45:44PM +0300, Evgeny Boger wrote:
> Hi Quentin,
> 
> thank you for the feedback!
> 
> 01.12.2021 14:02, Quentin Schulz пишет:
> > Hi all,
> > 
> > On Tue, Nov 30, 2021 at 02:58:23AM +0300, Evgeny Boger wrote:
> > > (added linux-pm@ list and maintainers)
> > > 
> > > 
> > > Actually, on second though, I think it might be doable to add voltage to
> > > temperature conversion to this driver.
> > > 
> > > I think since the NTC thermistor belongs to the battery, not charger, the
> > > thermistor should be described in monitored battery node.
> > > So I propose to extend battery node (power/supply/battery.yaml) by adding
> > > something like:
> > > 
> > > thermistor-resistance-temp-table = <25 10000>, <35 6530>, ...;
> > > 
> > > This driver will then interpolate between points to report temperature.
> > > 
> > I disagree, I think it does not make much sense. This is already done by
> > the NTC thermistor driver.
> > The battery "subsystem" already provides operating-range-celsius and
> > alert-celsius properties for that.
> > Since the battery is linked to the AXP, all we need is to be able to ask
> > the NTC thermistor driver to do the conversion from temperature to
> > voltage of the two voltage values we get from the battery and use the
> > result as threshold in the AXP registers.
> > I wouldn't want to have the extrapolation done in two different places.
> > 
> > I can see two ways of specifying that interation:
> > 
> > battery -------------------> axp --------------------> ntc
> > 	min/max °C			request °C to V
> > 				 <--------------------
> > 					response V
> > 
> > This however would require a phandle in the AXP to the NTC thermistor
> > driver and I don't feel like it's that good of an idea?
> > 
> > Another way would be to use the battery as a proxy for the voltage
> > request to ntc.
> > 
> > 		     battery --------------------> axp
> > 				min/max °C
> > ntc <--------------- 	     <--------------------
> > 	request °C to V		request °C to V
> >      --------------->	     --------------------->
> > 	response V		response V
> > 
> > This would require a phandle to the ntc thermistor in the battery node,
> > which kind of makes sense to me. And since the AXP already has knowledge
> > of the battery, it can request the appropriate value to the battery
> > which then proxies it to and back from the ntc.
> > 
> > Forgive me for my poor ASCII drawing skills :) hopefully it's good
> > enough to convey my thoughts.
> I see quite a few problems with NTC driver approach.
> 
> The problem is, I don't know any suitable subsystem for that. NTC
> is not a subsystem, NTC in kernel is a mere hwmon driver, and also
> is quite an old one.
> 
> Also, we already have iio-afe, which, in a sense, already does pretty much
> the same as NTC
> hwmon driver. Maybe using iio-afe is the better idea?
> But then, I think that's a very complicated interaction for a simple
> interpolation between points.
> 
> Another thing is, in our design we ended up using not a simple 10k NTC
> thermistor, but a 10k NTC is series with fixed 2.2k.
> The reason why it's needed is that AXP NTC voltage thresholds are fixed at
> startup time, and if we somehow have to deal
> with default thresholds to get different behaviour.  So the
> resistance-temperature curve in our case is different from any standard
> NTC. Speaking of "standard" NTC, our supplier has like 15 different models
> for *each* resistance, which slightly differ in
> resistance-temperature curve. Adding them all into a driver would be
> strange.
> 
> Personally, I think better approach with NTCs is to place the
> resistance-temperature tables for bunch of models to .dtsi
> files, describe the thermistor node in DT and then make all drivers (hwmon
> NTC, iio-afe, this one) to use this data in the same way
> it's done with monitored-battery node.
> 
> > > We can also adjust PMIC voltage thresholds based on this table and
> > > "alert-celsius" property already described in battery.yaml.
> > > 
> > > I think the driver should report raw TS voltage as well, because the TS pin
> > > can also be used as general-purpose ADC pin.
> > > 
> > Since the ntc anyway needs this raw TS voltage and that patch does that,
> > I think it's fine. Specifically, re-using this pin as a general-purpose
> > ADC won't impact the current patchset.
> > 
> > What we'll need is to have a pinctrl driver for the few pins in the AXP
> > which have multiple functions. But that's outside of the scope of this
> > patchset.
> Should it really be pinctrl, though? Unfortunately the choice will alter
> other
> functions as well. Say, if we use TS pin in GPADC mode, we'll have to
> disable
> temperature thresholds and current injection.
> > 
> > Regarding the injected current, I don't have enough knowledge in
> > electronics to understand how this will change things in the thermistor
> > since in the NTC thermistor driver there's no logic related to the
> > actual current being injected. Maybe it is related to some operating
> > value required by the NTC? I can't say unfortunately.
> It's basically Ohm's law, so it's not related to the NTC thermistor itself,
> but more to the voltage across NTC that the AXP can measure.
> Say, if maximum measurable voltage is 3.3V, than the maximum measurable
> resistance
> at the given current would be 3.3V/80uA = 41 kOhm. In case of 10k NTC it's
> about -5C or so.
> 
> But again, one can't really alter startup voltage thresholds of the AXP. And
> also, regardless of
> settings, at least AXP221s will completely disable TS-based protection if
> voltage on TS pin is below 0.2V.
> So at the end, unfortunately, there are not so many options when it comes to
> the thermistor and the injection current.

Linus W. recently sent a series for NTC support in power-supply
core, please synchronize with him (added to Cc):

https://lore.kernel.org/linux-pm/20211122234141.3356340-1-linus.walleij@linaro.org/

(FWIW I don't have any strong opinion about any solution)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Jonathan Cameron]

On Fri, 3 Dec 2021 21:47:54 +0100
Sebastian Reichel <sebastian.reichel@collabora.com> wrote:

> Hi,
> 
> On Wed, Dec 01, 2021 at 06:45:44PM +0300, Evgeny Boger wrote:
> > Hi Quentin,
> > 
> > thank you for the feedback!
> > 
> > 01.12.2021 14:02, Quentin Schulz пишет:  
> > > Hi all,
> > > 
> > > On Tue, Nov 30, 2021 at 02:58:23AM +0300, Evgeny Boger wrote:  
> > > > (added linux-pm@ list and maintainers)
> > > > 
> > > > 
> > > > Actually, on second though, I think it might be doable to add voltage to
> > > > temperature conversion to this driver.
> > > > 
> > > > I think since the NTC thermistor belongs to the battery, not charger, the
> > > > thermistor should be described in monitored battery node.
> > > > So I propose to extend battery node (power/supply/battery.yaml) by adding
> > > > something like:
> > > > 
> > > > thermistor-resistance-temp-table = <25 10000>, <35 6530>, ...;
> > > > 
> > > > This driver will then interpolate between points to report temperature.
> > > >   
> > > I disagree, I think it does not make much sense. This is already done by
> > > the NTC thermistor driver.
> > > The battery "subsystem" already provides operating-range-celsius and
> > > alert-celsius properties for that.
> > > Since the battery is linked to the AXP, all we need is to be able to ask
> > > the NTC thermistor driver to do the conversion from temperature to
> > > voltage of the two voltage values we get from the battery and use the
> > > result as threshold in the AXP registers.
> > > I wouldn't want to have the extrapolation done in two different places.
> > > 
> > > I can see two ways of specifying that interation:
> > > 
> > > battery -------------------> axp --------------------> ntc
> > > 	min/max °C			request °C to V
> > > 				 <--------------------
> > > 					response V
> > > 
> > > This however would require a phandle in the AXP to the NTC thermistor
> > > driver and I don't feel like it's that good of an idea?
> > > 
> > > Another way would be to use the battery as a proxy for the voltage
> > > request to ntc.
> > > 
> > > 		     battery --------------------> axp
> > > 				min/max °C
> > > ntc <--------------- 	     <--------------------
> > > 	request °C to V		request °C to V  
> > >      --------------->	     --------------------->  
> > > 	response V		response V
> > > 
> > > This would require a phandle to the ntc thermistor in the battery node,
> > > which kind of makes sense to me. And since the AXP already has knowledge
> > > of the battery, it can request the appropriate value to the battery
> > > which then proxies it to and back from the ntc.
> > > 
> > > Forgive me for my poor ASCII drawing skills :) hopefully it's good
> > > enough to convey my thoughts.  

If we were going to do something like this, I'd see the battery as a
consumer of the the temperature measurement from the NTC (might also consume other
things from axp directly).  So it should be

Temperature / events flow.

battery <---temperature----- NTC driver <--Voltage----   axp

Threshold configuration flow

battery --temp thresh-----> NTC driver ---volt thres--> axp

What's missing infrastructure wise is that we don't have an in kernel
interface for IIO events (i.e. the thresholds). It's been discussed
quite a few times in the past, but there has never been a strong enough
reason for anyone to have bothered implementing it.  It wouldn't be
very hard to do if we do need it.  Previous discussions concluded it is
fine to leave demux of events to the consumer unlike the main data flow.
We might refine that in future but for initial usecases that would greatly
simplify the code whilst still allowing multiple consumers from a single
device (which you would need here for example).

These flows are the same we do for data etc for things like
analog accelerometers. That accel driver is a consumer of the ADC
channels as it is using them to convert the voltages to accelerations
that it wants to then present to it's own consumers (typically userspace).


> > I see quite a few problems with NTC driver approach.
> > 
> > The problem is, I don't know any suitable subsystem for that. NTC
> > is not a subsystem, NTC in kernel is a mere hwmon driver, and also
> > is quite an old one.
> > 
> > Also, we already have iio-afe, which, in a sense, already does pretty much
> > the same as NTC
> > hwmon driver. Maybe using iio-afe is the better idea?
> > But then, I think that's a very complicated interaction for a simple
> > interpolation between points.
> > 
> > Another thing is, in our design we ended up using not a simple 10k NTC
> > thermistor, but a 10k NTC is series with fixed 2.2k.
> > The reason why it's needed is that AXP NTC voltage thresholds are fixed at
> > startup time, and if we somehow have to deal
> > with default thresholds to get different behaviour.  So the
> > resistance-temperature curve in our case is different from any standard
> > NTC. Speaking of "standard" NTC, our supplier has like 15 different models
> > for *each* resistance, which slightly differ in
> > resistance-temperature curve. Adding them all into a driver would be
> > strange.
> > 
> > Personally, I think better approach with NTCs is to place the
> > resistance-temperature tables for bunch of models to .dtsi
> > files, describe the thermistor node in DT and then make all drivers (hwmon
> > NTC, iio-afe, this one) to use this data in the same way
> > it's done with monitored-battery node.

Agreed those tables would be needed whatever the solution.  We might
stick to 'standard' tables for simple cases but someone will always wire
a circuit up that does something we haven't thought of.

> >   
> > > > We can also adjust PMIC voltage thresholds based on this table and
> > > > "alert-celsius" property already described in battery.yaml.
> > > > 
> > > > I think the driver should report raw TS voltage as well, because the TS pin
> > > > can also be used as general-purpose ADC pin.
> > > >   
> > > Since the ntc anyway needs this raw TS voltage and that patch does that,
> > > I think it's fine. Specifically, re-using this pin as a general-purpose
> > > ADC won't impact the current patchset.
> > > 
> > > What we'll need is to have a pinctrl driver for the few pins in the AXP
> > > which have multiple functions. But that's outside of the scope of this
> > > patchset.  
> > Should it really be pinctrl, though? Unfortunately the choice will alter
> > other
> > functions as well. Say, if we use TS pin in GPADC mode, we'll have to
> > disable
> > temperature thresholds and current injection.  
> > > 
> > > Regarding the injected current, I don't have enough knowledge in
> > > electronics to understand how this will change things in the thermistor
> > > since in the NTC thermistor driver there's no logic related to the
> > > actual current being injected. Maybe it is related to some operating
> > > value required by the NTC? I can't say unfortunately.  
> > It's basically Ohm's law, so it's not related to the NTC thermistor itself,
> > but more to the voltage across NTC that the AXP can measure.
> > Say, if maximum measurable voltage is 3.3V, than the maximum measurable
> > resistance
> > at the given current would be 3.3V/80uA = 41 kOhm. In case of 10k NTC it's
> > about -5C or so.
> > 
> > But again, one can't really alter startup voltage thresholds of the AXP. And
> > also, regardless of
> > settings, at least AXP221s will completely disable TS-based protection if
> > voltage on TS pin is below 0.2V.
> > So at the end, unfortunately, there are not so many options when it comes to
> > the thermistor and the injection current.  
> 
> Linus W. recently sent a series for NTC support in power-supply
> core, please synchronize with him (added to Cc):
> 
> https://lore.kernel.org/linux-pm/20211122234141.3356340-1-linus.walleij@linaro.org/
> 
> (FWIW I don't have any strong opinion about any solution)
> 
> -- Sebastian

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Jonathan Cameron]

On Mon, 29 Nov 2021 17:10:42 -0600
Rob Herring <robh@kernel.org> wrote:

> On Thu, 18 Nov 2021 17:12:33 +0300, Evgeny Boger wrote:
> > Most AXPxxx-based reference designs place a 10k NTC thermistor on a
> > TS pin. axp20x IIO driver now report the voltage of this pin via
> > additional IIO channel. Add new "ts_v" channel to the channel description.
> > 
> > Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> > ---
> >  .../devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml       | 3 +++
> >  1 file changed, 3 insertions(+)
> >   
> 
> Acked-by: Rob Herring <robh@kernel.org>

Given the broader discussion on how to handle the NTC isn't directly
related to these patches and these will be needed for any solution
anyway, applied to the togreg branch of iio.git and pushed out as testing
to let 0-day have first go at breaking things.

Thanks,

Jonathan

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Linus Walleij]

On Sat, Dec 4, 2021 at 4:21 PM Jonathan Cameron <jic23@kernel.org> wrote:

> If we were going to do something like this, I'd see the battery as a
> consumer of the the temperature measurement from the NTC (might also consume other
> things from axp directly).  So it should be
>
> Temperature / events flow.
>
> battery <---temperature----- NTC driver <--Voltage----   axp

That's the idea.

I think the battery will get a handle on a thermal zone and then
you get the temperature from that.

> Threshold configuration flow
>
> battery --temp thresh-----> NTC driver ---volt thres--> axp

I don't understand this so not commenting.

> > > Personally, I think better approach with NTCs is to place the
> > > resistance-temperature tables for bunch of models to .dtsi
> > > files, describe the thermistor node in DT and then make all drivers (hwmon
> > > NTC, iio-afe, this one) to use this data in the same way
> > > it's done with monitored-battery node.

The DT maintainers are not happy about the device tree being used
as a general data container. The rule of thumb is that things that are
configurable should be in the device tree, things which are hard data
from a datasheet should be in a struct in the driver, and the compatible
string tells you which data to use.

For an NTC the resistance-to-temperature is a clear case of data from
a datasheet and should not be in the device tree but in a table in
the kernel.

> Agreed those tables would be needed whatever the solution.  We might
> stick to 'standard' tables for simple cases but someone will always wire
> a circuit up that does something we haven't thought of.

What we usually do is model the wiring in the device tree like we (I) have
already done with much pain in:
Documentation/devicetree/bindings/hwmon/ntc-thermistor.yaml

> > Linus W. recently sent a series for NTC support in power-supply
> > core, please synchronize with him (added to Cc):
> >
> > https://lore.kernel.org/linux-pm/20211122234141.3356340-1-linus.walleij@linaro.org/

For the type of battery described in
Documentation/devicetree/bindings/power/supply/battery.yaml
a thermistor node inside the battery will be needed and then code
added to the power management core to spawn a OF-based
platform device from that.

battery: battery {
        compatible = "simple-battery";

        ntc-resistor {
                ...
        };
};

For the Samsung batteries my plan is to spawn a platform device from inside the
Samsung battery driver and add pull-down resistor value and
compatible using software nodes from within the kernel.

Since I already have the compatible of the battery itself, I know
which thermistor this is and how it is mounted with an ID resistor
as pull-down and Samsung batteries can just hardcode that from the
kernel without
modeling the resistor in the device tree at all.

Yours,
Linus Walleij

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Evgeny Boger]

Hi Linus!

05.12.2021 04:02, Linus Walleij пишет:
> On Sat, Dec 4, 2021 at 4:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
>> If we were going to do something like this, I'd see the battery as a
>> consumer of the the temperature measurement from the NTC (might also consume other
>> things from axp directly).  So it should be
>>
>> Temperature / events flow.
>>
>> battery <---temperature----- NTC driver <--Voltage----   axp
> That's the idea.
>
> I think the battery will get a handle on a thermal zone and then
> you get the temperature from that.



>
>> Threshold configuration flow
>>
>> battery --temp thresh-----> NTC driver ---volt thres--> axp
> I don't understand this so not commenting.
>
>>>> Personally, I think better approach with NTCs is to place the
>>>> resistance-temperature tables for bunch of models to .dtsi
>>>> files, describe the thermistor node in DT and then make all drivers (hwmon
>>>> NTC, iio-afe, this one) to use this data in the same way
>>>> it's done with monitored-battery node.
> The DT maintainers are not happy about the device tree being used
> as a general data container. The rule of thumb is that things that are
> configurable should be in the device tree, things which are hard data
> from a datasheet should be in a struct in the driver, and the compatible
> string tells you which data to use.
>
> For an NTC the resistance-to-temperature is a clear case of data from
> a datasheet and should not be in the device tree but in a table in
> the kernel.
>
>> Agreed those tables would be needed whatever the solution.  We might
>> stick to 'standard' tables for simple cases but someone will always wire
>> a circuit up that does something we haven't thought of.
> What we usually do is model the wiring in the device tree like we (I) have
> already done with much pain in:
> Documentation/devicetree/bindings/hwmon/ntc-thermistor.yaml
Earlier in this thread I mentioned our use case with AXP221s: we ended 
up wiring 10k NTC in series with 12k fixed resistance.
The only reason was to work around poorly-designed AXP NTC detection 
"feature", which would turn off protection whenever
the voltage on TS pin drops below 0.2V.

How would you suggest to handle such a wiring?

>
>>> Linus W. recently sent a series for NTC support in power-supply
>>> core, please synchronize with him (added to Cc):
>>>
>>> https://lore.kernel.org/linux-pm/20211122234141.3356340-1-linus.walleij@linaro.org/
> For the type of battery described in
> Documentation/devicetree/bindings/power/supply/battery.yaml
> a thermistor node inside the battery will be needed and then code
> added to the power management core to spawn a OF-based
> platform device from that.
>
> battery: battery {
>          compatible = "simple-battery";
>
>          ntc-resistor {
>                  ...
>          };
> };
>
> For the Samsung batteries my plan is to spawn a platform device from inside the
> Samsung battery driver and add pull-down resistor value and
> compatible using software nodes from within the kernel.
In this example, the ntc-resistor node will be handled by current NTC 
hwmon driver, right?
Frankly, I'm quite confused about hwmon vs iio choice in this case.
Wouldn't it be better to use iio here, say, by extending iio-afe?



>
> Since I already have the compatible of the battery itself, I know
> which thermistor this is and how it is mounted with an ID resistor
> as pull-down and Samsung batteries can just hardcode that from the
> kernel without
> modeling the resistor in the device tree at all.
As for pull-down it's the same for AXP, the only difference is there 
will be fixed injection current instead.
But you still need to describe the particular NTC model in device tree, 
right?
>
> Yours,
> Linus Walleij


-- 
С уважением,
     Евгений Богер / Evgeny Boger
     CTO, Wiren Board Team
     https://wirenboard.com/ru
     +7 495 150 66 19 (# 33)

Re: [PATCH 2/2] dt-bindings: iio: adc: document TS voltage in AXP PMICs

[Linus Walleij]

On Sun, Dec 5, 2021 at 11:50 AM Evgeny Boger <boger@wirenboard.com> wrote:

> >> Agreed those tables would be needed whatever the solution.  We might
> >> stick to 'standard' tables for simple cases but someone will always wire
> >> a circuit up that does something we haven't thought of.
> > What we usually do is model the wiring in the device tree like we (I) have
> > already done with much pain in:
> > Documentation/devicetree/bindings/hwmon/ntc-thermistor.yaml
>
> Earlier in this thread I mentioned our use case with AXP221s: we ended
> up wiring 10k NTC in series with 12k fixed resistance.
> The only reason was to work around poorly-designed AXP NTC detection
> "feature", which would turn off protection whenever
> the voltage on TS pin drops below 0.2V.
>
> How would you suggest to handle such a wiring?

Correct me if I'm wrong but it is pretty standard to connect an extra
resistor in series with a thermistor, for example batteries often have
an BTI "Battery Type Indicator" resistor in series with the thermistor.

If you check the NTC bindings:
Documentation/devicetree/bindings/hwmon/ntc-thermistor.yaml
they already cover this, in two different ways depending of how
the voltage is measured and if the resistor is in series with the
thermistor to ground or to VCC.

The thermistor table goes in the driver for the 10k resistor
and the 12k resistor is pullup-ohm or pulldown-ohm.

> > battery: battery {
> >          compatible = "simple-battery";
> >
> >          ntc-resistor {
> >                  ...
> >          };
> > };
> >
> > For the Samsung batteries my plan is to spawn a platform device from inside the
> > Samsung battery driver and add pull-down resistor value and
> > compatible using software nodes from within the kernel.
>
> In this example, the ntc-resistor node will be handled by current NTC
> hwmon driver, right?

Yes

> Frankly, I'm quite confused about hwmon vs iio choice in this case.
> Wouldn't it be better to use iio here, say, by extending iio-afe?

The hwmon driver uses IIO for voltage sampling. It is in the hwmon
driver folder because the driver provides a temperature and for
historical reasons: it was used for hwmon use cases.

It is possible to refactor it to a driver in
drivers/iio/temperature/* if so desired, and make sure all users
are moved over and the Kconfig symbols are resolved to the
new driver. After that it can be used by hwmon using the
iio-to-hwmon bridge driver.

It's just work, someone who want to fix it can step in and submit
the patches and drive the change.

> But you still need to describe the particular NTC model in device tree,
> right?

No. Drivers can spawn devices including fwnode properties that will
work identical to a device spawn from a device tree node.

We can instatiate an NTC as a subdevice of a battery without
the need of sich a NTC node in the device tree, This is the
ambition of the fwnode concept,

Yours,
Linus Walleij