Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH 2/2] hwmon: Driver for Texas Instruments INA238 | Date | Mon, 25 Oct 2021 08:45:27 -0700 |
| |
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
| |