[PATCH v2 0/8] Refactor ADT7X10

[PATCH v2 0/8] Refactor ADT7X10

[Cosmin Tanislav]

V1 -> V2:
 * add device managed action for restoring config
 * merge multiple small related patches into a single patch
   that converts the driver to use devm_hwmon_device_register_with_info
 * switch to devm_request_threaded_irq after switching to
   devm_hwmon_device_register_with_info to make sure that it is impossible
   for the interrupt handler to access the freed hwmon device
 * drop core driver remove callback

Cosmin Tanislav (8):
  hwmon: (adt7410) Pass name to core driver
  hwmon: (adt7x10) Store bus_dev in private data
  hwmon: (adt7x10) Add device managed action for restoring config
  hwmon: (adt7x10) Use devm_hwmon_device_register_with_info
  hwmon: (adt7x10) Use devm_request_threaded_irq
  hwmon: (adt7x10) Remove empty driver removal callback
  hwmon: (adt7x10) Pass hwinfo dev to irq handler
  hwmon: (adt7x10) Use hwmon_notify_event

 drivers/hwmon/adt7310.c |   7 -
 drivers/hwmon/adt7410.c |   9 +-
 drivers/hwmon/adt7x10.c | 281 ++++++++++++++++++++--------------------
 drivers/hwmon/adt7x10.h |   1 -
 4 files changed, 144 insertions(+), 154 deletions(-)

-- 
2.34.1

[PATCH v2 1/8] hwmon: (adt7410) Pass name to core driver

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

It will later be used to register hwmon device using
hwmon_device_register_with_info.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7410.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7410.c b/drivers/hwmon/adt7410.c
index 973db057427b..ef4b5af865e9 100644
--- a/drivers/hwmon/adt7410.c
+++ b/drivers/hwmon/adt7410.c
@@ -45,7 +45,7 @@ static int adt7410_i2c_probe(struct i2c_client *client)
 			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
 
-	return adt7x10_probe(&client->dev, NULL, client->irq, &adt7410_i2c_ops);
+	return adt7x10_probe(&client->dev, client->name, client->irq, &adt7410_i2c_ops);
 }
 
 static int adt7410_i2c_remove(struct i2c_client *client)
-- 
2.34.1

Re: [PATCH v2 1/8] hwmon: (adt7410) Pass name to core driver

[Guenter Roeck]

On 12/21/21 1:04 PM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> It will later be used to register hwmon device using
> hwmon_device_register_with_info.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>   drivers/hwmon/adt7410.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/adt7410.c b/drivers/hwmon/adt7410.c
> index 973db057427b..ef4b5af865e9 100644
> --- a/drivers/hwmon/adt7410.c
> +++ b/drivers/hwmon/adt7410.c
> @@ -45,7 +45,7 @@ static int adt7410_i2c_probe(struct i2c_client *client)
>   			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
>   		return -ENODEV;
>   
> -	return adt7x10_probe(&client->dev, NULL, client->irq, &adt7410_i2c_ops);
> +	return adt7x10_probe(&client->dev, client->name, client->irq, &adt7410_i2c_ops);
>   }
>   
>   static int adt7410_i2c_remove(struct i2c_client *client)
> 

Did you test this patch on its own ? It should result in a duplicate
attribute error from sysfs if an i2c device tries to register.

Again, that is unacceptable. The patches have to apply on their own
and must not cause any problems.

On a side note, it seems that your e-mails do not have a To: address.

To:     unlisted-recipients:; (no To-header on input)

This results in an error when trying to reply to all.

Guenter

[PATCH v2 2/8] hwmon: (adt7x10) Store bus_dev in private data

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

