[PATCH v1 01/10] hwmon: adt7x10: store bus_dev in private data

[PATCH v1 01/10] 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 v1 02/10] hwmon: adt7x10: do not create name attr

[Cosmin Tanislav]

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

It will later be created automatically by hwmon.

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

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 2439da9b64e6..dbe9f1ad7db0 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -54,7 +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;
@@ -316,14 +315,6 @@ static ssize_t adt7x10_alarm_show(struct device *dev,
 	return sprintf(buf, "%d\n", !!(ret & attr->index));
 }
 
-static ssize_t name_show(struct device *dev, struct device_attribute *da,
-			 char *buf)
-{
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%s\n", data->name);
-}
-
 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);
@@ -337,7 +328,6 @@ 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,
@@ -368,7 +358,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);
@@ -406,21 +395,10 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	if (ret)
 		goto exit_restore;
 
-	/*
-	 * 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;
+		goto exit_remove;
 	}
 
 	if (irq > 0) {
@@ -435,9 +413,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 
 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);
 exit_restore:
@@ -454,8 +429,6 @@ void adt7x10_remove(struct device *dev, int irq)
 		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);
 	if (data->oldconfig != data->config)
 		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
-- 
2.34.1

Re: [PATCH v1 02/10] hwmon: adt7x10: do not create name attr

[Guenter Roeck]

On Tue, Dec 21, 2021 at 02:39:36PM +0200, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> It will later be created automatically by hwmon.
> 
Patches have to work on their own to retain bisectability. 
The driver would be non-operational after this patch is applied
until the registration function is changed. Please merge this
patch into the with_info rework patch.

Guenter

> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  drivers/hwmon/adt7x10.c | 29 +----------------------------
>  1 file changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index 2439da9b64e6..dbe9f1ad7db0 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -54,7 +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;
> @@ -316,14 +315,6 @@ static ssize_t adt7x10_alarm_show(struct device *dev,
>  	return sprintf(buf, "%d\n", !!(ret & attr->index));
>  }
>  
> -static ssize_t name_show(struct device *dev, struct device_attribute *da,
> -			 char *buf)
> -{
> -	struct adt7x10_data *data = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%s\n", data->name);
> -}
> -
>  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);
> @@ -337,7 +328,6 @@ 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,
> @@ -368,7 +358,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);
> @@ -406,21 +395,10 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>  	if (ret)
>  		goto exit_restore;
>  
> -	/*
> -	 * 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;
> +		goto exit_remove;
>  	}
>  
>  	if (irq > 0) {
> @@ -435,9 +413,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>  
>  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);
>  exit_restore:
> @@ -454,8 +429,6 @@ void adt7x10_remove(struct device *dev, int irq)
>  		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);
>  	if (data->oldconfig != data->config)
>  		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
> -- 
> 2.34.1
> 

[PATCH v1 03/10] 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 | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index dbe9f1ad7db0..48adc0344e88 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -402,9 +402,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)
 			goto exit_hwmon_device_unregister;
 	}
@@ -425,9 +427,6 @@ 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);
 	sysfs_remove_group(&dev->kobj, &adt7x10_group);
 	if (data->oldconfig != data->config)
-- 
2.34.1

Re: [PATCH v1 03/10] hwmon: adt7x10: use devm_request_threaded_irq

[Guenter Roeck]

On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> 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 | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index dbe9f1ad7db0..48adc0344e88 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -402,9 +402,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)
>   			goto exit_hwmon_device_unregister;
>   	}
> @@ -425,9 +427,6 @@ 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);
>   	sysfs_remove_group(&dev->kobj, &adt7x10_group);
>   	if (data->oldconfig != data->config)
> 

This will keep the interrupt running after the hwmon device was removed,
and after the configuration was restored. If an interrupt occurs at that time,
the handler may potentially access released data. I don't mind making those
changes, but the patches will have to be well sequenced to ensure that each
patch on its own doesn't leave a crippled driver behind. Again, "oh, this will
be ok after the entire series was applied" is not acceptable.

Thanks,
Guenter

[PATCH v1 04/10] hwmon: adt7x10: avoid storing hwinfo dev into private data

[Cosmin Tanislav]

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

Store it later in the code to reduce the number of references
to the private data structure, so it is easier to remove it
altogether.

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

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 48adc0344e88..25c1ab199658 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -351,6 +351,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);
@@ -395,9 +396,9 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	if (ret)
 		goto exit_restore;
 
-	data->hwmon_dev = hwmon_device_register(dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		ret = PTR_ERR(data->hwmon_dev);
+	hdev = hwmon_device_register(dev);
+	if (IS_ERR(hdev)) {
+		ret = PTR_ERR(hdev);
 		goto exit_remove;
 	}
 
@@ -411,10 +412,12 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 			goto exit_hwmon_device_unregister;
 	}
 
+	data->hwmon_dev = hdev;
+
 	return 0;
 
 exit_hwmon_device_unregister:
-	hwmon_device_unregister(data->hwmon_dev);
+	hwmon_device_unregister(hdev);
 exit_remove:
 	sysfs_remove_group(&dev->kobj, &adt7x10_group);
 exit_restore:
-- 
2.34.1

Re: [PATCH v1 04/10] hwmon: adt7x10: avoid storing hwinfo dev into private data

[Guenter Roeck]

On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Store it later in the code to reduce the number of references
> to the private data structure, so it is easier to remove it
> altogether.
> 

This patch pretty pointless as it is only used in the remove function,
and that won't need it after the devm_ registration function is used.

Guenter

> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>   drivers/hwmon/adt7x10.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index 48adc0344e88..25c1ab199658 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -351,6 +351,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);
> @@ -395,9 +396,9 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	if (ret)
>   		goto exit_restore;
>   
> -	data->hwmon_dev = hwmon_device_register(dev);
> -	if (IS_ERR(data->hwmon_dev)) {
> -		ret = PTR_ERR(data->hwmon_dev);
> +	hdev = hwmon_device_register(dev);
> +	if (IS_ERR(hdev)) {
> +		ret = PTR_ERR(hdev);
>   		goto exit_remove;
>   	}
>   
> @@ -411,10 +412,12 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   			goto exit_hwmon_device_unregister;
>   	}
>   
> +	data->hwmon_dev = hdev;
> +
>   	return 0;
>   
>   exit_hwmon_device_unregister:
> -	hwmon_device_unregister(data->hwmon_dev);
> +	hwmon_device_unregister(hdev);
>   exit_remove:
>   	sysfs_remove_group(&dev->kobj, &adt7x10_group);
>   exit_restore:
> 

[PATCH v1 05/10] 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 25c1ab199658..ec7e526b3441 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -102,7 +102,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);
@@ -407,7 +409,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)
 			goto exit_hwmon_device_unregister;
 	}
-- 
2.34.1

[PATCH v1 06/10] hwmon: adt7x10: extract read/write methods from show/store

[Cosmin Tanislav]

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

These will later be used by hwmon read and write callbacks.

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

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index ec7e526b3441..32ea9438ad64 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -219,14 +219,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);
@@ -234,39 +231,62 @@ 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 ssize_t adt7x10_temp_show(struct device *dev,
+				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int nr = attr->index;
-	long temp;
+	long val;
 	int ret;
 
-	ret = kstrtol(buf, 10, &temp);
+	ret = adt7x10_temp_read(data, attr->index, &val);
 	if (ret)
 		return ret;
 
+	return sprintf(buf, "%ld\n", val);
+}
+
+static int adt7x10_temp_write(struct adt7x10_data *data, unsigned int nr,
+			      long temp)
+{
+	struct device *dev = data->bus_dev;
+	int 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 ssize_t adt7x10_temp_store(struct device *dev,
+				  struct device_attribute *da,
+				  const char *buf, size_t count)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct adt7x10_data *data = dev_get_drvdata(dev);
 	int nr = attr->index;
+	long temp;
+	int ret;
+
+	ret = kstrtol(buf, 10, &temp);
+	if (ret)
+		return ret;
+
+	ret = adt7x10_temp_write(data, nr, temp);
+
+	return ret ?: count;
+}
+
+static int adt7x10_hyst_read(struct adt7x10_data *data, unsigned int nr,
+			     long *val)
+{
 	int hyst;
 
 	hyst = (data->hyst & ADT7X10_T_HYST_MASK) * 1000;
@@ -277,44 +297,86 @@ 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 ssize_t adt7x10_t_hyst_show(struct device *dev,
+				   struct device_attribute *da, char *buf)
 {
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int limit, ret;
-	long hyst;
+	int nr = attr->index;
+	long val;
+	int ret;
 
-	ret = kstrtol(buf, 10, &hyst);
+	ret = adt7x10_hyst_read(data, nr, &val);
 	if (ret)
 		return ret;
+
+	return sprintf(buf, "%ld\n", val);
+}
+
+static int adt7x10_hyst_write(struct adt7x10_data *data, long hyst)
+{
+	struct device *dev = data->bus_dev;
+	int limit;
+
 	/* 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);
+			       0, ADT7X10_T_HYST_MASK);
+	return adt7x10_write_byte(dev, ADT7X10_T_HYST, data->hyst);
+}
+
+static ssize_t adt7x10_t_hyst_store(struct device *dev,
+				    struct device_attribute *da,
+				    const char *buf, size_t count)
+{
+	struct adt7x10_data *data = dev_get_drvdata(dev);
+	int ret;
+	long hyst;
+
+	ret = kstrtol(buf, 10, &hyst);
 	if (ret)
 		return ret;
 
-	return count;
+	ret = adt7x10_hyst_write(data, hyst);
+
+	return ret ?: count;
+}
+
+static int adt7x10_alarm_read(struct adt7x10_data *data, unsigned int index,
+			      long *val)
+{
+	struct device *dev = data->bus_dev;
+	int ret;
+
+	ret = adt7x10_read_byte(dev, ADT7X10_STATUS);
+	if (ret < 0)
+		return ret;
+
+	*val = !!(ret & index);
+
+	return 0;
 }
 
 static ssize_t adt7x10_alarm_show(struct device *dev,
 				  struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct adt7x10_data *data = dev_get_drvdata(dev);
+	long val;
 	int ret;
 
-	ret = adt7x10_read_byte(dev, ADT7X10_STATUS);
-	if (ret < 0)
+	ret = adt7x10_alarm_read(data, attr->index, &val);
+	if (ret)
 		return ret;
 
-	return sprintf(buf, "%d\n", !!(ret & attr->index));
+	return sprintf(buf, "%ld\n", val);
 }
 
 static SENSOR_DEVICE_ATTR_RO(temp1_input, adt7x10_temp, 0);
-- 
2.34.1

[PATCH v1 07/10] hwmon: adt7x10: remove attrs

[Cosmin Tanislav]

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

These will be replaced by hwmon read and write callbacks.

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

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 32ea9438ad64..2d36088e1a07 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -236,21 +236,6 @@ static int adt7x10_temp_read(struct adt7x10_data *data, int index, long *val)
 	return 0;
 }
 
-static ssize_t adt7x10_temp_show(struct device *dev,
-				 struct device_attribute *da, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	long val;
-	int ret;
-
-	ret = adt7x10_temp_read(data, attr->index, &val);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "%ld\n", val);
-}
-
 static int adt7x10_temp_write(struct adt7x10_data *data, unsigned int nr,
 			      long temp)
 {
@@ -265,25 +250,6 @@ static int adt7x10_temp_write(struct adt7x10_data *data, unsigned int nr,
 	return ret;
 }
 
-static ssize_t adt7x10_temp_store(struct device *dev,
-				  struct device_attribute *da,
-				  const char *buf, size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int nr = attr->index;
-	long temp;
-	int ret;
-
-	ret = kstrtol(buf, 10, &temp);
-	if (ret)
-		return ret;
-
-	ret = adt7x10_temp_write(data, nr, temp);
-
-	return ret ?: count;
-}
-
 static int adt7x10_hyst_read(struct adt7x10_data *data, unsigned int nr,
 			     long *val)
 {
@@ -303,22 +269,6 @@ static int adt7x10_hyst_read(struct adt7x10_data *data, unsigned int nr,
 	return 0;
 }
 
-static ssize_t adt7x10_t_hyst_show(struct device *dev,
-				   struct device_attribute *da, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int nr = attr->index;
-	long val;
-	int ret;
-
-	ret = adt7x10_hyst_read(data, nr, &val);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "%ld\n", val);
-}
-
 static int adt7x10_hyst_write(struct adt7x10_data *data, long hyst)
 {
 	struct device *dev = data->bus_dev;
@@ -332,23 +282,6 @@ static int adt7x10_hyst_write(struct adt7x10_data *data, long hyst)
 	return adt7x10_write_byte(dev, ADT7X10_T_HYST, data->hyst);
 }
 
-static ssize_t adt7x10_t_hyst_store(struct device *dev,
-				    struct device_attribute *da,
-				    const char *buf, size_t count)
-{
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	int ret;
-	long hyst;
-
-	ret = kstrtol(buf, 10, &hyst);
-	if (ret)
-		return ret;
-
-	ret = adt7x10_hyst_write(data, hyst);
-
-	return ret ?: count;
-}
-
 static int adt7x10_alarm_read(struct adt7x10_data *data, unsigned int index,
 			      long *val)
 {
@@ -364,53 +297,6 @@ static int adt7x10_alarm_read(struct adt7x10_data *data, unsigned int index,
 	return 0;
 }
 
-static ssize_t adt7x10_alarm_show(struct device *dev,
-				  struct device_attribute *da, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct adt7x10_data *data = dev_get_drvdata(dev);
-	long val;
-	int ret;
-
-	ret = adt7x10_alarm_read(data, attr->index, &val);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "%ld\n", val);
-}
-
-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 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 attribute_group adt7x10_group = {
-	.attrs = adt7x10_attributes,
-};
-
 int adt7x10_probe(struct device *dev, const char *name, int irq,
 		  const struct adt7x10_ops *ops)
 {
@@ -455,15 +341,10 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	if (ret)
 		goto exit_restore;
 
-	/* Register sysfs hooks */
-	ret = sysfs_create_group(&dev->kobj, &adt7x10_group);
-	if (ret)
-		goto exit_restore;
-
 	hdev = hwmon_device_register(dev);
 	if (IS_ERR(hdev)) {
 		ret = PTR_ERR(hdev);
-		goto exit_remove;
+		goto exit_restore;
 	}
 
 	if (irq > 0) {
@@ -482,8 +363,6 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 
 exit_hwmon_device_unregister:
 	hwmon_device_unregister(hdev);
-exit_remove:
-	sysfs_remove_group(&dev->kobj, &adt7x10_group);
 exit_restore:
 	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
 	return ret;
@@ -495,7 +374,6 @@ void adt7x10_remove(struct device *dev, int irq)
 	struct adt7x10_data *data = dev_get_drvdata(dev);
 
 	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&dev->kobj, &adt7x10_group);
 	if (data->oldconfig != data->config)
 		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
 }
