[PATCH v3 0/7] Refactor ADT7X10

[PATCH v3 0/7] 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

V2 -> V3:
 * merge patch that passes name from i2c driver into the
   devm_hwmon_device_register_with_info patch

Cosmin Tanislav (7):
  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 v3 1/7] 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 v3 2/7] 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 v3 3/7] 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.
Also, pass name to core driver for the i2c device so that
it can be used to register hwmon device.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7410.c |   2 +-
 drivers/hwmon/adt7x10.c | 233 ++++++++++++++++++++--------------------
 2 files changed, 115 insertions(+), 120 deletions(-)

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)
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 v3 3/7] hwmon: (adt7x10) Use devm_hwmon_device_register_with_info

[Guenter Roeck]

On 12/21/21 1:58 PM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
[ ... ]

> +
> +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);

Ok, so you want to keep using the internal "index" to indicate the
array position in the register cache. I _did_ specifically ask
to use defines for index values in this case. You did not explain
why you ignored this. So now you'll have to explain 1) why you
ignored my request and 2) why you want to keep the code as is.

And, _please_, add a To: recipient to your patches. I am getting
tired having to handle the fallout.

Guenter

Re: [PATCH v3 3/7] hwmon: (adt7x10) Use devm_hwmon_device_register_with_info

[Cosmin Tanislav]

On 12/22/21 05:08, Guenter Roeck wrote:
> On 12/21/21 1:58 PM, Cosmin Tanislav wrote:
>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>
> [ ... ]
> 
>> +
>> +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);
> 
> Ok, so you want to keep using the internal "index" to indicate the
> array position in the register cache. I _did_ specifically ask
> to use defines for index values in this case. You did not explain
> why you ignored this. So now you'll have to explain 1) why you
> ignored my request and 2) why you want to keep the code as is.
> 
> And, _please_, add a To: recipient to your patches. I am getting
> tired having to handle the fallout.
> 
> Guenter


First of all, maybe you should compare the time these patches were
sent to the time that your reply that "I ignored" was sent.
I sent the patches before you "specifically asked me to use defines for
index values in this case".

Second of all, this specific place is a 1:1 conversion from the original 
code. If I change to using defines here, I'll have to change to using
defines everywhere else in the code, which doesn't seem to be the scope
of this exact patch. Of course it looks bad, but it looked equally bad
before. I can introduce more following patches that fix some obvious
non-functional problems with the driver.

Third of all, why are you so tense? You're making both of our lives
harder. I understand being an upstream maintainer is hard work, but
everyone's job is hard work. It's not like I wanted to work on
refactoring ADT7x10, I just had to so I can later introduce a
patch that implements debugfs reg access...

Re: [PATCH v3 3/7] hwmon: (adt7x10) Use devm_hwmon_device_register_with_info

[Guenter Roeck]

On 12/21/21 10:35 PM, Cosmin Tanislav wrote:
> 
> 
> On 12/22/21 05:08, Guenter Roeck wrote:
>> On 12/21/21 1:58 PM, Cosmin Tanislav wrote:
>>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>>
>> [ ... ]
>>
>>> +
>>> +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);
>>
>> Ok, so you want to keep using the internal "index" to indicate the
>> array position in the register cache. I _did_ specifically ask
>> to use defines for index values in this case. You did not explain
>> why you ignored this. So now you'll have to explain 1) why you
>> ignored my request and 2) why you want to keep the code as is.
>>
>> And, _please_, add a To: recipient to your patches. I am getting
>> tired having to handle the fallout.
>>
>> Guenter
> 
> 
> First of all, maybe you should compare the time these patches were
> sent to the time that your reply that "I ignored" was sent.
> I sent the patches before you "specifically asked me to use defines for
> index values in this case".
> 

Maybe it would be beneficial to wait a few minutes to give me time
to review all patches of a series before sending a new series.

> Second of all, this specific place is a 1:1 conversion from the original code. If I change to using defines here, I'll have to change to using
> defines everywhere else in the code, which doesn't seem to be the scope
> of this exact patch. Of course it looks bad, but it looked equally bad

As suggested, you could have dropped the use of 'index' entirely.
Its use was a side effect of the old hwmon API, where extra fields
in sensor attributes were used to separate attributes. Using the
with_info API, that is no longer necessary. You chose to keep that
part of the code, which is fine, but then I am also asking for
changing the hard coded constants to make it easier to understand
the code going forward. This is especially necessary because "index"
now has two meanings: First, it is the sensor channel, as passed
from the hwmon subsystem and, second, it is the index into
the register cache. Those different use cases now need to be
clearly separated, and the best way to do that is to use an enum
or define to indicate the index into the register cache.

... and I specifically asked for that because I got confused
about it while reviewing the code. People looking at the code
at a later time should not have that problem.

> before. I can introduce more following patches that fix some obvious
> non-functional problems with the driver.
> 
> Third of all, why are you so tense? You're making both of our lives
> harder. I understand being an upstream maintainer is hard work, but
> everyone's job is hard work. It's not like I wanted to work on
> refactoring ADT7x10, I just had to so I can later introduce a
> patch that implements debugfs reg access...

I tend to get more tense if people give me extra work, and do so
repeatedly (like sending follow-up patch series tagged as v1 and
with no change log). I am human, after all, Sorry for that.

Guenter

[PATCH v3 4/7] 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 v3 5/7] 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 v3 6/7] 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 v3 7/7] 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