It will later be used to access the bus device because
callbacks only provide the hwmon device.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index e9d33aa78a19..2439da9b64e6 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -56,6 +56,7 @@ struct adt7x10_data {
 	const struct adt7x10_ops *ops;
 	const char		*name;
 	struct device		*hwmon_dev;
+	struct device		*bus_dev;
 	struct mutex		update_lock;
 	u8			config;
 	u8			oldconfig;
@@ -368,6 +369,7 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 
 	data->ops = ops;
 	data->name = name;
+	data->bus_dev = dev;
 
 	dev_set_drvdata(dev, data);
 	mutex_init(&data->update_lock);
-- 
2.34.1

[PATCH v2 3/8] hwmon: (adt7x10) Add device managed action for restoring config

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

To simplify the core driver remove function.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 2439da9b64e6..e8a6c541a590 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -357,6 +357,17 @@ static const struct attribute_group adt7x10_group = {
 	.attrs = adt7x10_attributes,
 };
 
+static void adt7x10_restore_config(void *private)
+{
+	struct adt7x10_data *data = private;
+	struct device *dev = data->bus_dev;
+
+	if (data->oldconfig == data->config)
+		return;
+
+	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
+}
+
 int adt7x10_probe(struct device *dev, const char *name, int irq,
 		  const struct adt7x10_ops *ops)
 {
@@ -397,14 +408,18 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	}
 	dev_dbg(dev, "Config %02x\n", data->config);
 
+	ret = devm_add_action_or_reset(dev, adt7x10_restore_config, data);
+	if (ret)
+		return ret;
+
 	ret = adt7x10_fill_cache(dev);
 	if (ret)
-		goto exit_restore;
+		return ret;
 
 	/* Register sysfs hooks */
 	ret = sysfs_create_group(&dev->kobj, &adt7x10_group);
 	if (ret)
-		goto exit_restore;
+		return ret;
 
 	/*
 	 * The I2C device will already have it's own 'name' attribute, but for
@@ -440,8 +455,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 		device_remove_file(dev, &dev_attr_name);
 exit_remove:
 	sysfs_remove_group(&dev->kobj, &adt7x10_group);
-exit_restore:
-	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(adt7x10_probe);
@@ -457,8 +470,6 @@ void adt7x10_remove(struct device *dev, int irq)
 	if (data->name)
 		device_remove_file(dev, &dev_attr_name);
 	sysfs_remove_group(&dev->kobj, &adt7x10_group);
-	if (data->oldconfig != data->config)
-		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
 }
 EXPORT_SYMBOL_GPL(adt7x10_remove);
 
-- 
2.34.1

[PATCH v2 4/8] hwmon: (adt7x10) Use devm_hwmon_device_register_with_info

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

Describe the only available channel, implement read, write
and is_visible callbacks.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 233 ++++++++++++++++++++--------------------
 1 file changed, 114 insertions(+), 119 deletions(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index e8a6c541a590..2e6e54bf1d15 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -54,8 +54,6 @@
 /* Each client has this additional data */
 struct adt7x10_data {
 	const struct adt7x10_ops *ops;
-	const char		*name;
-	struct device		*hwmon_dev;
 	struct device		*bus_dev;
 	struct mutex		update_lock;
 	u8			config;
@@ -218,14 +216,11 @@ static int ADT7X10_REG_TO_TEMP(struct adt7x10_data *data, s16 reg)
 
 /* sysfs attributes for hwmon */
 
-static ssize_t adt7x10_temp_show(struct device *dev,
-				 struct device_attribute *da, char *buf)
+static int adt7x10_temp_read(struct adt7x10_data *data, int index, long *val)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-
+	struct device *dev = data->bus_dev;
 
-	if (attr->index == 0) {
+	if (index == 0) {
 		int ret;
 
 		ret = adt7x10_update_temp(dev);
@@ -233,39 +228,28 @@ static ssize_t adt7x10_temp_show(struct device *dev,
 			return ret;
 	}
 
-	return sprintf(buf, "%d\n", ADT7X10_REG_TO_TEMP(data,
-		       data->temp[attr->index]));
+	*val = ADT7X10_REG_TO_TEMP(data, data->temp[index]);
+
+	return 0;
 }
 
-static ssize_t adt7x10_temp_store(struct device *dev,
-				  struct device_attribute *da,
-				  const char *buf, size_t count)
+static int adt7x10_temp_write(struct adt7x10_data *data, unsigned int nr,
+			      long temp)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int nr = attr->index;
-	long temp;
+	struct device *dev = data->bus_dev;
 	int ret;
 
-	ret = kstrtol(buf, 10, &temp);
-	if (ret)
-		return ret;
-
 	mutex_lock(&data->update_lock);
 	data->temp[nr] = ADT7X10_TEMP_TO_REG(temp);
 	ret = adt7x10_write_word(dev, ADT7X10_REG_TEMP[nr], data->temp[nr]);
-	if (ret)
-		count = ret;
 	mutex_unlock(&data->update_lock);
-	return count;
+
+	return ret;
 }
 
-static ssize_t adt7x10_t_hyst_show(struct device *dev,
-				   struct device_attribute *da, char *buf)
+static int adt7x10_hyst_read(struct adt7x10_data *data, unsigned int nr,
+			     long *val)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int nr = attr->index;
 	int hyst;
 
 	hyst = (data->hyst & ADT7X10_T_HYST_MASK) * 1000;
@@ -276,85 +260,127 @@ static ssize_t adt7x10_t_hyst_show(struct device *dev,
 	 */
 	if (nr == 2)	/* min has positive offset, others have negative */
 		hyst = -hyst;
-	return sprintf(buf, "%d\n",
-		       ADT7X10_REG_TO_TEMP(data, data->temp[nr]) - hyst);
+
+	*val = ADT7X10_REG_TO_TEMP(data, data->temp[nr]) - hyst;
+
+	return 0;
 }
 
-static ssize_t adt7x10_t_hyst_store(struct device *dev,
-				    struct device_attribute *da,
-				    const char *buf, size_t count)
+static int adt7x10_hyst_write(struct adt7x10_data *data, long hyst)
 {
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int limit, ret;
-	long hyst;
+	struct device *dev = data->bus_dev;
+	int limit;
 
-	ret = kstrtol(buf, 10, &hyst);
-	if (ret)
-		return ret;
 	/* convert absolute hysteresis value to a 4 bit delta value */
 	limit = ADT7X10_REG_TO_TEMP(data, data->temp[1]);
 	hyst = clamp_val(hyst, ADT7X10_TEMP_MIN, ADT7X10_TEMP_MAX);
 	data->hyst = clamp_val(DIV_ROUND_CLOSEST(limit - hyst, 1000),
-				   0, ADT7X10_T_HYST_MASK);
-	ret = adt7x10_write_byte(dev, ADT7X10_T_HYST, data->hyst);
-	if (ret)
-		return ret;
-
-	return count;
+			       0, ADT7X10_T_HYST_MASK);
+	return adt7x10_write_byte(dev, ADT7X10_T_HYST, data->hyst);
 }
 
-static ssize_t adt7x10_alarm_show(struct device *dev,
-				  struct device_attribute *da, char *buf)
+static int adt7x10_alarm_read(struct adt7x10_data *data, unsigned int index,
+			      long *val)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct device *dev = data->bus_dev;
 	int ret;
 
 	ret = adt7x10_read_byte(dev, ADT7X10_STATUS);
 	if (ret < 0)
 		return ret;
 
-	return sprintf(buf, "%d\n", !!(ret & attr->index));
+	*val = !!(ret & index);
+
+	return 0;
+}
+
+static umode_t adt7x10_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	umode_t mode = 0444;
+
+	switch (attr) {
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_max_hyst:
+		mode |= 0200;
+		break;
+	default:
+		break;
+	}
+
+	return mode;
+}
+
+static int adt7x10_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct adt7x10_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_temp_input:
+		return adt7x10_temp_read(data, 0, val);
+	case hwmon_temp_max:
+		return adt7x10_temp_read(data, 1, val);
+	case hwmon_temp_min:
+		return adt7x10_temp_read(data, 2, val);
+	case hwmon_temp_crit:
+		return adt7x10_temp_read(data, 3, val);
+	case hwmon_temp_max_hyst:
+		return adt7x10_hyst_read(data, 1, val);
+	case hwmon_temp_min_hyst:
+		return adt7x10_hyst_read(data, 2, val);
+	case hwmon_temp_crit_hyst:
+		return adt7x10_hyst_read(data, 3, val);
+	case hwmon_temp_min_alarm:
+		return adt7x10_alarm_read(data, ADT7X10_STAT_T_LOW, val);
+	case hwmon_temp_max_alarm:
+		return adt7x10_alarm_read(data, ADT7X10_STAT_T_HIGH, val);
+	case hwmon_temp_crit_alarm:
+		return adt7x10_alarm_read(data, ADT7X10_STAT_T_CRIT, val);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
-static ssize_t name_show(struct device *dev, struct device_attribute *da,
-			 char *buf)
+static int adt7x10_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
 {
 	struct adt7x10_data *data = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%s\n", data->name);
+	switch (attr) {
+	case hwmon_temp_max:
+		return adt7x10_temp_write(data, 1, val);
+	case hwmon_temp_min:
+		return adt7x10_temp_write(data, 2, val);
+	case hwmon_temp_crit:
+		return adt7x10_temp_write(data, 3, val);
+	case hwmon_temp_max_hyst:
+		return adt7x10_hyst_write(data, val);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
-static SENSOR_DEVICE_ATTR_RO(temp1_input, adt7x10_temp, 0);
-static SENSOR_DEVICE_ATTR_RW(temp1_max, adt7x10_temp, 1);
-static SENSOR_DEVICE_ATTR_RW(temp1_min, adt7x10_temp, 2);
-static SENSOR_DEVICE_ATTR_RW(temp1_crit, adt7x10_temp, 3);
-static SENSOR_DEVICE_ATTR_RW(temp1_max_hyst, adt7x10_t_hyst, 1);
-static SENSOR_DEVICE_ATTR_RO(temp1_min_hyst, adt7x10_t_hyst, 2);
-static SENSOR_DEVICE_ATTR_RO(temp1_crit_hyst, adt7x10_t_hyst, 3);
-static SENSOR_DEVICE_ATTR_RO(temp1_min_alarm, adt7x10_alarm,
-			     ADT7X10_STAT_T_LOW);
-static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, adt7x10_alarm,
-			     ADT7X10_STAT_T_HIGH);
-static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, adt7x10_alarm,
-			     ADT7X10_STAT_T_CRIT);
-static DEVICE_ATTR_RO(name);
-
-static struct attribute *adt7x10_attributes[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_min.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_min_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
-	NULL
+static const struct hwmon_channel_info *adt7x10_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MIN |
+			   HWMON_T_CRIT | HWMON_T_MAX_HYST | HWMON_T_MIN_HYST |
+			   HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM),
+	NULL,
+};
+
+static const struct hwmon_ops adt7x10_hwmon_ops = {
+	.is_visible = adt7x10_is_visible,
+	.read = adt7x10_read,
+	.write = adt7x10_write,
 };
 