-- 
2.34.1

Re: [PATCH v1 07/10] hwmon: adt7x10: remove attrs

[Guenter Roeck]

On Tue, Dec 21, 2021 at 02:39:41PM +0200, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> These will be replaced by hwmon read and write callbacks.
> 

Again, this is a no-go. The driver is no longer operational
after this patch was applied.

The split into multiple patches is interesting, but it is confusing,
adds a lot of complexity, and makes it impossible to review the series.

Please combine all patches related to the with_info conversion into
a single patch.

Thanks,
Guenter

[PATCH v1 08/10] hwmon: adt7x10: use 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 | 93 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 2d36088e1a07..dd4901299590 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -297,6 +297,95 @@ static int adt7x10_alarm_read(struct adt7x10_data *data, unsigned int 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 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);
+
+	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 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 hwmon_chip_info adt7x10_chip_info = {
+	.ops = &adt7x10_hwmon_ops,
+	.info = adt7x10_info,
+};
+
 int adt7x10_probe(struct device *dev, const char *name, int irq,
 		  const struct adt7x10_ops *ops)
 {
@@ -341,7 +430,9 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	if (ret)
 		goto exit_restore;
 
-	hdev = hwmon_device_register(dev);
+	hdev = hwmon_device_register_with_info(dev, name, data,
+					       &adt7x10_chip_info, NULL);
+
 	if (IS_ERR(hdev)) {
 		ret = PTR_ERR(hdev);
 		goto exit_restore;
-- 
2.34.1

[PATCH v1 09/10] hwmon: adt7x10: use devm_hwmon_device_register_with_info

[Cosmin Tanislav]

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

To simplify core driver remove function.

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

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index dd4901299590..c03805c72906 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -54,7 +54,6 @@
 /* Each client has this additional data */
 struct adt7x10_data {
 	const struct adt7x10_ops *ops;
-	struct device		*hwmon_dev;
 	struct device		*bus_dev;
 	struct mutex		update_lock;
 	u8			config;
@@ -430,8 +429,8 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 	if (ret)
 		goto exit_restore;
 
-	hdev = hwmon_device_register_with_info(dev, name, data,
-					       &adt7x10_chip_info, NULL);
+	hdev = devm_hwmon_device_register_with_info(dev, name, data,
+						    &adt7x10_chip_info, NULL);
 
 	if (IS_ERR(hdev)) {
 		ret = PTR_ERR(hdev);
@@ -445,15 +444,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
 						IRQF_ONESHOT,
 						dev_name(dev), hdev);
 		if (ret)
-			goto exit_hwmon_device_unregister;
+			goto exit_restore;
 	}
 
-	data->hwmon_dev = hdev;
-
 	return 0;
 
-exit_hwmon_device_unregister:
-	hwmon_device_unregister(hdev);
 exit_restore:
 	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
 	return ret;
@@ -464,7 +459,6 @@ void adt7x10_remove(struct device *dev, int irq)
 {
 	struct adt7x10_data *data = dev_get_drvdata(dev);
 
-	hwmon_device_unregister(data->hwmon_dev);
 	if (data->oldconfig != data->config)
 		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
 }
-- 
2.34.1

Re: [PATCH v1 09/10] hwmon: adt7x10: use devm_hwmon_device_register_with_info

[Guenter Roeck]

On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> To simplify core driver remove function.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>   drivers/hwmon/adt7x10.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index dd4901299590..c03805c72906 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -54,7 +54,6 @@
>   /* Each client has this additional data */
>   struct adt7x10_data {
>   	const struct adt7x10_ops *ops;
> -	struct device		*hwmon_dev;
>   	struct device		*bus_dev;
>   	struct mutex		update_lock;
>   	u8			config;
> @@ -430,8 +429,8 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	if (ret)
>   		goto exit_restore;
>   
> -	hdev = hwmon_device_register_with_info(dev, name, data,
> -					       &adt7x10_chip_info, NULL);
> +	hdev = devm_hwmon_device_register_with_info(dev, name, data,
> +						    &adt7x10_chip_info, NULL);
>   
>   	if (IS_ERR(hdev)) {
>   		ret = PTR_ERR(hdev);
> @@ -445,15 +444,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   						IRQF_ONESHOT,
>   						dev_name(dev), hdev);
>   		if (ret)
> -			goto exit_hwmon_device_unregister;
> +			goto exit_restore;
>   	}
>   
> -	data->hwmon_dev = hdev;
> -
>   	return 0;
>   
> -exit_hwmon_device_unregister:
> -	hwmon_device_unregister(hdev);
>   exit_restore:
>   	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
>   	return ret;
> @@ -464,7 +459,6 @@ void adt7x10_remove(struct device *dev, int irq)
>   {
>   	struct adt7x10_data *data = dev_get_drvdata(dev);
>   
> -	hwmon_device_unregister(data->hwmon_dev);
>   	if (data->oldconfig != data->config)
>   		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);

This doesn't work as-is because the hwmon device still exists at this point
and at least in theory userspace could still write into the device
after the old configuration was restored.

To fix this, you'll need a preceding patch to introduce adt7x10_restore_config()
or similar, and call it through devm_add_action_or_reset() after updating the
chip configuration. You can then drop the restore code from here and from
the exist_restore code in the probe function.
After that, you can use devm_hwmon_device_register_with_info() and drop the
remove function entirely.

Guenter

[PATCH v1 10/10] 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 c03805c72906..1a99989a744d 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