[PATCH 0/6] hwmon: Fixes for lm90 driver

[PATCH 0/6] hwmon: Fixes for lm90 driver

[Guenter Roeck]

This series fixes a number of issues with the lm90 driver.

- Not all versions of G781 and compatible chips support a conversion rate
  of 8 (62.5 ms). Limit its value to 0..7.
- Re-enable interrupts/alerts for devices supporting interrupts and with
  broken ALERT after the alert(s) clear.
- Various chips have a buggy ALERT implementation. Mark them accordingly.
- The attributes for sending alarm notifications should be the alarm
  attributes, not the associated value attributes.

----------------------------------------------------------------
Guenter Roeck (6):
      hwmon: (lm90) Reduce maximum conversion rate for G781
      hwmon: (lm90) Re-enable interrupts after alert clears
      hwmon: (lm90) Mark alert as broken for MAX6654
      hwmon: (lm90) Mark alert as broken for MAX6680
      hwmon: (lm90) Mark alert as broken for MAX6646/6647/6649
      hwmon: (lm90) Fix sysfs and udev notifications

 drivers/hwmon/lm90.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

[PATCH 1/6] hwmon: (lm90) Reduce maximum conversion rate for G781

[Guenter Roeck]

According to its datasheet, G781 supports a maximum conversion rate value
of 8 (62.5 ms). However, chips labeled G781 and G780 were found to only
support a maximum conversion rate value of 7 (125 ms). On the other side,
chips labeled G781-1 and G784 were found to support a conversion rate value
of 8. There is no known means to distinguish G780 from G781 or G784; all
chips report the same manufacturer ID and chip revision.
Setting the conversion rate register value to 8 on chips not supporting
it causes unexpected behavior since the real conversion rate is set to 0
(16 seconds) if a value of 8 is written into the conversion rate register.
Limit the conversion rate register value to 7 for all G78x chips to avoid
the problem.