-static const struct attribute_group adt7x10_group = {
-	.attrs = adt7x10_attributes,
+static const struct hwmon_chip_info adt7x10_chip_info = {
+	.ops = &adt7x10_hwmon_ops,
+	.info = adt7x10_info,
 };
 
 static void adt7x10_restore_config(void *private)
@@ -372,6 +398,7 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 		  const struct adt7x10_ops *ops)
 {
 	struct adt7x10_data *data;
+	struct device *hdev;
 	int ret;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
@@ -379,7 +406,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 		return -ENOMEM;
 
 	data->ops = ops;
-	data->name = name;
 	data->bus_dev = dev;
 
 	dev_set_drvdata(dev, data);
@@ -416,26 +442,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	if (ret)
 		return ret;
 
-	/* Register sysfs hooks */
-	ret = sysfs_create_group(&dev->kobj, &adt7x10_group);
-	if (ret)
+	hdev = devm_hwmon_device_register_with_info(dev, name, data,
+						    &adt7x10_chip_info, NULL);
+	if (IS_ERR(hdev)) {
+		ret = PTR_ERR(hdev);
 		return ret;
-
-	/*
-	 * The I2C device will already have it's own 'name' attribute, but for
-	 * the SPI device we need to register it. name will only be non NULL if
-	 * the device doesn't register the 'name' attribute on its own.
-	 */
-	if (name) {
-		ret = device_create_file(dev, &dev_attr_name);
-		if (ret)
-			goto exit_remove;
-	}
-
-	data->hwmon_dev = hwmon_device_register(dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		ret = PTR_ERR(data->hwmon_dev);
-		goto exit_remove_name;
 	}
 
 	if (irq > 0) {
@@ -443,33 +454,17 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				dev_name(dev), dev);
 		if (ret)
-			goto exit_hwmon_device_unregister;
+			return ret;
 	}
 
 	return 0;
-
-exit_hwmon_device_unregister:
-	hwmon_device_unregister(data->hwmon_dev);
-exit_remove_name:
-	if (name)
-		device_remove_file(dev, &dev_attr_name);
-exit_remove:
-	sysfs_remove_group(&dev->kobj, &adt7x10_group);
-	return ret;
 }
 EXPORT_SYMBOL_GPL(adt7x10_probe);
 
 void adt7x10_remove(struct device *dev, int irq)
 {
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-
 	if (irq > 0)
 		free_irq(irq, dev);
-
-	hwmon_device_unregister(data->hwmon_dev);
-	if (data->name)
-		device_remove_file(dev, &dev_attr_name);
-	sysfs_remove_group(&dev->kobj, &adt7x10_group);
 }
 EXPORT_SYMBOL_GPL(adt7x10_remove);
 
