diff mbox series

[v2,4/4] hwmon: (adt7470) Use standard update_interval property

Message ID 20210826024121.15665-5-chris.packham@alliedtelesis.co.nz (mailing list archive)
State Rejected
Headers show
Series hwmon: (adt7470) Clean up | expand

Commit Message

Chris Packham Aug. 26, 2021, 2:41 a.m. UTC 
Instead of the non-standard auto_update_interval make use of the
update_interval property that is supported by the hwmon core.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    I kind of anticipate a NAK on this because it affects the ABI. But I figured
    I'd run it past the ML to see if moving towards the hwmon core is worth the hit
    in ABI compatibility.
    
    Changes in v2:
    - none

 drivers/hwmon/adt7470.c | 64 +++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 25 deletions(-)

Comments

Guenter Roeck Aug. 27, 2021, 9:29 p.m. UTC | #1 
On Thu, Aug 26, 2021 at 02:41:21PM +1200, Chris Packham wrote:
> Instead of the non-standard auto_update_interval make use of the
> update_interval property that is supported by the hwmon core.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     I kind of anticipate a NAK on this because it affects the ABI. But I figured
>     I'd run it past the ML to see if moving towards the hwmon core is worth the hit
>     in ABI compatibility.
>     
I personally don't mind (most likely no one is using it anyway), but let's
wait until after the upcoming commit window closes to give people time to
complain.

Guenter