Fixes: ae544f64cc7b ("hwmon: (lm90) Add support for GMT G781")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 74019dff2550..cc5e48fe304b 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -373,7 +373,7 @@ static const struct lm90_params lm90_params[] = {
 		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
 		  | LM90_HAVE_BROKEN_ALERT | LM90_HAVE_CRIT,
 		.alert_alarms = 0x7c,
-		.max_convrate = 8,
+		.max_convrate = 7,
 	},
 	[lm86] = {
 		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
-- 
2.33.0

[PATCH 2/6] hwmon: (lm90) Re-enable interrupts after alert clears

[Guenter Roeck]

If alert handling is broken, interrupts are disabled after an alert and
re-enabled after the alert clears. However, if there is an interrupt
handler, this does not apply if alerts were originally disabled and enabled
when the driver was loaded. In that case, interrupts will stay disabled
after an alert was handled though the alert handler even after the alert
condition clears. Address the situation by always re-enabling interrupts
after the alert condition clears if there is an interrupt handler.

Fixes: 2abdc357c55d9 ("hwmon: (lm90) Unmask hardware interrupt")
Cc: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cc5e48fe304b..e4ecf3440d7c 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -848,7 +848,7 @@ static int lm90_update_device(struct device *dev)
 		 * Re-enable ALERT# output if it was originally enabled and
 		 * relevant alarms are all clear
 		 */
-		if (!(data->config_orig & 0x80) &&
+		if ((client->irq || !(data->config_orig & 0x80)) &&
 		    !(data->alarms & data->alert_alarms)) {
 			if (data->config & 0x80) {
 				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
-- 
2.33.0

Re: [PATCH 2/6] hwmon: (lm90) Re-enable interrupts after alert clears

[Dmitry Osipenko]

11.01.2022 19:51, Guenter Roeck пишет:
> If alert handling is broken, interrupts are disabled after an alert and
> re-enabled after the alert clears. However, if there is an interrupt
> handler, this does not apply if alerts were originally disabled and enabled
> when the driver was loaded. In that case, interrupts will stay disabled
> after an alert was handled though the alert handler even after the alert
> condition clears. Address the situation by always re-enabling interrupts
> after the alert condition clears if there is an interrupt handler.
> 
> Fixes: 2abdc357c55d9 ("hwmon: (lm90) Unmask hardware interrupt")
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm90.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index cc5e48fe304b..e4ecf3440d7c 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -848,7 +848,7 @@ static int lm90_update_device(struct device *dev)
>  		 * Re-enable ALERT# output if it was originally enabled and
>  		 * relevant alarms are all clear
>  		 */
> -		if (!(data->config_orig & 0x80) &&
> +		if ((client->irq || !(data->config_orig & 0x80)) &&
>  		    !(data->alarms & data->alert_alarms)) {
>  			if (data->config & 0x80) {
>  				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
> 

Good catch!

[PATCH 3/6] hwmon: (lm90) Mark alert as broken for MAX6654

[Guenter Roeck]

Experiments with MAX6654 show that its alert function is broken,
similar to other chips supported by the lm90 driver. Mark it accordingly.

Fixes: 229d495d8189 ("hwmon: (lm90) Add max6654 support to lm90 driver")
Cc: Josh Lehan <krellan@google.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index e4ecf3440d7c..280ae5f58187 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -400,6 +400,7 @@ static const struct lm90_params lm90_params[] = {
 		.reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
 	},
 	[max6654] = {
+		.flags = LM90_HAVE_BROKEN_ALERT,
 		.alert_alarms = 0x7c,
 		.max_convrate = 7,
 		.reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
-- 
2.33.0

[PATCH 4/6] hwmon: (lm90) Mark alert as broken for MAX6680

[Guenter Roeck]

Experiments with MAX6680 and MAX6681 show that the alert function of those
chips is broken, similar to other chips supported by the lm90 driver.
Mark it accordingly.

Fixes: 4667bcb8d8fc ("hwmon: (lm90) Introduce chip parameter structure")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 280ae5f58187..06cb971c889b 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -419,7 +419,7 @@ static const struct lm90_params lm90_params[] = {
 	},
 	[max6680] = {
 		.flags = LM90_HAVE_OFFSET | LM90_HAVE_CRIT
-		  | LM90_HAVE_CRIT_ALRM_SWP,
+		  | LM90_HAVE_CRIT_ALRM_SWP | LM90_HAVE_BROKEN_ALERT,
 		.alert_alarms = 0x7c,
 		.max_convrate = 7,
 	},
-- 
2.33.0

[PATCH 5/6] hwmon: (lm90) Mark alert as broken for MAX6646/6647/6649

[Guenter Roeck]

Experiments with MAX6646 and MAX6648 show that the alert function of those
chips is broken, similar to other chips supported by the lm90 driver.
Mark it accordingly.

Fixes: 4667bcb8d8fc ("hwmon: (lm90) Introduce chip parameter structure")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 06cb971c889b..ba01127c1deb 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -394,7 +394,7 @@ static const struct lm90_params lm90_params[] = {
 		.max_convrate = 9,
 	},
 	[max6646] = {
-		.flags = LM90_HAVE_CRIT,
+		.flags = LM90_HAVE_CRIT | LM90_HAVE_BROKEN_ALERT,
 		.alert_alarms = 0x7c,
 		.max_convrate = 6,
 		.reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
-- 
2.33.0

[PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Guenter Roeck]

sysfs and udev notifications need to be sent to the _alarm
attributes, not to the value attributes.

Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
Cc: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index ba01127c1deb..1c9493c70813 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
 
 	if (st & LM90_STATUS_LLOW)
 		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
-				   hwmon_temp_min, 0);
+				   hwmon_temp_min_alarm, 0);
 	if (st & LM90_STATUS_RLOW)
 		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
-				   hwmon_temp_min, 1);
+				   hwmon_temp_min_alarm, 1);
 	if (st2 & MAX6696_STATUS2_R2LOW)
 		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
-				   hwmon_temp_min, 2);
+				   hwmon_temp_min_alarm, 2);
 	if (st & LM90_STATUS_LHIGH)
 		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
-				   hwmon_temp_max, 0);
+				   hwmon_temp_max_alarm, 0);
 	if (st & LM90_STATUS_RHIGH)
 		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
-				   hwmon_temp_max, 1);
+				   hwmon_temp_max_alarm, 1);
 	if (st2 & MAX6696_STATUS2_R2HIGH)
 		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