-- 
2.34.1

Re: [PATCH v2 4/8] hwmon: (adt7x10) Use devm_hwmon_device_register_with_info

[Guenter Roeck]

On 12/21/21 1:04 PM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Describe the only available channel, implement read, write
> and is_visible callbacks.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>   drivers/hwmon/adt7x10.c | 233 ++++++++++++++++++++--------------------
>   1 file changed, 114 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index e8a6c541a590..2e6e54bf1d15 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -54,8 +54,6 @@
>   /* Each client has this additional data */
>   struct adt7x10_data {
>   	const struct adt7x10_ops *ops;
> -	const char		*name;
> -	struct device		*hwmon_dev;
>   	struct device		*bus_dev;
>   	struct mutex		update_lock;
>   	u8			config;
> @@ -218,14 +216,11 @@ static int ADT7X10_REG_TO_TEMP(struct adt7x10_data *data, s16 reg)
>   
>   /* sysfs attributes for hwmon */
>   
> -static ssize_t adt7x10_temp_show(struct device *dev,
> -				 struct device_attribute *da, char *buf)
> +static int adt7x10_temp_read(struct adt7x10_data *data, int index, long *val)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> -	struct adt7x10_data *data = dev_get_drvdata(dev);
> -
> +	struct device *dev = data->bus_dev;
>   
> -	if (attr->index == 0) {
> +	if (index == 0) {

Using "index" here is just confusing and has little value. The calling code
can just assign values directly instead, ie

	case hwmon_input:
		ret = adt7x10_update_temp(dev);
		if (ret)
			return ret;
		*val = ADT7X10_REG_TO_TEMP(data, data->temp[0]);
		return 0;
	case hwmon_temp_max:
		*val = ADT7X10_REG_TO_TEMP(data, data->temp[1]);
		return 0;
	...


If you really want to keep using index values, please define constants.
Code such as "index == 0" or, below "nr == 2" is error prone.


>   		int ret;
>   
>   		ret = adt7x10_update_temp(dev);
> @@ -233,39 +228,28 @@ static ssize_t adt7x10_temp_show(struct device *dev,
>   			return ret;
>   	}
>   
> -	return sprintf(buf, "%d\n", ADT7X10_REG_TO_TEMP(data,
> -		       data->temp[attr->index]));
> +	*val = ADT7X10_REG_TO_TEMP(data, data->temp[index]);
> +
> +	return 0;
>   }
>   
> -static ssize_t adt7x10_temp_store(struct device *dev,
> -				  struct device_attribute *da,
> -				  const char *buf, size_t count)
> +static int adt7x10_temp_write(struct adt7x10_data *data, unsigned int nr,
> +			      long temp)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> -	struct adt7x10_data *data = dev_get_drvdata(dev);
> -	int nr = attr->index;
> -	long temp;
> +	struct device *dev = data->bus_dev;
>   	int ret;
>   
> -	ret = kstrtol(buf, 10, &temp);
> -	if (ret)
> -		return ret;
> -
>   	mutex_lock(&data->update_lock);
>   	data->temp[nr] = ADT7X10_TEMP_TO_REG(temp);
>   	ret = adt7x10_write_word(dev, ADT7X10_REG_TEMP[nr], data->temp[nr]);
> -	if (ret)
> -		count = ret;
>   	mutex_unlock(&data->update_lock);
> -	return count;
> +
> +	return ret;
>   }
>   
> -static ssize_t adt7x10_t_hyst_show(struct device *dev,
> -				   struct device_attribute *da, char *buf)
> +static int adt7x10_hyst_read(struct adt7x10_data *data, unsigned int nr,
> +			     long *val)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> -	struct adt7x10_data *data = dev_get_drvdata(dev);
> -	int nr = attr->index;
>   	int hyst;
>   
>   	hyst = (data->hyst & ADT7X10_T_HYST_MASK) * 1000;
> @@ -276,85 +260,127 @@ static ssize_t adt7x10_t_hyst_show(struct device *dev,
>   	 */
>   	if (nr == 2)	/* min has positive offset, others have negative */
>   		hyst = -hyst;
> -	return sprintf(buf, "%d\n",
> -		       ADT7X10_REG_TO_TEMP(data, data->temp[nr]) - hyst);
> +
> +	*val = ADT7X10_REG_TO_TEMP(data, data->temp[nr]) - hyst;
> +
> +	return 0;
>   }
>   
> -static ssize_t adt7x10_t_hyst_store(struct device *dev,
> -				    struct device_attribute *da,
> -				    const char *buf, size_t count)
> +static int adt7x10_hyst_write(struct adt7x10_data *data, long hyst)
>   {
> -	struct adt7x10_data *data = dev_get_drvdata(dev);
> -	int limit, ret;
> -	long hyst;
> +	struct device *dev = data->bus_dev;
> +	int limit;
>   
> -	ret = kstrtol(buf, 10, &hyst);
> -	if (ret)
> -		return ret;
>   	/* convert absolute hysteresis value to a 4 bit delta value */
>   	limit = ADT7X10_REG_TO_TEMP(data, data->temp[1]);
>   	hyst = clamp_val(hyst, ADT7X10_TEMP_MIN, ADT7X10_TEMP_MAX);
>   	data->hyst = clamp_val(DIV_ROUND_CLOSEST(limit - hyst, 1000),
> -				   0, ADT7X10_T_HYST_MASK);
> -	ret = adt7x10_write_byte(dev, ADT7X10_T_HYST, data->hyst);
> -	if (ret)
> -		return ret;
> -
> -	return count;
> +			       0, ADT7X10_T_HYST_MASK);
> +	return adt7x10_write_byte(dev, ADT7X10_T_HYST, data->hyst);
>   }
>   
> -static ssize_t adt7x10_alarm_show(struct device *dev,
> -				  struct device_attribute *da, char *buf)
> +static int adt7x10_alarm_read(struct adt7x10_data *data, unsigned int index,
> +			      long *val)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	struct device *dev = data->bus_dev;
>   	int ret;
>   
>   	ret = adt7x10_read_byte(dev, ADT7X10_STATUS);
>   	if (ret < 0)
>   		return ret;
>   
> -	return sprintf(buf, "%d\n", !!(ret & attr->index));
> +	*val = !!(ret & index);
> +
> +	return 0;
> +}
> +
> +static umode_t adt7x10_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	umode_t mode = 0444;
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +	case hwmon_temp_min:
> +	case hwmon_temp_crit:
> +	case hwmon_temp_max_hyst:
> +		mode |= 0200;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return mode;
> +}
> +
> +static int adt7x10_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct adt7x10_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		return adt7x10_temp_read(data, 0, val);
> +	case hwmon_temp_max:
> +		return adt7x10_temp_read(data, 1, val);
> +	case hwmon_temp_min:
> +		return adt7x10_temp_read(data, 2, val);
> +	case hwmon_temp_crit:
> +		return adt7x10_temp_read(data, 3, val);
> +	case hwmon_temp_max_hyst:
> +		return adt7x10_hyst_read(data, 1, val);
> +	case hwmon_temp_min_hyst:
> +		return adt7x10_hyst_read(data, 2, val);
> +	case hwmon_temp_crit_hyst:
> +		return adt7x10_hyst_read(data, 3, val);
> +	case hwmon_temp_min_alarm:
> +		return adt7x10_alarm_read(data, ADT7X10_STAT_T_LOW, val);
> +	case hwmon_temp_max_alarm:
> +		return adt7x10_alarm_read(data, ADT7X10_STAT_T_HIGH, val);
> +	case hwmon_temp_crit_alarm:
> +		return adt7x10_alarm_read(data, ADT7X10_STAT_T_CRIT, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>   }
>   
> -static ssize_t name_show(struct device *dev, struct device_attribute *da,
> -			 char *buf)
> +static int adt7x10_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
>   {
>   	struct adt7x10_data *data = dev_get_drvdata(dev);
>   
> -	return sprintf(buf, "%s\n", data->name);
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		return adt7x10_temp_write(data, 1, val);
> +	case hwmon_temp_min:
> +		return adt7x10_temp_write(data, 2, val);
> +	case hwmon_temp_crit:
> +		return adt7x10_temp_write(data, 3, val);
> +	case hwmon_temp_max_hyst:
> +		return adt7x10_hyst_write(data, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>   }
>   
> -static SENSOR_DEVICE_ATTR_RO(temp1_input, adt7x10_temp, 0);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max, adt7x10_temp, 1);
> -static SENSOR_DEVICE_ATTR_RW(temp1_min, adt7x10_temp, 2);
> -static SENSOR_DEVICE_ATTR_RW(temp1_crit, adt7x10_temp, 3);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max_hyst, adt7x10_t_hyst, 1);
> -static SENSOR_DEVICE_ATTR_RO(temp1_min_hyst, adt7x10_t_hyst, 2);
> -static SENSOR_DEVICE_ATTR_RO(temp1_crit_hyst, adt7x10_t_hyst, 3);
> -static SENSOR_DEVICE_ATTR_RO(temp1_min_alarm, adt7x10_alarm,
> -			     ADT7X10_STAT_T_LOW);
> -static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, adt7x10_alarm,
> -			     ADT7X10_STAT_T_HIGH);
> -static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, adt7x10_alarm,
> -			     ADT7X10_STAT_T_CRIT);
> -static DEVICE_ATTR_RO(name);
> -
> -static struct attribute *adt7x10_attributes[] = {
> -	&sensor_dev_attr_temp1_input.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max.dev_attr.attr,
> -	&sensor_dev_attr_temp1_min.dev_attr.attr,
> -	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> -	&sensor_dev_attr_temp1_min_hyst.dev_attr.attr,
> -	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> -	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
> -	NULL
> +static const struct hwmon_channel_info *adt7x10_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MIN |
> +			   HWMON_T_CRIT | HWMON_T_MAX_HYST | HWMON_T_MIN_HYST |
> +			   HWMON_T_CRIT_HYST | HWMON_T_MIN_ALARM |
> +			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM),
> +	NULL,
> +};
> +
> +static const struct hwmon_ops adt7x10_hwmon_ops = {
> +	.is_visible = adt7x10_is_visible,
> +	.read = adt7x10_read,
> +	.write = adt7x10_write,
>   };
>   
> -static const struct attribute_group adt7x10_group = {
> -	.attrs = adt7x10_attributes,
> +static const struct hwmon_chip_info adt7x10_chip_info = {
> +	.ops = &adt7x10_hwmon_ops,
> +	.info = adt7x10_info,
>   };
>   
>   static void adt7x10_restore_config(void *private)
> @@ -372,6 +398,7 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   		  const struct adt7x10_ops *ops)
>   {
>   	struct adt7x10_data *data;
> +	struct device *hdev;
>   	int ret;
>   
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> @@ -379,7 +406,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   		return -ENOMEM;
>   
>   	data->ops = ops;
> -	data->name = name;
>   	data->bus_dev = dev;
>   
>   	dev_set_drvdata(dev, data);
> @@ -416,26 +442,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	if (ret)
>   		return ret;
>   
> -	/* Register sysfs hooks */
> -	ret = sysfs_create_group(&dev->kobj, &adt7x10_group);
> -	if (ret)
> +	hdev = devm_hwmon_device_register_with_info(dev, name, data,
> +						    &adt7x10_chip_info, NULL);
> +	if (IS_ERR(hdev)) {
> +		ret = PTR_ERR(hdev);
>   		return ret;
> -
> -	/*
> -	 * The I2C device will already have it's own 'name' attribute, but for
> -	 * the SPI device we need to register it. name will only be non NULL if
> -	 * the device doesn't register the 'name' attribute on its own.
> -	 */
> -	if (name) {
> -		ret = device_create_file(dev, &dev_attr_name);
> -		if (ret)
> -			goto exit_remove;
> -	}
> -
> -	data->hwmon_dev = hwmon_device_register(dev);
> -	if (IS_ERR(data->hwmon_dev)) {
> -		ret = PTR_ERR(data->hwmon_dev);
> -		goto exit_remove_name;
>   	}
>   
>   	if (irq > 0) {
> @@ -443,33 +454,17 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>   				dev_name(dev), dev);
>   		if (ret)
> -			goto exit_hwmon_device_unregister;
> +			return ret;
>   	}
>   
>   	return 0;
> -
> -exit_hwmon_device_unregister:
> -	hwmon_device_unregister(data->hwmon_dev);
> -exit_remove_name:
> -	if (name)
> -		device_remove_file(dev, &dev_attr_name);
> -exit_remove:
> -	sysfs_remove_group(&dev->kobj, &adt7x10_group);
> -	return ret;
>   }
>   EXPORT_SYMBOL_GPL(adt7x10_probe);
>   
>   void adt7x10_remove(struct device *dev, int irq)
>   {
> -	struct adt7x10_data *data = dev_get_drvdata(dev);
> -
>   	if (irq > 0)
>   		free_irq(irq, dev);
> -
> -	hwmon_device_unregister(data->hwmon_dev);
> -	if (data->name)
> -		device_remove_file(dev, &dev_attr_name);
> -	sysfs_remove_group(&dev->kobj, &adt7x10_group);
>   }
>   EXPORT_SYMBOL_GPL(adt7x10_remove);
>   
> 

