Re: [v3,10/12] hwmon (occ): Add non-hwmon attributes

[Guenter Roeck]

On Mon, Nov 20, 2017 at 05:53:39PM -0600, eaja...@linux.vnet.ibm.com wrote:
> From: "Edward A. James" 
> 
> Create device attributes for additional OCC properties that do not
> belong as hwmon sensors. These provide additional information as to the
> state of the processor and system.
> 
... and those attributes should really not be part of the hwmon driver.

> Signed-off-by: Edward A. James 
> ---
>  drivers/hwmon/occ/common.c | 93 
> ++
>  drivers/hwmon/occ/common.h |  1 +
>  drivers/hwmon/occ/p8_i2c.c | 10 +
>  drivers/hwmon/occ/p9_sbe.c |  2 +
>  4 files changed, 106 insertions(+)
> 
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 337f286b..53e3592 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -24,6 +24,14 @@
>  
>  #define OCC_FRU_TYPE_VRM 0x3
>  
> +/* OCC status bits */
> +#define OCC_STAT_MASTER  0x80
> +#define OCC_STAT_ACTIVE  0x01
> +#define OCC_EXT_STAT_DVFS_OT 0x80
> +#define OCC_EXT_STAT_DVFS_POWER  0x40
> +#define OCC_EXT_STAT_MEM_THROTTLE0x20
> +#define OCC_EXT_STAT_QUICK_DROP  0x10
> +
>  /* OCC sensor type and version definitions */
>  
>  struct temp_sensor_1 {
> @@ -1108,6 +1116,81 @@ static int occ_setup_sensor_attrs(struct occ *occ)
>   return 0;
>  }
>  
> +static ssize_t occ_show_status(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + int rc;
> + int val = 0;
> + struct occ *occ = dev_get_drvdata(dev);
> + struct occ_poll_response_header *header;
> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +
> + rc = occ_update_response(occ);
> + if (rc)
> + return rc;
> +
> + header = (struct occ_poll_response_header *)occ->resp.data;
> +
> + switch (sattr->index) {
> + case 0:
> + val = (header->status & OCC_STAT_MASTER) ? 1 : 0;
> + break;
> + case 1:
> + val = (header->status & OCC_STAT_ACTIVE) ? 1 : 0;
> + break;
> + case 2:
> + val = (header->ext_status & OCC_EXT_STAT_DVFS_OT) ? 1 : 0;
> + break;
> + case 3:
> + val = (header->ext_status & OCC_EXT_STAT_DVFS_POWER) ? 1 : 0;
> + break;
> + case 4:
> + val = (header->ext_status & OCC_EXT_STAT_MEM_THROTTLE) ? 1 : 0;
> + break;
> + case 5:
> + val = (header->ext_status & OCC_EXT_STAT_QUICK_DROP) ? 1 : 0;
> + break;
> + case 6:
> + val = header->occ_state;
> + break;
> + case 7:
> + if (header->status & OCC_STAT_MASTER)
> + val = hweight8(header->occs_present);
> + else
> + val = 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +static SENSOR_DEVICE_ATTR(occ_master, 0444, occ_show_status, NULL, 0);
> +static SENSOR_DEVICE_ATTR(occ_active, 0444, occ_show_status, NULL, 1);
> +static SENSOR_DEVICE_ATTR(occ_dvfs_ot, 0444, occ_show_status, NULL, 2);
> +static SENSOR_DEVICE_ATTR(occ_dvfs_power, 0444, occ_show_status, NULL, 3);
> +static SENSOR_DEVICE_ATTR(occ_mem_throttle, 0444, occ_show_status, NULL, 4);
> +static SENSOR_DEVICE_ATTR(occ_quick_drop, 0444, occ_show_status, NULL, 5);
> +static SENSOR_DEVICE_ATTR(occ_status, 0444, occ_show_status, NULL, 6);
> +static SENSOR_DEVICE_ATTR(occs_present, 0444, occ_show_status, NULL, 7);
> +
> +static struct attribute *occ_attributes[] = {
> + _dev_attr_occ_master.dev_attr.attr,
> + _dev_attr_occ_active.dev_attr.attr,
> + _dev_attr_occ_dvfs_ot.dev_attr.attr,
> + _dev_attr_occ_dvfs_power.dev_attr.attr,
> + _dev_attr_occ_mem_throttle.dev_attr.attr,
> + _dev_attr_occ_quick_drop.dev_attr.attr,
> + _dev_attr_occ_status.dev_attr.attr,
> + _dev_attr_occs_present.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group occ_attr_group = {
> + .attrs = occ_attributes,
> +};
> +
>  /* only need to do this once at startup, as OCC won't change sensors on us */
>  static void occ_parse_poll_response(struct occ *occ)
>  {
> @@ -1188,5 +1271,15 @@ int occ_setup(struct occ *occ, const char *name)
>   return rc;
>   }
>  
> + rc = sysfs_create_group(>bus_dev->kobj, _attr_group);
> + if (rc)
> + dev_warn(occ->bus_dev, "failed to create status attrs: %d\n",
> +  rc);
> +
>   return 0;
>  }
> +
> +void occ_shutdown(struct occ *occ)
> +{
> + sysfs_remove_group(>bus_dev->kobj, _attr_group);
> +}
> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> index 049c3b4..dc9e06d 100644
> --- a/drivers/hwmon/occ/common.h
> +++ b/drivers/hwmon/occ/common.h
> @@ -110,5 +110,6 

Re: [v3,10/12] hwmon (occ): Add non-hwmon attributes

[Guenter Roeck]

On Mon, Nov 20, 2017 at 05:53:39PM -0600, eaja...@linux.vnet.ibm.com wrote:
> From: "Edward A. James" 
> 
> Create device attributes for additional OCC properties that do not
> belong as hwmon sensors. These provide additional information as to the
> state of the processor and system.
> 
... and those attributes should really not be part of the hwmon driver.

> Signed-off-by: Edward A. James 
> ---
>  drivers/hwmon/occ/common.c | 93 
> ++
>  drivers/hwmon/occ/common.h |  1 +
>  drivers/hwmon/occ/p8_i2c.c | 10 +
>  drivers/hwmon/occ/p9_sbe.c |  2 +
>  4 files changed, 106 insertions(+)
> 
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 337f286b..53e3592 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -24,6 +24,14 @@
>  
>  #define OCC_FRU_TYPE_VRM 0x3
>  
> +/* OCC status bits */
> +#define OCC_STAT_MASTER  0x80
> +#define OCC_STAT_ACTIVE  0x01
> +#define OCC_EXT_STAT_DVFS_OT 0x80
> +#define OCC_EXT_STAT_DVFS_POWER  0x40
> +#define OCC_EXT_STAT_MEM_THROTTLE0x20
> +#define OCC_EXT_STAT_QUICK_DROP  0x10
> +
>  /* OCC sensor type and version definitions */
>  
>  struct temp_sensor_1 {
> @@ -1108,6 +1116,81 @@ static int occ_setup_sensor_attrs(struct occ *occ)
>   return 0;
>  }
>  
> +static ssize_t occ_show_status(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + int rc;
> + int val = 0;
> + struct occ *occ = dev_get_drvdata(dev);
> + struct occ_poll_response_header *header;
> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +
> + rc = occ_update_response(occ);
> + if (rc)
> + return rc;
> +
> + header = (struct occ_poll_response_header *)occ->resp.data;
> +
> + switch (sattr->index) {
> + case 0:
> + val = (header->status & OCC_STAT_MASTER) ? 1 : 0;
> + break;
> + case 1:
> + val = (header->status & OCC_STAT_ACTIVE) ? 1 : 0;
> + break;
> + case 2:
> + val = (header->ext_status & OCC_EXT_STAT_DVFS_OT) ? 1 : 0;
> + break;
> + case 3:
> + val = (header->ext_status & OCC_EXT_STAT_DVFS_POWER) ? 1 : 0;
> + break;
> + case 4:
> + val = (header->ext_status & OCC_EXT_STAT_MEM_THROTTLE) ? 1 : 0;
> + break;
> + case 5:
> + val = (header->ext_status & OCC_EXT_STAT_QUICK_DROP) ? 1 : 0;
> + break;
> + case 6:
> + val = header->occ_state;
> + break;
> + case 7:
> + if (header->status & OCC_STAT_MASTER)
> + val = hweight8(header->occs_present);
> + else
> + val = 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +static SENSOR_DEVICE_ATTR(occ_master, 0444, occ_show_status, NULL, 0);
> +static SENSOR_DEVICE_ATTR(occ_active, 0444, occ_show_status, NULL, 1);
> +static SENSOR_DEVICE_ATTR(occ_dvfs_ot, 0444, occ_show_status, NULL, 2);
> +static SENSOR_DEVICE_ATTR(occ_dvfs_power, 0444, occ_show_status, NULL, 3);
> +static SENSOR_DEVICE_ATTR(occ_mem_throttle, 0444, occ_show_status, NULL, 4);
> +static SENSOR_DEVICE_ATTR(occ_quick_drop, 0444, occ_show_status, NULL, 5);
> +static SENSOR_DEVICE_ATTR(occ_status, 0444, occ_show_status, NULL, 6);
> +static SENSOR_DEVICE_ATTR(occs_present, 0444, occ_show_status, NULL, 7);
> +
> +static struct attribute *occ_attributes[] = {
> + _dev_attr_occ_master.dev_attr.attr,
> + _dev_attr_occ_active.dev_attr.attr,
> + _dev_attr_occ_dvfs_ot.dev_attr.attr,
> + _dev_attr_occ_dvfs_power.dev_attr.attr,
> + _dev_attr_occ_mem_throttle.dev_attr.attr,
> + _dev_attr_occ_quick_drop.dev_attr.attr,
> + _dev_attr_occ_status.dev_attr.attr,
> + _dev_attr_occs_present.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group occ_attr_group = {
> + .attrs = occ_attributes,
> +};
> +
>  /* only need to do this once at startup, as OCC won't change sensors on us */
>  static void occ_parse_poll_response(struct occ *occ)
>  {
> @@ -1188,5 +1271,15 @@ int occ_setup(struct occ *occ, const char *name)
>   return rc;
>   }
>  
> + rc = sysfs_create_group(>bus_dev->kobj, _attr_group);
> + if (rc)
> + dev_warn(occ->bus_dev, "failed to create status attrs: %d\n",
> +  rc);
> +
>   return 0;
>  }
> +
> +void occ_shutdown(struct occ *occ)
> +{
> + sysfs_remove_group(>bus_dev->kobj, _attr_group);
> +}
> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> index 049c3b4..dc9e06d 100644
> --- a/drivers/hwmon/occ/common.h
> +++ b/drivers/hwmon/occ/common.h
> @@ -110,5 +110,6 @@ struct occ {
>  };
>  
>  int