[PATCH 0/2] hwmon: Add PEC attribute support to hardware monitoring core

[PATCH 0/2] hwmon: Add PEC attribute support to hardware monitoring core

[Guenter Roeck]

Several hardware monitoring chips optionally support Packet Error Checking
(PEC). For some chips, PEC support can be enabled simply by setting
I2C_CLIENT_PEC in the i2c client data structure. Others require chip
specific code to enable or disable PEC support.

Introduce hwmon_chip_pec and HWMON_C_PEC to simplify adding configurable
PEC support for hardware monitoring drivers. A driver can set HWMON_C_PEC
in its chip information data to indicate PEC support. If a chip requires
chip specific code to enable or disable PEC support, the driver only needs
to implement support for the hwmon_chip_pec attribute to its write
function.

The hardware monitoring core does not depend on the I2C subsystem after
this change. However, the I2C subsystem needs to be reachable. This
requires a new HWMON dependency to ensure that HWMON can only be built
as module if I2C is built as module. This should not make a practical
difference.

The first patch of the series introduces PEC support to the harwdare
monitoring core. The second patch converts to lm90 driver to use the
new infrastructure.

Tested with ADM7421A using lm90 driver and Devantech USB-ISS.

---

[PATCH 1/2] hwmon: Add PEC attribute support to hardware monitoring core

[Guenter Roeck]

Several hardware monitoring chips optionally support Packet Error Checking
(PEC). For some chips, PEC support can be enabled simply by setting
I2C_CLIENT_PEC in the i2c client data structure. Others require chip
specific code to enable or disable PEC support.

Introduce hwmon_chip_pec and HWMON_C_PEC to simplify adding configurable
PEC support for hardware monitoring drivers. A driver can set HWMON_C_PEC
in its chip information data to indicate PEC support. If a chip requires
chip specific code to enable or disable PEC support, the driver only needs
to implement support for the hwmon_chip_pec attribute to its write
function.

The hardware monitoring core does not depend on the I2C subsystem after
this change. However, the I2C subsystem needs to be reachable. This
requires a new HWMON dependency to ensure that HWMON can only be built
as module if I2C is built as module. This should not make a practical
difference.

Cc: Radu Sabau <radu.sabau@analog.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/Kconfig |   1 +
 drivers/hwmon/hwmon.c | 136 +++++++++++++++++++++++++++++++++++++-----
 include/linux/hwmon.h |   2 +
 3 files changed, 123 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..7f384a2494c9 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -6,6 +6,7 @@
 menuconfig HWMON
 	tristate "Hardware Monitoring support"
 	depends on HAS_IOMEM
+	depends on I2C || I2C=n
 	default y
 	help
 	  Hardware monitoring devices let you monitor the hardware health
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 3b259c425ab7..1fdea8b1ec91 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/hwmon.h>
+#include <linux/i2c.h>
 #include <linux/idr.h>
 #include <linux/kstrtox.h>
 #include <linux/list.h>
@@ -309,6 +310,103 @@ static int hwmon_attr_base(enum hwmon_sensor_types type)
 	return 1;
 }
 
