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
