Hello Guenter,
On Wed, Oct 10, 2018 at 06:22:39AM -0700, Guenter Roeck wrote:
> > The hwmon core now has a new optional mode interface. So this patch
> > just implements this mode support so that user space can check and
> > configure via sysfs node its operating modes: power-down, one-shot,
> > and continuous modes.
> One-shot mode on its own does not make sense or add value: It would require
> explicit driver support to trigger a reading, wait for the result, and
> report it back to the user. If the intent here is to have the user write the
> mode (which triggers the one-shot reading), wait a bit, and then read the
> results, that doesn't make sense because standard userspace applications
> won't know that. Also, that would be unsynchronized - one has to read the
> CVRF bit in the mask/enable register to know if the reading is complete.
I think I oversimplified the one-shot mode here and you are right:
there should be a one-shot reading routine; the conversion time in
the configuration register also needs to be taken care of.
> The effort to do all this using CPU cycles would in most if not all cases
> outweigh any perceived power savings. As such, I just don't see the
> practical use case.
It really depends on the use case and how often the one-shot gets
triggered. For battery-powered devices, running in the continuous
mode does consume considerable power based on the measurement from
our power folks. If a system is running in a power sensitive mode,
while it still needs to occasionally check the inputs, it could be
a use case for one-shot mode, though it's purely a user decision.
> power-down mode effectively reinvents runtime power management (runtime
> suspend/resume support) and is thus simply unacceptable.
Similar to one-shot, if a system is in a low power mode where it
doesn't want to check the inputs anymore, I feel the user space
could at least make the decision to turn on/off the chips, I am
not quite sure if the generic runtime PM system already has this
kind of support though.
> I am open to help explore adding support for runtime power management
> to the hwmon subsystem, but that would be less than straightforward and
> require an actual use case to warrant the effort.
Is there any feasible solution from your point of view?
Thanks
Nicolin
----
>
> As such, NACK, sorry.
>
> Thanks,
> Guenter
>
> > Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
> > ---
> > drivers/hwmon/ina3221.c | 64 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> > index d61688f04594..5218fd85506d 100644
> > --- a/drivers/hwmon/ina3221.c
> > +++ b/drivers/hwmon/ina3221.c
> > @@ -77,6 +77,28 @@ enum ina3221_channels {
> > INA3221_NUM_CHANNELS
> > };
> > +enum ina3221_modes {
> > + INA3221_MODE_POWERDOWN,
> > + INA3221_MODE_ONESHOT,
> > + INA3221_MODE_CONTINUOUS,
> > + INA3221_NUM_MODES,
> > +};
> > +
> > +static const char *ina3221_mode_names[INA3221_NUM_MODES] = {
> > + [INA3221_MODE_POWERDOWN] = "power-down",
> > + [INA3221_MODE_ONESHOT] = "one-shot",
> > + [INA3221_MODE_CONTINUOUS] = "continuous",
> > +};
> > +
> > +static const u16 ina3221_mode_val[] = {
> > + [INA3221_MODE_POWERDOWN] = INA3221_CONFIG_MODE_POWERDOWN,
> > + [INA3221_MODE_ONESHOT] = INA3221_CONFIG_MODE_SHUNT |
> > + INA3221_CONFIG_MODE_BUS,
> > + [INA3221_MODE_CONTINUOUS] = INA3221_CONFIG_MODE_CONTINUOUS |
> > + INA3221_CONFIG_MODE_SHUNT |
> > + INA3221_CONFIG_MODE_BUS,
> > +};
> > +
> > /**
> > * struct ina3221_input - channel input source specific information
> > * @label: label of channel input source
> > @@ -386,9 +408,51 @@ static const struct hwmon_ops ina3221_hwmon_ops = {
> > .write = ina3221_write,
> > };
> > +static int ina3221_mode_get_index(struct device *dev, unsigned int *index)
> > +{
> > + struct ina3221_data *ina = dev_get_drvdata(dev);
> > + u16 mode = ina->reg_config & INA3221_CONFIG_MODE_MASK;
> > +
> > + if (mode == INA3221_CONFIG_MODE_POWERDOWN)
> > + *index = INA3221_MODE_POWERDOWN;
> > + if (mode & INA3221_CONFIG_MODE_CONTINUOUS)
> > + *index = INA3221_MODE_CONTINUOUS;
> > + else
> > + *index = INA3221_MODE_ONESHOT;
> > +
> > + return 0;
> > +}
> > +
> > +static int ina3221_mode_set_index(struct device *dev, unsigned int index)
> > +{
> > + struct ina3221_data *ina = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> > + INA3221_CONFIG_MODE_MASK,
> > + ina3221_mode_val[index]);
> > + if (ret)
> > + return ret;
> > +
> > + /* Cache the latest config register value */
> > + ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct hwmon_mode ina3221_hwmon_mode = {
> > + .names = ina3221_mode_names,
> > + .list_size = INA3221_NUM_MODES,
> > + .get_index = ina3221_mode_get_index,
> > + .set_index = ina3221_mode_set_index,
> > +};
> > +
> > static const struct hwmon_chip_info ina3221_chip_info = {
> > .ops = &ina3221_hwmon_ops,
> > .info = ina3221_info,
> > + .mode = &ina3221_hwmon_mode,
> > };
> > /* Extra attribute groups */
> >
>