+/*
+ * PEC support
+ *
+ * The 'pec' attribute is attached to I2C client devices. It is only provided
+ * if the i2c controller supports PEC.
+ *
+ * The mutex ensures that PEC configuration between i2c device and the hardware
+ * is consistent. Use a single mutex because attribute writes are supposed to be
+ * rare, and maintaining a separate mutex for each hardware monitoring device
+ * would add substantial complexity to the driver for little if any gain.
+ *
+ * The hardware monitoring device is identified as child of the i2c client
+ * device. This assumes that only a single hardware monitoring device is
+ * attached to an i2c client device.
+ */
+
+static DEFINE_MUTEX(hwmon_pec_mutex);
+
+static int hwmon_match_device(struct device *dev, void *data)
+{
+	return dev->class == &hwmon_class;
+}
+
+static ssize_t pec_show(struct device *dev, struct device_attribute *dummy,
+			char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
+}
+
+static ssize_t pec_store(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct hwmon_device *hwdev;
+	struct device *hdev;
+	bool val;
+	int err;
+
+	err = kstrtobool(buf, &val);
+	if (err < 0)
+		return err;
+
+	hdev = device_find_child(dev, NULL, hwmon_match_device);
+	if (!hdev)
+		return -ENODEV;
+
+	mutex_lock(&hwmon_pec_mutex);
+
+	/*
+	 * If there is no write function, we assume that chip specific
+	 * handling is not required.
+	 */
+	hwdev = to_hwmon_device(hdev);
+	if (hwdev->chip->ops->write) {
+		err = hwdev->chip->ops->write(hdev, hwmon_chip, hwmon_chip_pec, 0, val);
+		if (err && err != -EOPNOTSUPP)
+			goto unlock;
+	}
+
+	if (!val)
+		client->flags &= ~I2C_CLIENT_PEC;
+	else
+		client->flags |= I2C_CLIENT_PEC;
+
+	err = count;
+unlock:
+	mutex_unlock(&hwmon_pec_mutex);
+	put_device(hdev);
+
+	return err;
+}
+
+static DEVICE_ATTR_RW(pec);
+
+static void hwmon_remove_pec(void *dev)
+{
+	device_remove_file(dev, &dev_attr_pec);
+}
+
+static int hwmon_pec_register(struct device *hdev)
+{
+	struct i2c_client *client = i2c_verify_client(hdev->parent);
+	int err;
+
+	if (!client ||
+	    !i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_PEC))
+		return 0;
+
+	err = device_create_file(&client->dev, &dev_attr_pec);
+	if (err)
+		return err;
+
+	return devm_add_action_or_reset(hdev, hwmon_remove_pec, &client->dev);
+}
+
 /* sysfs attribute management */
 
 static ssize_t hwmon_attr_show(struct device *dev,
@@ -397,10 +495,6 @@ static struct attribute *hwmon_genattr(const void *drvdata,
 	const char *name;
 	bool is_string = is_string_attr(type, attr);
 
-	/* The attribute is invisible if there is no template string */
-	if (!template)
-		return ERR_PTR(-ENOENT);
-
 	mode = ops->is_visible(drvdata, type, attr, index);
 	if (!mode)
 		return ERR_PTR(-ENOENT);
@@ -712,8 +806,8 @@ static int hwmon_genattrs(const void *drvdata,
 
 			attr = __ffs(attr_mask);
 			attr_mask &= ~BIT(attr);
-			if (attr >= template_size)
-				return -EINVAL;
+			if (attr >= template_size || !templates[attr])
+				continue;	/* attribute is invisible */
 			a = hwmon_genattr(drvdata, info->type, attr, i,
 					  templates[attr], ops);
 			if (IS_ERR(a)) {
@@ -849,16 +943,26 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 	INIT_LIST_HEAD(&hwdev->tzdata);
 
 	if (hdev->of_node && chip && chip->ops->read &&
-	    chip->info[0]->type == hwmon_chip &&
-	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
-		err = hwmon_thermal_register_sensors(hdev);
-		if (err) {
-			device_unregister(hdev);
-			/*
-			 * Don't worry about hwdev; hwmon_dev_release(), called
-			 * from device_unregister(), will free it.
-			 */
-			goto ida_remove;
+	    chip->info[0]->type == hwmon_chip) {
+		u32 config = chip->info[0]->config[0];
+
+		if (config & HWMON_C_REGISTER_TZ) {
+			err = hwmon_thermal_register_sensors(hdev);
+			if (err) {
+				device_unregister(hdev);
+				/*
+				 * Don't worry about hwdev; hwmon_dev_release(),
+				 * called from device_unregister(), will free it.
+				 */
+				goto ida_remove;
+			}
+		}
+		if (config & HWMON_C_PEC) {
+			err = hwmon_pec_register(hdev);
+			if (err) {
+				device_unregister(hdev);
+				goto ida_remove;
+			}
 		}
 	}
 
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index edf96f249eb5..e94314760aab 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -45,6 +45,7 @@ enum hwmon_chip_attributes {
 	hwmon_chip_power_samples,
 	hwmon_chip_temp_samples,
 	hwmon_chip_beep_enable,
+	hwmon_chip_pec,
 };
 
 #define HWMON_C_TEMP_RESET_HISTORY	BIT(hwmon_chip_temp_reset_history)
@@ -60,6 +61,7 @@ enum hwmon_chip_attributes {
 #define HWMON_C_POWER_SAMPLES		BIT(hwmon_chip_power_samples)
 #define HWMON_C_TEMP_SAMPLES		BIT(hwmon_chip_temp_samples)
 #define HWMON_C_BEEP_ENABLE		BIT(hwmon_chip_beep_enable)
+#define HWMON_C_PEC			BIT(hwmon_chip_pec)
 
 enum hwmon_temp_attributes {
 	hwmon_temp_enable,
-- 
2.39.2

[PATCH 2/2] hwmon: (lm90) Convert to use PEC support from hwmon core

[Guenter Roeck]

Replace driver specific PEC handling code with hardware monitoring core
functionality.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 56 ++------------------------------------------
 1 file changed, 2 insertions(+), 54 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index e0d7454a301c..40d9e21b528c 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1270,42 +1270,6 @@ static int lm90_update_device(struct device *dev)
 	return 0;
 }
 
-/* pec used for devices with PEC support */
-static ssize_t pec_show(struct device *dev, struct device_attribute *dummy,
-			char *buf)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
-}
-
-static ssize_t pec_store(struct device *dev, struct device_attribute *dummy,
-			 const char *buf, size_t count)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
-
-	switch (val) {
-	case 0:
-		client->flags &= ~I2C_CLIENT_PEC;
-		break;
-	case 1:
-		client->flags |= I2C_CLIENT_PEC;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return count;
-}
-
-static DEVICE_ATTR_RW(pec);
-
 static int lm90_temp_get_resolution(struct lm90_data *data, int index)
 {
 	switch (index) {
@@ -2659,11 +2623,6 @@ static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
 		return IRQ_NONE;
 }
 
-static void lm90_remove_pec(void *dev)
-{
-	device_remove_file(dev, &dev_attr_pec);
-}
-
 static int lm90_probe_channel_from_dt(struct i2c_client *client,
 				      struct device_node *child,
 				      struct lm90_data *data)
@@ -2802,6 +2761,8 @@ static int lm90_probe(struct i2c_client *client)
 		data->chip_config[0] |= HWMON_C_UPDATE_INTERVAL;
 	if (data->flags & LM90_HAVE_FAULTQUEUE)
 		data->chip_config[0] |= HWMON_C_TEMP_SAMPLES;
+	if (data->flags & (LM90_HAVE_PEC | LM90_HAVE_PARTIAL_PEC))
+		data->chip_config[0] |= HWMON_C_PEC;
 	data->info[1] = &data->temp_info;
 
 	info = &data->temp_info;
@@ -2878,19 +2839,6 @@ static int lm90_probe(struct i2c_client *client)
 		return err;
 	}
 
-	/*
-	 * The 'pec' attribute is attached to the i2c device and thus created
-	 * separately.
-	 */
-	if (data->flags & (LM90_HAVE_PEC | LM90_HAVE_PARTIAL_PEC)) {
-		err = device_create_file(dev, &dev_attr_pec);
-		if (err)
-			return err;
-		err = devm_add_action_or_reset(dev, lm90_remove_pec, dev);
-		if (err)
-			return err;
-	}
-
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
 							 data, &data->chip,
 							 NULL);
-- 
2.39.2

Re: [PATCH 1/2] hwmon: Add PEC attribute support to hardware monitoring core

[Nuno Sá]

On Wed, 2024-05-29 at 11:01 -0700, Guenter Roeck wrote:
> Several hardware monitoring chips optionally support Packet Error Checking
> (PEC). For some chips, PEC support can be enabled simply by setting
> I2C_CLIENT_PEC in the i2c client data structure. Others require chip
> specific code to enable or disable PEC support.
> 
> Introduce hwmon_chip_pec and HWMON_C_PEC to simplify adding configurable
> PEC support for hardware monitoring drivers. A driver can set HWMON_C_PEC
> in its chip information data to indicate PEC support. If a chip requires
> chip specific code to enable or disable PEC support, the driver only needs
> to implement support for the hwmon_chip_pec attribute to its write
> function.
> 
> The hardware monitoring core does not depend on the I2C subsystem after
> this change. However, the I2C subsystem needs to be reachable. This
> requires a new HWMON dependency to ensure that HWMON can only be built
> as module if I2C is built as module. This should not make a practical
> difference.
> 
> Cc: Radu Sabau <radu.sabau@analog.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/Kconfig |   1 +
>  drivers/hwmon/hwmon.c | 136 +++++++++++++++++++++++++++++++++++++-----
>  include/linux/hwmon.h |   2 +
>  3 files changed, 123 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e14ae18a973b..7f384a2494c9 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -6,6 +6,7 @@
>  menuconfig HWMON
>  	tristate "Hardware Monitoring support"
>  	depends on HAS_IOMEM
> +	depends on I2C || I2C=n
>  	default y
>  	help
>  	  Hardware monitoring devices let you monitor the hardware health
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 3b259c425ab7..1fdea8b1ec91 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -14,6 +14,7 @@
>  #include <linux/err.h>
>  #include <linux/gfp.h>
>  #include <linux/hwmon.h>
> +#include <linux/i2c.h>
>  #include <linux/idr.h>
>  #include <linux/kstrtox.h>
>  #include <linux/list.h>
> @@ -309,6 +310,103 @@ static int hwmon_attr_base(enum hwmon_sensor_types type)
>  	return 1;
>  }
>  
> +/*
> + * PEC support
> + *
> + * The 'pec' attribute is attached to I2C client devices. It is only provided
> + * if the i2c controller supports PEC.
> + *
> + * The mutex ensures that PEC configuration between i2c device and the hardware
> + * is consistent. Use a single mutex because attribute writes are supposed to be
> + * rare, and maintaining a separate mutex for each hardware monitoring device
> + * would add substantial complexity to the driver for little if any gain.
> + *
> + * The hardware monitoring device is identified as child of the i2c client
> + * device. This assumes that only a single hardware monitoring device is
> + * attached to an i2c client device.
> + */
> +
> +static DEFINE_MUTEX(hwmon_pec_mutex);
> +
> +static int hwmon_match_device(struct device *dev, void *data)
> +{
> +	return dev->class == &hwmon_class;
> +}
> +
> +static ssize_t pec_show(struct device *dev, struct device_attribute *dummy,
> +			char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));

sysfs_emit()?


with the above,

Acked-by: Nuno Sa <nuno.sa@analog.com>

Re: [PATCH 2/2] hwmon: (lm90) Convert to use PEC support from hwmon core

[Nuno Sá]

On Wed, 2024-05-29 at 11:01 -0700, Guenter Roeck wrote:
> Replace driver specific PEC handling code with hardware monitoring core
> functionality.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

Acked-by: Nuno Sa <nuno.sa@analog.com>

Re: [PATCH 1/2] hwmon: Add PEC attribute support to hardware monitoring core

[Guenter Roeck]

On 5/29/24 23:37, Nuno Sá wrote:
[ ... ]
>> +static ssize_t pec_show(struct device *dev, struct device_attribute *dummy,
>> +			char *buf)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +
>> +	return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
> 
> sysfs_emit()?
> 
Done.
> 
> with the above,
> 
> Acked-by: Nuno Sa <nuno.sa@analog.com>
> 
Thanks!

Guenter

Re: [PATCH 2/2] hwmon: (lm90) Convert to use PEC support from hwmon core

[Guenter Roeck]

On 5/29/24 23:38, Nuno Sá wrote:
> On Wed, 2024-05-29 at 11:01 -0700, Guenter Roeck wrote:
>> Replace driver specific PEC handling code with hardware monitoring core
>> functionality.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
> 
> Acked-by: Nuno Sa <nuno.sa@analog.com>
> 

Thanks!

Guenter