Re: [PATCH v2 2/2] iio: adc: adding support for pac193x

From: <Marius.Cristea@microchip.com>
To: <rgetz@mathworks.com>, <jic23@kernel.org>
Cc: <devicetree@vger.kernel.org>, <conor+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <lars@metafoo.de>,
	<linux-iio@vger.kernel.org>, <robh+dt@kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: adding support for pac193x
Date: Tue, 7 Nov 2023 12:55:48 +0000	[thread overview]
Message-ID: <710b749db585689eb959ccc00a0230a5059b424f.camel@microchip.com> (raw)
In-Reply-To: <BYAPR05MB4424FBC5CFF90F55198C4351A9A6A@BYAPR05MB4424.namprd05.prod.outlook.com>

Hi Robin,

On Thu, 2023-11-02 at 14:28 +0000, Robin Getz wrote:
> [You don't often get email from rgetz@mathworks.com. Learn why this
> is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Friday, October 27, 2023 11:18 AM
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Wed, 25 Oct 2023 16:44:04 +0300
> > <mailto:marius.cristea@microchip.com> wrote:
> > 
> > > From: Marius Cristea <mailto:marius.cristea@microchip.com>
> > > 
> > > This is the iio driver for Microchip
> > > PAC193X series of Power Monitor with Accumulator chip family.
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
> > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
> > > new file mode 100644
> > > index 000000000000..ea43df292b9c
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
> > > @@ -0,0 +1,15 @@
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_shunt_resistor_X
> > > +KernelVersion: 6.6
> > > +Contact: mailto:linux-iio@vger.kernel.org
> > > +Description:
> > > + The value of the shunt resistor may be known only at runtime
> > > and set
> > > + by a client application.
> 
> What? End users (people with access to userspace) don't whip out
> their soldering iron to add/change shunt resistors.
> 

Yes, end users will not change the hardware (most of the time).

> OEMs do this during PCB design. This is fixed per board in the wild
> (apart from device evaluation boards)
> and is typically managed via device tree. (Allowing it to be changed
> by OEMs, but not end users).
> 
> > > This attribute allows to set its value
> > > + in micro-ohms. X is the IIO index of the device. The value is
> > > + used to calculate current, power and accumulated energy.
> > 
> > How common is it that this isn't known?
> 
> I would say zero.

Also, in the case of computer boards the resistor is known, and it
could be set from the device tree.

> 
> The ADM1177 (which is different, but also requires a shunt resistor),
> hwmon driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/adm1177.c
> iio driver:
> https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/adm1177.c
> 
> uses device tree to manage the value of the shunt resistor.
> 
> > I'm not sure we've found it necessary to
> > support userspace control of this for any other device and there
> > are quite a few
> > where this could in theory be known only at runtime...
> 
> Not run time, but at PCB manufacturing time, when the device tree is
> compiled by the OEM.
> The assumption is - who ever controls the BOM - controls the device
> tree. This has been pretty true in my history.
> 
> It allows OEMs (who manage the hardware) to change it as they want,
> but keeps the complexity away from end users.
> 

Here there are two kinds of customers (that were asking for this
functionality) for some particular use cases.

The first category of customers was asking to change the resistor value
from the userspace to use a lower cost resistor, with lower precision,
(the nominal value will still be used into the device tree) but to
leave the possibility to calibrate the system and update the resistor
value at runtime. Calibrated value will be kept into an eeprom. 

The second type of customers are using a modular design (the
exchangeable module will contain also the PAC chip). The module design
was made to support different currents (different order of magnitude)
like battery banks. At runtime/boot time you need to identify the
module used/inserted in the field and set the shunt resistor
accordingly.

The "in_shunt_resistor" property is also used by:
drivers/iio/adc/ina2xx-adc.c
drivers/iio/adc/rtq6056.c

> - Robin
> 

Thanks,
Marius