Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238

[Guenter Roeck]

On 10/24/21 11:27 PM, Nathan Rossi wrote:
[ ... ]
> > 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.

[ ... ]

> > 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,
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.

Thanks,
Guenter