-				   hwmon_temp_max, 2);
+				   hwmon_temp_max_alarm, 2);
 
 	return true;
 }
-- 
2.33.0

Re: [PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Dmitry Osipenko]

11.01.2022 19:51, Guenter Roeck пишет:
> sysfs and udev notifications need to be sent to the _alarm
> attributes, not to the value attributes.
> 
> Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm90.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index ba01127c1deb..1c9493c70813 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
>  
>  	if (st & LM90_STATUS_LLOW)
>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> -				   hwmon_temp_min, 0);
> +				   hwmon_temp_min_alarm, 0);
>  	if (st & LM90_STATUS_RLOW)
>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> -				   hwmon_temp_min, 1);
> +				   hwmon_temp_min_alarm, 1);
>  	if (st2 & MAX6696_STATUS2_R2LOW)
>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> -				   hwmon_temp_min, 2);
> +				   hwmon_temp_min_alarm, 2);
>  	if (st & LM90_STATUS_LHIGH)
>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> -				   hwmon_temp_max, 0);
> +				   hwmon_temp_max_alarm, 0);
>  	if (st & LM90_STATUS_RHIGH)
>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> -				   hwmon_temp_max, 1);
> +				   hwmon_temp_max_alarm, 1);
>  	if (st2 & MAX6696_STATUS2_R2HIGH)
>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> -				   hwmon_temp_max, 2);
> +				   hwmon_temp_max_alarm, 2);


IIUC, "alarm" is about the T_CRIT output line. While these attributes
are about the ALERT line. Hence why "alert" notifications need to be
sent to the unrelated "alarm" attributes? This change doesn't look right.

Re: [PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Dmitry Osipenko]

15.01.2022 23:33, Dmitry Osipenko пишет:
> 11.01.2022 19:51, Guenter Roeck пишет:
>> sysfs and udev notifications need to be sent to the _alarm
>> attributes, not to the value attributes.
>>
>> Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
>> Cc: Dmitry Osipenko <digetx@gmail.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  drivers/hwmon/lm90.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index ba01127c1deb..1c9493c70813 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
>>  
>>  	if (st & LM90_STATUS_LLOW)
>>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> -				   hwmon_temp_min, 0);
>> +				   hwmon_temp_min_alarm, 0);
>>  	if (st & LM90_STATUS_RLOW)
>>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> -				   hwmon_temp_min, 1);
>> +				   hwmon_temp_min_alarm, 1);
>>  	if (st2 & MAX6696_STATUS2_R2LOW)
>>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> -				   hwmon_temp_min, 2);
>> +				   hwmon_temp_min_alarm, 2);
>>  	if (st & LM90_STATUS_LHIGH)
>>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> -				   hwmon_temp_max, 0);
>> +				   hwmon_temp_max_alarm, 0);
>>  	if (st & LM90_STATUS_RHIGH)
>>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> -				   hwmon_temp_max, 1);
>> +				   hwmon_temp_max_alarm, 1);
>>  	if (st2 & MAX6696_STATUS2_R2HIGH)
>>  		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> -				   hwmon_temp_max, 2);
>> +				   hwmon_temp_max_alarm, 2);
> 
> 
> IIUC, "alarm" is about the T_CRIT output line. While these attributes
> are about the ALERT line. Hence why "alert" notifications need to be
> sent to the unrelated "alarm" attributes? This change doesn't look right.
> 

Although, no. I see now that the "alarm_bits" in the code are about the
alerts. Should be okay then.

Re: [PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Guenter Roeck]

