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.)
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?
Thank you
Nicolin
------
> In short, NACK. I am open to enhancing the ABI, but I don't see the value
> of this attribute.
>
> Guenter
>
>
> > update_interval The interval at which the chip will update readings.
> > Unit: millisecond
> > RW
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index 975c95169884..5a33b616284b 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> > @@ -72,25 +72,88 @@ name_show(struct device *dev, struct device_attribute
> > *attr, char *buf)
> > }
> > static DEVICE_ATTR_RO(name);
> > +static ssize_t available_modes_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > + const struct hwmon_chip_info *chip = hwdev->chip;
> > + const struct hwmon_mode *mode = chip->mode;
> > + int i;
> > +
> > + for (i = 0; i < mode->list_size; i++)
> > + snprintf(buf, PAGE_SIZE, "%s%s ", buf, mode->names[i]);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", buf);
> > +}
> > +static DEVICE_ATTR_RO(available_modes);
> > +
> > +static ssize_t mode_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > + const struct hwmon_chip_info *chip = hwdev->chip;
> > + const struct hwmon_mode *mode = chip->mode;
> > + unsigned int index;
> > + int ret;
> > +
> > + ret = mode->get_index(dev, &index);
> > + if (ret)
> > + return ret;
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", mode->names[index]);
> > +}
> > +
> > +static ssize_t mode_store(struct device *dev, struct device_attribute
> > *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > + const struct hwmon_chip_info *chip = hwdev->chip;
> > + const struct hwmon_mode *mode = chip->mode;
> > + const char **names = mode->names;
> > + unsigned int index;
> > + int ret;
> > +
> > + /* Get the corresponding mode index */
> > + for (index = 0; index < mode->list_size; index++) {
> > + if (!strncmp(buf, names[index], strlen(names[index])))
> > + break;
> > + }
> > +
> > + if (index >= mode->list_size)
> > + return -EINVAL;
> > +
> > + ret = mode->set_index(dev, index);
> > + if (ret)
> > + return ret;
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(mode);
> > +
> > static struct attribute *hwmon_dev_attrs[] = {
> > - &dev_attr_name.attr,
> > + &dev_attr_name.attr, /* index = 0 */
> > + &dev_attr_available_modes.attr, /* index = 1 */
> > + &dev_attr_mode.attr, /* index = 2 */
> > NULL
> > };
> > -static umode_t hwmon_dev_name_is_visible(struct kobject *kobj,
> > - struct attribute *attr, int n)
> > +static umode_t hwmon_dev_is_visible(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > {
> > struct device *dev = container_of(kobj, struct device, kobj);
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > - if (to_hwmon_device(dev)->name == NULL)
> > - return 0;
> > + if (n == 0 && hwdev->name)
> > + return attr->mode;
> > + else if (n <= 2 && hwdev->chip->mode)
> > + return attr->mode;
> > - return attr->mode;
> > + return 0;
> > }
> > static const struct attribute_group hwmon_dev_attr_group = {
> > .attrs = hwmon_dev_attrs,
> > - .is_visible = hwmon_dev_name_is_visible,
> > + .is_visible = hwmon_dev_is_visible,
> > };
> > static const struct attribute_group *hwmon_dev_attr_groups[] = {
> > @@ -591,6 +654,16 @@ __hwmon_device_register(struct device *dev, const char
> > *name, void *drvdata,
> > struct attribute **attrs;
> > int ngroups = 2; /* terminating NULL plus &hwdev->groups */
> > + /* Validate optional hwmon_mode */
> > + if (chip->mode) {
> > + /* Check mandatory properties */
> > + if (!chip->mode->names || !chip->mode->list_size ||
> > + !chip->mode->get_index || !chip->mode->set_index) {
> > + err = -EINVAL;
> > + goto free_hwmon;
> > + }
> > + }
> > +
> > if (groups)
> > for (i = 0; groups[i]; i++)
> > ngroups++;
> > diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> > index 99e0c1b0b5fb..06c1940ca98b 100644
> > --- a/include/linux/hwmon.h
> > +++ b/include/linux/hwmon.h
> > @@ -365,14 +365,38 @@ struct hwmon_channel_info {
> > const u32 *config;
> > };
> > +/**
> > + * Chip operating mode information
> > + * @names: A list of available operating mode names.
> > + * @list_size: The total number of available operating mode names.
> > + * @get_index: Callback to get current operating mode index. Mandatory.
> > + * Parameters are:
> > + * @dev: Pointer to hardware monitoring device
> > + * @index: Pointer to returned mode index
> > + * The function returns 0 on success or a negative error number.
> > + * @set_index: Callback to set operating mode using the index.
> > Mandatory.
> > + * Parameters are:
> > + * @dev: Pointer to hardware monitoring device
> > + * @index: Mode index in the mode list
> > + * The function returns 0 on success or a negative error number.
> > + */
> > +struct hwmon_mode {
> > + const char **names;
> > + unsigned int list_size;
> > + int (*get_index)(struct device *dev, unsigned int *index);
> > + int (*set_index)(struct device *dev, unsigned int index);
> > +};
> > +
> > /**
> > * Chip configuration
> > * @ops: Pointer to hwmon operations.
> > * @info: Null-terminated list of channel information.
> > + * @mode: Pointer to hwmon operating mode (optional).
> > */
> > struct hwmon_chip_info {
> > const struct hwmon_ops *ops;
> > const struct hwmon_channel_info **info;
> > + const struct hwmon_mode *mode;
> > };
> > /* hwmon_device_register() is deprecated */
> >
>
