Re: AW: [PATCH 0/2] hwmon: ina238: Add samples and update_interval support

[Guenter Roeck]

On Tue, May 26, 2026 at 09:08:18AM +0000, Ferdinand Schwenk wrote:
> On 5/22/26 13:18, Guenter Roeck wrote:
> > On 5/22/26 00:03, Ferdinand Schwenk via B4 Relay wrote:
> > > This series exposes the INA238 ADC_CONFIG register's averaging count
> > > (AVG) and conversion time fields (VBUSCT, VSHCT, VTCT) through the
> > > standard hwmon chip-level attributes chip/samples and
> > > chip/update_interval.
> > >
> > > The first patch adds read/write support for both attributes using a
> > > per-chip conversion-time lookup table to correctly handle all
> > > supported variants: INA228, INA237, INA238, INA700, INA780 (sharing
> > > ina238_conv_time[]) and the Silergy SQ52206 (sq52206_conv_time[]).
> > >
> > > The second patch uses microseconds as the unit for update_interval
> > > instead of milliseconds.  The INA238 supports conversion times as
> > > short as 50 us; millisecond precision would make the four shortest
> > > steps (50, 84, 150, 280 us) indistinguishable and inaccessible.
> > > Since chip/update_interval is introduced in this same series, no
> > > existing ABI is broken.
> > >
> > 
> > Are you serious ? The hwmon ABI says clearly that the unit is milliseconds.
> > 
> > What:           /sys/class/hwmon/hwmonX/update_interval
> > Description:
> >                  The interval at which the chip will update readings.
> >                  Unit: millisecond
> > 
> > It doesn't matter if attribute support is introduced with the series
> > or not. It is still an ABI violation.
> > 
> > Guenter
> 
> Thanks for the clarification. I understand and accept that update_interval must
> be in milliseconds.
> 
> This raises the question of how to handle hardware whose native resolution
> falls below one millisecond. The INA238 has eight conversion steps from 50 µs
> to 4120 µs — atmillisecond resolution the four shortest steps (50, 84, 150,
> 280 µs) collapse to 1 ms and become indistinguishable from userspace. The same
> issue already affects the ina3221 driver in mainline, whose conversion time
> table starts at 140, 204, 332 µs.
> 
> Would adding a `update_interval_us` attribute alongside `update_interval` be an
> acceptable path for hwmon? This pattern has precedent in the kernel: the 1-Wire
> subsystem added `w1_master_timeout_us` alongside the existing
> `w1_master_timeout` when hardware required finer resolution (commit c30983569272).
> 
Yes, that would be acceptable.

Guenter

> Ferdinand