On 1/15/22 12:41 PM, Dmitry Osipenko wrote:
> 15.01.2022 23:33, Dmitry Osipenko пишет:
>> 11.01.2022 19:51, Guenter Roeck пишет:
>>> sysfs and udev notifications need to be sent to the _alarm
>>> attributes, not to the value attributes.
>>>
>>> Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   drivers/hwmon/lm90.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index ba01127c1deb..1c9493c70813 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
>>>   
>>>   	if (st & LM90_STATUS_LLOW)
>>>   		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> -				   hwmon_temp_min, 0);
>>> +				   hwmon_temp_min_alarm, 0);
>>>   	if (st & LM90_STATUS_RLOW)
>>>   		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> -				   hwmon_temp_min, 1);
>>> +				   hwmon_temp_min_alarm, 1);
>>>   	if (st2 & MAX6696_STATUS2_R2LOW)
>>>   		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> -				   hwmon_temp_min, 2);
>>> +				   hwmon_temp_min_alarm, 2);
>>>   	if (st & LM90_STATUS_LHIGH)
>>>   		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> -				   hwmon_temp_max, 0);
>>> +				   hwmon_temp_max_alarm, 0);
>>>   	if (st & LM90_STATUS_RHIGH)
>>>   		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> -				   hwmon_temp_max, 1);
>>> +				   hwmon_temp_max_alarm, 1);
>>>   	if (st2 & MAX6696_STATUS2_R2HIGH)
>>>   		hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> -				   hwmon_temp_max, 2);
>>> +				   hwmon_temp_max_alarm, 2);
>>
>>
>> IIUC, "alarm" is about the T_CRIT output line. While these attributes
>> are about the ALERT line. Hence why "alert" notifications need to be
>> sent to the unrelated "alarm" attributes? This change doesn't look right.
>>
> 
> Although, no. I see now that the "alarm_bits" in the code are about the
> alerts. Should be okay then.
> 

Alarm attributes are really neither about interrupts nor about alerts.
Alarm attributes alert userspace that a limit has been exceeded.
How and if the driver notices that a limit has been exceeded is
an implementation detail. In a specific implementation, alerts
or interrupts can be used to notify a driver that a notable event
has occurred on a given device, but technically that is not
necessary. Polling would do just as well.

Guenter

Re: [PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Dmitry Osipenko]

16.01.2022 00:11, Guenter Roeck пишет:
> On 1/15/22 12:41 PM, Dmitry Osipenko wrote:
>> 15.01.2022 23:33, Dmitry Osipenko пишет:
>>> 11.01.2022 19:51, Guenter Roeck пишет:
>>>> sysfs and udev notifications need to be sent to the _alarm
>>>> attributes, not to the value attributes.
>>>>
>>>> Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>   drivers/hwmon/lm90.c | 12 ++++++------
>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>> index ba01127c1deb..1c9493c70813 100644
>>>> --- a/drivers/hwmon/lm90.c
>>>> +++ b/drivers/hwmon/lm90.c
>>>> @@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct
>>>> i2c_client *client, u16 *status)
>>>>         if (st & LM90_STATUS_LLOW)
>>>>           hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> -                   hwmon_temp_min, 0);
>>>> +                   hwmon_temp_min_alarm, 0);
>>>>       if (st & LM90_STATUS_RLOW)
>>>>           hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> -                   hwmon_temp_min, 1);
>>>> +                   hwmon_temp_min_alarm, 1);
>>>>       if (st2 & MAX6696_STATUS2_R2LOW)
>>>>           hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> -                   hwmon_temp_min, 2);
>>>> +                   hwmon_temp_min_alarm, 2);
>>>>       if (st & LM90_STATUS_LHIGH)
>>>>           hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> -                   hwmon_temp_max, 0);
>>>> +                   hwmon_temp_max_alarm, 0);
>>>>       if (st & LM90_STATUS_RHIGH)
>>>>           hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> -                   hwmon_temp_max, 1);
>>>> +                   hwmon_temp_max_alarm, 1);
>>>>       if (st2 & MAX6696_STATUS2_R2HIGH)
>>>>           hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> -                   hwmon_temp_max, 2);
>>>> +                   hwmon_temp_max_alarm, 2);
>>>
>>>
>>> IIUC, "alarm" is about the T_CRIT output line. While these attributes
>>> are about the ALERT line. Hence why "alert" notifications need to be
>>> sent to the unrelated "alarm" attributes? This change doesn't look
>>> right.
>>>
>>
>> Although, no. I see now that the "alarm_bits" in the code are about the
>> alerts. Should be okay then.
>>
> 
> Alarm attributes are really neither about interrupts nor about alerts.
> Alarm attributes alert userspace that a limit has been exceeded.
> How and if the driver notices that a limit has been exceeded is
> an implementation detail. In a specific implementation, alerts
> or interrupts can be used to notify a driver that a notable event
> has occurred on a given device, but technically that is not
> necessary. Polling would do just as well.

