On Tue, 26 Oct 2021 at 01:45, Guenter Roeck [off-list ref] wrote:

On 10/24/21 11:27 PM, Nathan Rossi wrote:
[ ... ]

quoted

quoted

Is there reason to believe that the shunt register value needs to be visible
and writeable with sysfs attributes ? This is quite unusual nowadays.
If so, please provide a use case.

I do not have a specific use case for being able to change the shunt
resistor at run time. The only reason this behaviour is here is to
mirror the api that is provided by the ina2xx driver. Since as you
mention its unusual should I remove the write and leave the read?
Being able to determine the resistor value is useful if manually using
the shunt voltage. Though the shunt information could be obtained from
the device tree node?

Please drop the attribute. There is already probe noise displaying it,
and it is contained in the devicetree data. If there is a use case,
the attribute can always be added later.

[ ... ]

quoted

quoted

Those preinitializations make me wonder if there should be devicetree
properties for at least some of the data.

Yes, I did consider adding configuration for the conversion time and
sampling average as device tree properties. The existing ina2xx driver
handles configuring sampling average via the "update_interval" sysfs
attribute. Our use case does not require changing these at runtime so
did not implement the update_interval and was unsure if changes to
device tree bindings would make sense. Should these be device tree
properties? If yes, should the other ina drivers be updated to support
the properties?

I wasn't specifically thinking about conversion time or sampling average,
but I do think it would be desirable to be able to configure all those
values via devicetree. The datasheet says "... allows for robust operation
in a variety of noisy environments", and that is definitely a system property.
ADCRANGE should also be configurable via devicetree. The same applies to MODE,

I will add a property "ti,shunt-gain". Since the ina209, ina219,
ina220 use the PGA term which is the actual function being provided,
where ADCRANGE=0 is a /4 PGA for ina238 and the other devices have /1,
/2, /4, /8.

but that would add some complexity so I am not sure if you'd want to get
into that (it would require per-channel entries in devicetree to be able
to enable/disable each channel). FWIW, you _could_ also do that with
standard sysfs attributes if you want to ({in,curr,temp}X_enable, or
hwmon_{temp,in,curr}_enable in the with_info API). That can also be added
later if needed, so there is no need to do it now if it is not required
for your use case.

As for other ina drivers - that is a different question. I would not touch
those unless you have a use case (and a means to test the code). I'd also
consider it more important to convert those drivers to use the _with_info
API before implementing any other changes. There is also the added
complexity that we already have two drivers for those other chips (see
drivers/iio/adc/ina2xx-adc.c), and I'd rather have a better API
between IIO and HWMON and drop drivers/hwmon/ina2xx.c entirely than
making changes to it.

Can you possibly send me a register dump for the INA238 ? That would
enable me to write a module test for the driver.

Dump of registers below. Other notes, upper two bits are ignored so
the address space wraps at 0x40, etc. The undocumented/unused
registers between 0x11 and 0x3e are 0xffff.

power on, before probe:
0x00: 0x0000
0x01: 0xfb68
0x02: 0x1000
0x03: 0xffff
0x04: 0x087c
0x05: 0x0e77
0x06: 0x0f10
0x07: 0x087c
0x08: 0x01eac3
0x09: 0xffff
0x10: 0x7ff0
0x11: 0xffff
0x3e: 0x5449
0x3f: 0x2381

after probe, actual state of device ~10mV shunt voltage, 20 mOhm
shunt, ~12V bus, ~6W power, ~30C temperature
0x00: 0x0000
0x01: 0xfb6a
0x02: 0x4000
0x03: 0xffff
0x04: 0x087c
0x05: 0x0e77
0x06: 0x0f10
0x07: 0x087c
0x08: 0x01eac3
0x09: 0xffff
0x10: 0x7ff0
0x11: 0xffff
0x3e: 0x5449
0x3f: 0x2381

Thanks,
Nathan