[PATCH v2 5/8] hwmon: (adt7x10) Use devm_request_threaded_irq

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

To simplify the core driver remove function.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 2e6e54bf1d15..84e8879d4ca7 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -450,9 +450,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	}
 
 	if (irq > 0) {
-		ret = request_threaded_irq(irq, NULL, adt7x10_irq_handler,
-				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				dev_name(dev), dev);
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						adt7x10_irq_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						dev_name(dev), dev);
 		if (ret)
 			return ret;
 	}
@@ -463,8 +465,6 @@ EXPORT_SYMBOL_GPL(adt7x10_probe);
 
 void adt7x10_remove(struct device *dev, int irq)
 {
-	if (irq > 0)
-		free_irq(irq, dev);
 }
 EXPORT_SYMBOL_GPL(adt7x10_remove);
 
-- 
2.34.1

[PATCH v2 6/8] hwmon: (adt7x10) Remove empty driver removal callback

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

Not used to do anything anymore.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7310.c | 7 -------
 drivers/hwmon/adt7410.c | 7 -------
 drivers/hwmon/adt7x10.c | 5 -----
 drivers/hwmon/adt7x10.h | 1 -
 4 files changed, 20 deletions(-)

diff --git a/drivers/hwmon/adt7310.c b/drivers/hwmon/adt7310.c
index c40cac16af68..4a69b394525b 100644
--- a/drivers/hwmon/adt7310.c
+++ b/drivers/hwmon/adt7310.c
@@ -88,12 +88,6 @@ static int adt7310_spi_probe(struct spi_device *spi)
 			&adt7310_spi_ops);
 }
 