Datasheet refers to T_CRIT using the "alarm" term, this confused me a tad.

BTW, we don't have events wired for the temp_crit_alarm attribute.

Re: [PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Guenter Roeck]

On 1/16/22 12:14 AM, Dmitry Osipenko wrote:
> 16.01.2022 00:11, Guenter Roeck пишет:
>> On 1/15/22 12:41 PM, Dmitry Osipenko wrote:
>>> 15.01.2022 23:33, Dmitry Osipenko пишет:
>>>> 11.01.2022 19:51, Guenter Roeck пишет:
>>>>> sysfs and udev notifications need to be sent to the _alarm
>>>>> attributes, not to the value attributes.
>>>>>
>>>>> Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>> ---
>>>>>    drivers/hwmon/lm90.c | 12 ++++++------
>>>>>    1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>>> index ba01127c1deb..1c9493c70813 100644
>>>>> --- a/drivers/hwmon/lm90.c
>>>>> +++ b/drivers/hwmon/lm90.c
>>>>> @@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct
>>>>> i2c_client *client, u16 *status)
>>>>>          if (st & LM90_STATUS_LLOW)
>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> -                   hwmon_temp_min, 0);
>>>>> +                   hwmon_temp_min_alarm, 0);
>>>>>        if (st & LM90_STATUS_RLOW)
>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> -                   hwmon_temp_min, 1);
>>>>> +                   hwmon_temp_min_alarm, 1);
>>>>>        if (st2 & MAX6696_STATUS2_R2LOW)
>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> -                   hwmon_temp_min, 2);
>>>>> +                   hwmon_temp_min_alarm, 2);
>>>>>        if (st & LM90_STATUS_LHIGH)
>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> -                   hwmon_temp_max, 0);
>>>>> +                   hwmon_temp_max_alarm, 0);
>>>>>        if (st & LM90_STATUS_RHIGH)
>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> -                   hwmon_temp_max, 1);
>>>>> +                   hwmon_temp_max_alarm, 1);
>>>>>        if (st2 & MAX6696_STATUS2_R2HIGH)
>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> -                   hwmon_temp_max, 2);
>>>>> +                   hwmon_temp_max_alarm, 2);
>>>>
>>>>
>>>> IIUC, "alarm" is about the T_CRIT output line. While these attributes
>>>> are about the ALERT line. Hence why "alert" notifications need to be
>>>> sent to the unrelated "alarm" attributes? This change doesn't look
>>>> right.
>>>>
>>>
>>> Although, no. I see now that the "alarm_bits" in the code are about the
>>> alerts. Should be okay then.
>>>
>>
>> Alarm attributes are really neither about interrupts nor about alerts.
>> Alarm attributes alert userspace that a limit has been exceeded.
>> How and if the driver notices that a limit has been exceeded is
>> an implementation detail. In a specific implementation, alerts
>> or interrupts can be used to notify a driver that a notable event
>> has occurred on a given device, but technically that is not
>> necessary. Polling would do just as well.
> 
> Datasheet refers to T_CRIT using the "alarm" term, this confused me a tad.
> 
> BTW, we don't have events wired for the temp_crit_alarm attribute.
> 
And not for for emergency alarms either. I have a patch prepared to address
this, as part of a larger patch series. Unlike the patches in this series,
I considered adding events for those attributes an enhancement and not
a bug fix.

