| Message ID | 20211118141233.247907-3-boger@wirenboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: axp20x: add support for NTC thermistor | expand |
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
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. --
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
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
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>
(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
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
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
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
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
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
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
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
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
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
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:
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(+)