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