Re: [PATCH 1/2] hwmon: (core) Add hwmon_mode structure and mode sysfs node

[Guenter Roeck]

Hi Nicolin,

On Wed, Oct 10, 2018 at 02:13:57PM -0700, Nicolin Chen wrote:
> Hi Guenter,
> 
> On Wed, Oct 10, 2018 at 06:08:30AM -0700, Guenter Roeck wrote:
> > > +available_modes The available operating modes of the chip.
> > > +		This should be short, lowercase string, not containing
> > > +		whitespace, or the wildcard character '*'.
> > > +		This attribute shows all the available of the operating modes,
> > > +		for example, "power-down" "one-shot" and "continuous".
> > > +		RO
> > > +
> > > +mode		The current operating mode of the chip.
> > > +		This should be short, lowercase string, not containing
> > > +		whitespace, or the wildcard character '*'.
> > > +		This attribute shows the current operating mode of the chip.
> > > +		Writing a valid string from the list of available_modes will
> > > +		configure the chip to the corresponding operating mode.
> > > +		RW
> > > +
> 
> > This is not a well defined ABI: The modes would be under full and arbitrary
> > control by drivers, and be completely driver dependent. It isn't just the sysfs
> > attribute that makes the ABI, it is also the contents.
> 
> > Also, being able to set the mode itself (for whatever definition of mode)
> > is of questionable value. This is not only for the modes suggested here, but
> > for other possible modes such as comparator mode vs. interrupt mode (which,
> > if configurable, should be via platform data or devicetree node entries).
> > For the modes suggested here, more in the other patch.
> 
> I could foresee an objection here but still wrote the change after
> seeing quite a few drivers (especially TI's chips) share the same
> pattern for operating modes: power-down, one-shot and continuous.
> For example, I could add it to ina3221 driver instead of touching
> the core code, but later I would do the same for the ina2xx driver
> (just received a board having ina230/226.)
> 
Most hardware monitoring chips have the functionality. That doesn't
mean that it makes sense to use/expose it.

> Although I don't mind doing this and will put it to ina3221 driver
> in v2, yet maybe we could think about a better way to abstract it?
> 

My comments to patch 2/2 still apply. Powerdown duplicates existing and
standardized functionality, one-shot mode is not as simple as just enabling
the mode, and I find it quite unlikely to one-shot mode would actually
save any energy.

Thanks,
Guenter