-static int adt7310_spi_remove(struct spi_device *spi)
-{
-	adt7x10_remove(&spi->dev, spi->irq);
-	return 0;
-}
-
 static const struct spi_device_id adt7310_id[] = {
 	{ "adt7310", 0 },
 	{ "adt7320", 0 },
@@ -107,7 +101,6 @@ static struct spi_driver adt7310_driver = {
 		.pm	= ADT7X10_DEV_PM_OPS,
 	},
 	.probe		= adt7310_spi_probe,
-	.remove		= adt7310_spi_remove,
 	.id_table	= adt7310_id,
 };
 module_spi_driver(adt7310_driver);
diff --git a/drivers/hwmon/adt7410.c b/drivers/hwmon/adt7410.c
index ef4b5af865e9..2ad79533a384 100644
--- a/drivers/hwmon/adt7410.c
+++ b/drivers/hwmon/adt7410.c
@@ -48,12 +48,6 @@ static int adt7410_i2c_probe(struct i2c_client *client)
 	return adt7x10_probe(&client->dev, client->name, client->irq, &adt7410_i2c_ops);
 }
 
-static int adt7410_i2c_remove(struct i2c_client *client)
-{
-	adt7x10_remove(&client->dev, client->irq);
-	return 0;
-}
-
 static const struct i2c_device_id adt7410_ids[] = {
 	{ "adt7410", 0 },
 	{ "adt7420", 0 },
@@ -68,7 +62,6 @@ static struct i2c_driver adt7410_driver = {
 		.pm	= ADT7X10_DEV_PM_OPS,
 	},
 	.probe_new	= adt7410_i2c_probe,
-	.remove		= adt7410_i2c_remove,
 	.id_table	= adt7410_ids,
 	.address_list	= I2C_ADDRS(0x48, 0x49, 0x4a, 0x4b),
 };
diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 84e8879d4ca7..72d3a5de359e 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -463,11 +463,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 }
 EXPORT_SYMBOL_GPL(adt7x10_probe);
 
-void adt7x10_remove(struct device *dev, int irq)
-{
-}
-EXPORT_SYMBOL_GPL(adt7x10_remove);
-
 #ifdef CONFIG_PM_SLEEP
 
 static int adt7x10_suspend(struct device *dev)
diff --git a/drivers/hwmon/adt7x10.h b/drivers/hwmon/adt7x10.h
index a1ae682eb32e..bde674e24ca0 100644
--- a/drivers/hwmon/adt7x10.h
+++ b/drivers/hwmon/adt7x10.h
@@ -26,7 +26,6 @@ struct adt7x10_ops {
 
 int adt7x10_probe(struct device *dev, const char *name, int irq,
 	const struct adt7x10_ops *ops);
-void adt7x10_remove(struct device *dev, int irq);
 
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops adt7x10_dev_pm_ops;
-- 
2.34.1

[PATCH v2 7/8] hwmon: (adt7x10) Pass hwinfo dev to irq handler

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

The irq handler will need to trigger events on the
hwmon device. Pass it so that we don't store it in
private data.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 72d3a5de359e..964cbe43de46 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -101,7 +101,9 @@ static const u8 ADT7X10_REG_TEMP[4] = {
 
 static irqreturn_t adt7x10_irq_handler(int irq, void *private)
 {
-	struct device *dev = private;
+	struct device *hdev = private;
+	struct adt7x10_data *d = dev_get_drvdata(hdev);
+	struct device *dev = d->bus_dev;
 	int status;
 
 	status = adt7x10_read_byte(dev, ADT7X10_STATUS);
@@ -454,7 +456,7 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 						adt7x10_irq_handler,
 						IRQF_TRIGGER_FALLING |
 						IRQF_ONESHOT,
-						dev_name(dev), dev);
+						dev_name(dev), hdev);
 		if (ret)
 			return ret;
 	}
-- 
2.34.1

[PATCH v2 8/8] hwmon: (adt7x10) Use hwmon_notify_event

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

The hwmon subsystem provides means of notifying userspace
about events. Use it.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 964cbe43de46..994160bd8901 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -111,11 +111,11 @@ static irqreturn_t adt7x10_irq_handler(int irq, void *private)
 		return IRQ_HANDLED;
 
 	if (status & ADT7X10_STAT_T_HIGH)
-		sysfs_notify(&dev->kobj, NULL, "temp1_max_alarm");
+		hwmon_notify_event(hdev, hwmon_temp, hwmon_temp_max_alarm, 0);
 	if (status & ADT7X10_STAT_T_LOW)
-		sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
+		hwmon_notify_event(hdev, hwmon_temp, hwmon_temp_min_alarm, 0);
 	if (status & ADT7X10_STAT_T_CRIT)
-		sysfs_notify(&dev->kobj, NULL, "temp1_crit_alarm");
+		hwmon_notify_event(hdev, hwmon_temp, hwmon_temp_crit_alarm, 0);
 
 	return IRQ_HANDLED;
 }
-- 
2.34.1