>     Changes in v2:
>     - none
> 
>  drivers/hwmon/adt7470.c | 64 +++++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index db19a52b13de..7afbd1e4721e 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -469,35 +469,37 @@ static struct adt7470_data *adt7470_update_device(struct device *dev)
>  	return err < 0 ? ERR_PTR(err) : data;
>  }
>  
> -static ssize_t auto_update_interval_show(struct device *dev,
> -					 struct device_attribute *devattr,
> -					 char *buf)
> -{
> -	struct adt7470_data *data = adt7470_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", data->auto_update_interval);
> -}
> -
> -static ssize_t auto_update_interval_store(struct device *dev,
> -					  struct device_attribute *devattr,
> -					  const char *buf, size_t count)
> +static int adt7470_chip_read(struct device *dev, u32 attr, long *val)
>  {
>  	struct adt7470_data *data = dev_get_drvdata(dev);
> -	long temp;
>  
> -	if (kstrtol(buf, 10, &temp))
> -		return -EINVAL;
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		*val = data->auto_update_interval;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  
> -	temp = clamp_val(temp, 0, 60000);
> +	return 0;
> +}
>  
> -	mutex_lock(&data->lock);
> -	data->auto_update_interval = temp;
> -	mutex_unlock(&data->lock);
> +static int adt7470_chip_write(struct device *dev, u32 attr, long val)
> +{
> +	struct adt7470_data *data = dev_get_drvdata(dev);
>  
> -	return count;
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		val = clamp_val(val, 0, 60000);
> +		mutex_lock(&data->lock);
> +		data->auto_update_interval = val;
> +		mutex_unlock(&data->lock);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
>  }
>  
>  static ssize_t num_temp_sensors_show(struct device *dev,
> @@ -1034,7 +1036,6 @@ static ssize_t pwm_auto_temp_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(alarm_mask);
>  static DEVICE_ATTR_RW(num_temp_sensors);
> -static DEVICE_ATTR_RW(auto_update_interval);
>  
>  static SENSOR_DEVICE_ATTR_RW(force_pwm_max, force_pwm_max, 0);
>  
> @@ -1066,7 +1067,6 @@ static SENSOR_DEVICE_ATTR_RW(pwm4_auto_channels_temp, pwm_auto_temp, 3);
>  static struct attribute *adt7470_attrs[] = {
>  	&dev_attr_alarm_mask.attr,
>  	&dev_attr_num_temp_sensors.attr,
> -	&dev_attr_auto_update_interval.attr,
>  	&sensor_dev_attr_force_pwm_max.dev_attr.attr,
>  	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
>  	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
> @@ -1097,6 +1097,8 @@ static int adt7470_read(struct device *dev, enum hwmon_sensor_types type, u32 at
>  			int channel, long *val)
>  {
>  	switch (type) {
> +	case hwmon_chip:
> +		return adt7470_chip_read(dev, attr, val);
>  	case hwmon_temp:
>  		return adt7470_temp_read(dev, attr, channel, val);
>  	case hwmon_fan:
> @@ -1112,6 +1114,8 @@ static int adt7470_write(struct device *dev, enum hwmon_sensor_types type, u32 a
>  			int channel, long val)
>  {
>  	switch (type) {
> +	case hwmon_chip:
> +		return adt7470_chip_write(dev, attr, val);
>  	case hwmon_temp:
>  		return adt7470_temp_write(dev, attr, channel, val);
>  	case hwmon_fan:
> @@ -1129,6 +1133,15 @@ static umode_t adt7470_is_visible(const void *_data, enum hwmon_sensor_types typ
>  	umode_t mode = 0;
>  
>  	switch (type) {
> +	case hwmon_chip:
> +		switch (attr) {
> +		case hwmon_chip_update_interval:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
>  	case hwmon_temp:
>  		switch (attr) {
>  		case hwmon_temp:
> @@ -1187,6 +1200,7 @@ static const struct hwmon_ops adt7470_hwmon_ops = {
>  };
>  
>  static const struct hwmon_channel_info *adt7470_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
>  	HWMON_CHANNEL_INFO(temp,
>  			HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_ALARM,
>  			HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_ALARM,
Chris Packham Aug. 29, 2021, 9:09 p.m. UTC | #2 
On 28/08/21 9:29 am, Guenter Roeck wrote:
> On Thu, Aug 26, 2021 at 02:41:21PM +1200, Chris Packham wrote:
>> Instead of the non-standard auto_update_interval make use of the
>> update_interval property that is supported by the hwmon core.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      I kind of anticipate a NAK on this because it affects the ABI. But I figured
>>      I'd run it past the ML to see if moving towards the hwmon core is worth the hit
>>      in ABI compatibility.
>>      
> I personally don't mind (most likely no one is using it anyway), but let's
> wait until after the upcoming commit window closes to give people time to
> complain.

I know of one application using this sysfs entry. But it's our in-house 
environmental monitoring code so if this gets merged I'll just update it 
to use the new path.

One thought I had was we could do both. i.e. have an entry that conforms 
to the hwmon core and a backwards compatible entry that just aliases the 
new path.

>
> Guenter
>
>>      Changes in v2:
>>      - none
>>
>>   drivers/hwmon/adt7470.c | 64 +++++++++++++++++++++++++----------------
>>   1 file changed, 39 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
>> index db19a52b13de..7afbd1e4721e 100644
>> --- a/drivers/hwmon/adt7470.c
>> +++ b/drivers/hwmon/adt7470.c
>> @@ -469,35 +469,37 @@ static struct adt7470_data *adt7470_update_device(struct device *dev)
>>   	return err < 0 ? ERR_PTR(err) : data;
>>   }
>>   
>> -static ssize_t auto_update_interval_show(struct device *dev,
>> -					 struct device_attribute *devattr,
>> -					 char *buf)
>> -{
>> -	struct adt7470_data *data = adt7470_update_device(dev);
>> -
>> -	if (IS_ERR(data))
>> -		return PTR_ERR(data);
>> -
>> -	return sprintf(buf, "%d\n", data->auto_update_interval);
>> -}
>> -
>> -static ssize_t auto_update_interval_store(struct device *dev,
>> -					  struct device_attribute *devattr,
>> -					  const char *buf, size_t count)
>> +static int adt7470_chip_read(struct device *dev, u32 attr, long *val)
>>   {
>>   	struct adt7470_data *data = dev_get_drvdata(dev);
>> -	long temp;
>>   
>> -	if (kstrtol(buf, 10, &temp))
>> -		return -EINVAL;
>> +	switch (attr) {
>> +	case hwmon_chip_update_interval:
>> +		*val = data->auto_update_interval;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>>   
>> -	temp = clamp_val(temp, 0, 60000);
>> +	return 0;
>> +}
>>   
>> -	mutex_lock(&data->lock);
>> -	data->auto_update_interval = temp;
>> -	mutex_unlock(&data->lock);
>> +static int adt7470_chip_write(struct device *dev, u32 attr, long val)
>> +{
>> +	struct adt7470_data *data = dev_get_drvdata(dev);
>>   
>> -	return count;
>> +	switch (attr) {
>> +	case hwmon_chip_update_interval:
>> +		val = clamp_val(val, 0, 60000);
>> +		mutex_lock(&data->lock);
>> +		data->auto_update_interval = val;
>> +		mutex_unlock(&data->lock);
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static ssize_t num_temp_sensors_show(struct device *dev,
>> @@ -1034,7 +1036,6 @@ static ssize_t pwm_auto_temp_store(struct device *dev,
>>   
>>   static DEVICE_ATTR_RW(alarm_mask);
>>   static DEVICE_ATTR_RW(num_temp_sensors);
>> -static DEVICE_ATTR_RW(auto_update_interval);
>>   
>>   static SENSOR_DEVICE_ATTR_RW(force_pwm_max, force_pwm_max, 0);
>>   
>> @@ -1066,7 +1067,6 @@ static SENSOR_DEVICE_ATTR_RW(pwm4_auto_channels_temp, pwm_auto_temp, 3);
>>   static struct attribute *adt7470_attrs[] = {
>>   	&dev_attr_alarm_mask.attr,
>>   	&dev_attr_num_temp_sensors.attr,
>> -	&dev_attr_auto_update_interval.attr,
>>   	&sensor_dev_attr_force_pwm_max.dev_attr.attr,
>>   	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
>>   	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
>> @@ -1097,6 +1097,8 @@ static int adt7470_read(struct device *dev, enum hwmon_sensor_types type, u32 at
>>   			int channel, long *val)
>>   {
>>   	switch (type) {
>> +	case hwmon_chip:
>> +		return adt7470_chip_read(dev, attr, val);
>>   	case hwmon_temp:
>>   		return adt7470_temp_read(dev, attr, channel, val);
>>   	case hwmon_fan:
>> @@ -1112,6 +1114,8 @@ static int adt7470_write(struct device *dev, enum hwmon_sensor_types type, u32 a
>>   			int channel, long val)
>>   {
>>   	switch (type) {
>> +	case hwmon_chip:
>> +		return adt7470_chip_write(dev, attr, val);
>>   	case hwmon_temp:
>>   		return adt7470_temp_write(dev, attr, channel, val);
>>   	case hwmon_fan:
>> @@ -1129,6 +1133,15 @@ static umode_t adt7470_is_visible(const void *_data, enum hwmon_sensor_types typ
>>   	umode_t mode = 0;
>>   
>>   	switch (type) {
>> +	case hwmon_chip:
>> +		switch (attr) {
>> +		case hwmon_chip_update_interval:
>> +			mode = 0644;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>>   	case hwmon_temp:
>>   		switch (attr) {
>>   		case hwmon_temp:
>> @@ -1187,6 +1200,7 @@ static const struct hwmon_ops adt7470_hwmon_ops = {
>>   };
>>   
>>   static const struct hwmon_channel_info *adt7470_info[] = {
>> +	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
>>   	HWMON_CHANNEL_INFO(temp,
>>   			HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_ALARM,
>>   			HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_ALARM,
Guenter Roeck Aug. 30, 2021, 3:36 p.m. UTC | #3 
On 8/29/21 2:09 PM, Chris Packham wrote:
> 
> On 28/08/21 9:29 am, Guenter Roeck wrote:
>> On Thu, Aug 26, 2021 at 02:41:21PM +1200, Chris Packham wrote:
>>> Instead of the non-standard auto_update_interval make use of the
>>> update_interval property that is supported by the hwmon core.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>>       I kind of anticipate a NAK on this because it affects the ABI. But I figured
>>>       I'd run it past the ML to see if moving towards the hwmon core is worth the hit
>>>       in ABI compatibility.
>>>       
>> I personally don't mind (most likely no one is using it anyway), but let's
>> wait until after the upcoming commit window closes to give people time to
>> complain.
> 
> I know of one application using this sysfs entry. But it's our in-house
> environmental monitoring code so if this gets merged I'll just update it
> to use the new path.
> 
> One thought I had was we could do both. i.e. have an entry that conforms
> to the hwmon core and a backwards compatible entry that just aliases the
> new path.
> 
Now you almost convinced me to indeed reject this patch. The idea of the new API
is to simplify driver code, not to make it more complicated. If we can't simplify
the code, it is better to leave it alone.

Guenter
Chris Packham Aug. 30, 2021, 8:59 p.m. UTC | #4 
On 31/08/21 3:36 am, Guenter Roeck wrote:
> On 8/29/21 2:09 PM, Chris Packham wrote:
>>
>> On 28/08/21 9:29 am, Guenter Roeck wrote:
>>> On Thu, Aug 26, 2021 at 02:41:21PM +1200, Chris Packham wrote:
>>>> Instead of the non-standard auto_update_interval make use of the
>>>> update_interval property that is supported by the hwmon core.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> ---
>>>>
>>>> Notes:
>>>>       I kind of anticipate a NAK on this because it affects the 
>>>> ABI. But I figured
>>>>       I'd run it past the ML to see if moving towards the hwmon 
>>>> core is worth the hit
>>>>       in ABI compatibility.
>>> I personally don't mind (most likely no one is using it anyway), but 
>>> let's
>>> wait until after the upcoming commit window closes to give people 
>>> time to
>>> complain.
>>
>> I know of one application using this sysfs entry. But it's our in-house
>> environmental monitoring code so if this gets merged I'll just update it
>> to use the new path.
>>
>> One thought I had was we could do both. i.e. have an entry that conforms
>> to the hwmon core and a backwards compatible entry that just aliases the
>> new path.
>>
> Now you almost convinced me to indeed reject this patch. The idea of 
> the new API
> is to simplify driver code, not to make it more complicated. If we 
> can't simplify
> the code, it is better to leave it alone.
Sold. I agree what I've just suggested is adding more complexity without 
much gain. If something does start to care about having a standard 
update_interval property we could resurrect this.
>
> Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index db19a52b13de..7afbd1e4721e 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -469,35 +469,37 @@  static struct adt7470_data *adt7470_update_device(struct device *dev)
 	return err < 0 ? ERR_PTR(err) : data;
 }
 
-static ssize_t auto_update_interval_show(struct device *dev,
-					 struct device_attribute *devattr,
-					 char *buf)
-{
-	struct adt7470_data *data = adt7470_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", data->auto_update_interval);
-}
-
-static ssize_t auto_update_interval_store(struct device *dev,
-					  struct device_attribute *devattr,
-					  const char *buf, size_t count)
+static int adt7470_chip_read(struct device *dev, u32 attr, long *val)
 {
 	struct adt7470_data *data = dev_get_drvdata(dev);
-	long temp;
 
-	if (kstrtol(buf, 10, &temp))
-		return -EINVAL;
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		*val = data->auto_update_interval;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
 
-	temp = clamp_val(temp, 0, 60000);
+	return 0;
+}
 
-	mutex_lock(&data->lock);
-	data->auto_update_interval = temp;
-	mutex_unlock(&data->lock);
+static int adt7470_chip_write(struct device *dev, u32 attr, long val)
+{
+	struct adt7470_data *data = dev_get_drvdata(dev);
 
-	return count;
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		val = clamp_val(val, 0, 60000);
+		mutex_lock(&data->lock);
+		data->auto_update_interval = val;
+		mutex_unlock(&data->lock);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
 }
 
 static ssize_t num_temp_sensors_show(struct device *dev,
@@ -1034,7 +1036,6 @@  static ssize_t pwm_auto_temp_store(struct device *dev,
 
 static DEVICE_ATTR_RW(alarm_mask);
 static DEVICE_ATTR_RW(num_temp_sensors);
-static DEVICE_ATTR_RW(auto_update_interval);
 
 static SENSOR_DEVICE_ATTR_RW(force_pwm_max, force_pwm_max, 0);
 
@@ -1066,7 +1067,6 @@  static SENSOR_DEVICE_ATTR_RW(pwm4_auto_channels_temp, pwm_auto_temp, 3);
 static struct attribute *adt7470_attrs[] = {
 	&dev_attr_alarm_mask.attr,
 	&dev_attr_num_temp_sensors.attr,
-	&dev_attr_auto_update_interval.attr,
 	&sensor_dev_attr_force_pwm_max.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
@@ -1097,6 +1097,8 @@  static int adt7470_read(struct device *dev, enum hwmon_sensor_types type, u32 at
 			int channel, long *val)
 {
 	switch (type) {
+	case hwmon_chip:
+		return adt7470_chip_read(dev, attr, val);
 	case hwmon_temp:
 		return adt7470_temp_read(dev, attr, channel, val);
 	case hwmon_fan:
@@ -1112,6 +1114,8 @@  static int adt7470_write(struct device *dev, enum hwmon_sensor_types type, u32 a
 			int channel, long val)
 {
 	switch (type) {
+	case hwmon_chip:
+		return adt7470_chip_write(dev, attr, val);
 	case hwmon_temp:
 		return adt7470_temp_write(dev, attr, channel, val);
 	case hwmon_fan:
@@ -1129,6 +1133,15 @@  static umode_t adt7470_is_visible(const void *_data, enum hwmon_sensor_types typ
 	umode_t mode = 0;
 
 	switch (type) {
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
 	case hwmon_temp:
 		switch (attr) {
 		case hwmon_temp:
@@ -1187,6 +1200,7 @@  static const struct hwmon_ops adt7470_hwmon_ops = {
 };
 
 static const struct hwmon_channel_info *adt7470_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
 	HWMON_CHANNEL_INFO(temp,
 			HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_ALARM,
 			HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_ALARM,