diff mbox series

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

Message ID 20211221215841.2641417-4-demonsingur@gmail.com (mailing list archive)
State Superseded
Headers show
Series Refactor ADT7X10 | expand

Commit Message

Cosmin Tanislav Dec. 21, 2021, 9:58 p.m. UTC 
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(-)

Comments

Guenter Roeck Dec. 22, 2021, 3:08 a.m. UTC | #1 
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
Cosmin Tanislav Dec. 22, 2021, 6:35 a.m. UTC | #2 
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...
Guenter Roeck Dec. 22, 2021, 5:07 p.m. UTC | #3 
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
diff mbox series

Patch

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