Guenter

Re: [PATCH 6/6] hwmon: (lm90) Fix sysfs and udev notifications

[Dmitry Osipenko]

16.01.2022 18:58, Guenter Roeck пишет:
> On 1/16/22 12:14 AM, Dmitry Osipenko wrote:
>> 16.01.2022 00:11, Guenter Roeck пишет:
>>> On 1/15/22 12:41 PM, Dmitry Osipenko wrote:
>>>> 15.01.2022 23:33, Dmitry Osipenko пишет:
>>>>> 11.01.2022 19:51, Guenter Roeck пишет:
>>>>>> sysfs and udev notifications need to be sent to the _alarm
>>>>>> attributes, not to the value attributes.
>>>>>>
>>>>>> Fixes: 94dbd23ed88c ("hwmon: (lm90) Use hwmon_notify_event()")
>>>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>> ---
>>>>>>    drivers/hwmon/lm90.c | 12 ++++++------
>>>>>>    1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>>>> index ba01127c1deb..1c9493c70813 100644
>>>>>> --- a/drivers/hwmon/lm90.c
>>>>>> +++ b/drivers/hwmon/lm90.c
>>>>>> @@ -1808,22 +1808,22 @@ static bool lm90_is_tripped(struct
>>>>>> i2c_client *client, u16 *status)
>>>>>>          if (st & LM90_STATUS_LLOW)
>>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> -                   hwmon_temp_min, 0);
>>>>>> +                   hwmon_temp_min_alarm, 0);
>>>>>>        if (st & LM90_STATUS_RLOW)
>>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> -                   hwmon_temp_min, 1);
>>>>>> +                   hwmon_temp_min_alarm, 1);
>>>>>>        if (st2 & MAX6696_STATUS2_R2LOW)
>>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> -                   hwmon_temp_min, 2);
>>>>>> +                   hwmon_temp_min_alarm, 2);
>>>>>>        if (st & LM90_STATUS_LHIGH)
>>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> -                   hwmon_temp_max, 0);
>>>>>> +                   hwmon_temp_max_alarm, 0);
>>>>>>        if (st & LM90_STATUS_RHIGH)
>>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> -                   hwmon_temp_max, 1);
>>>>>> +                   hwmon_temp_max_alarm, 1);
>>>>>>        if (st2 & MAX6696_STATUS2_R2HIGH)
>>>>>>            hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> -                   hwmon_temp_max, 2);
>>>>>> +                   hwmon_temp_max_alarm, 2);
>>>>>
>>>>>
>>>>> IIUC, "alarm" is about the T_CRIT output line. While these attributes
>>>>> are about the ALERT line. Hence why "alert" notifications need to be
>>>>> sent to the unrelated "alarm" attributes? This change doesn't look
>>>>> right.
>>>>>
>>>>
>>>> Although, no. I see now that the "alarm_bits" in the code are about the
>>>> alerts. Should be okay then.
>>>>
>>>
>>> Alarm attributes are really neither about interrupts nor about alerts.
>>> Alarm attributes alert userspace that a limit has been exceeded.
>>> How and if the driver notices that a limit has been exceeded is
>>> an implementation detail. In a specific implementation, alerts
>>> or interrupts can be used to notify a driver that a notable event
>>> has occurred on a given device, but technically that is not
>>> necessary. Polling would do just as well.
>>
>> Datasheet refers to T_CRIT using the "alarm" term, this confused me a
>> tad.
>>
>> BTW, we don't have events wired for the temp_crit_alarm attribute.
>>
> And not for for emergency alarms either. I have a patch prepared to address
> this, as part of a larger patch series. Unlike the patches in this series,
> I considered adding events for those attributes an enhancement and not
> a bug fix.

Alright