linux-kernel - Re: [PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c18f6ed-2736-4675-9ed3-042491da01ad@roeck-us.net>
Date: Thu, 30 May 2024 06:18:45 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Amna Waseem <Amna.Waseem@...s.com>, Jean Delvare <jdelvare@...e.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, linux-hwmon@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, kernel@...s.com
Subject: Re: [PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert
 polarity

On 5/30/24 01:06, Amna Waseem wrote:
> On 5/29/24 16:07, Guenter Roeck wrote:
>> On 5/28/24 23:07, Amna Waseem wrote:
>>> The INA230 has an Alert pin which is asserted when the alert
>>> function selected in the Mask/Enable register exceeds the
>>> value programmed into the Alert Limit register. Assertion is based
>>> on the Alert Polarity Bit (APOL, bit 1 of the Mask/Enable register).
>>> It is default set to value 0 i.e Normal (active-low open collector).
>>> However, hardware can be designed in such a way that expects Alert pin
>>> to become active high if a user-defined threshold in Alert limit
>>> register has been exceeded. This patch adds a way to pass alert polarity
>>> value to the driver via device tree.
>>>
>>> Signed-off-by: Amna Waseem <Amna.Waseem@...s.com>
>>> ---
>>>   drivers/hwmon/ina2xx.c | 28 ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>>> index d8415d1f21fc..b58e795bdc8f 100644
>>> --- a/drivers/hwmon/ina2xx.c
>>> +++ b/drivers/hwmon/ina2xx.c
>>> @@ -73,6 +73,9 @@
>>>   #define INA226_READ_AVG(reg)        (((reg) & INA226_AVG_RD_MASK) >> 9)
>>>   #define INA226_SHIFT_AVG(val)        ((val) << 9)
>>>   +#define INA226_ALERT_POLARITY_MASK        0x0002
>>> +#define INA226_SHIFT_ALERT_POLARITY(val)    ((val) << 1)
>>> +
>>>   /* bit number of alert functions in Mask/Enable Register */
>>>   #define INA226_SHUNT_OVER_VOLTAGE_BIT    15
>>>   #define INA226_SHUNT_UNDER_VOLTAGE_BIT    14
>>> @@ -178,6 +181,23 @@ static u16 ina226_interval_to_reg(int interval)
>>>       return INA226_SHIFT_AVG(avg_bits);
>>>   }
>>>   +static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
>>> +                     unsigned long val)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (val > INT_MAX || !(val == 0 || val == 1))
>>
>>     if (val != 0 && val !=1)
>>
>> would be sufficient and much easier to understand.
> 
> 
> Agreed.
> 
>>
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&data->config_lock);
>>
>> Pointless lock.
>>
>>> +    ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
>>> +                 INA226_ALERT_POLARITY_MASK,
>>> +                 INA226_SHIFT_ALERT_POLARITY(val));
>>> +
>>> +    mutex_unlock(&data->config_lock);
>>> +    return ret;
>>> +}
>>> +
>>>   /*
>>>    * Calibration register is set to the best value, which eliminates
>>>    * truncation errors on calculating current register in hardware.
>>> @@ -659,6 +679,14 @@ static int ina2xx_probe(struct i2c_client *client)
>>>       if (ret)
>>>           return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
>>>   +    if (!of_property_read_u32(dev->of_node, "alert-polarity", &val)) {
>>> +        ret = ina2xx_set_alert_polarity(data, val);
>>> +        if (ret < 0)
>>> +            return dev_err_probe(
>>> +                dev, ret,
>>> +                "failed to set APOL bit of Enable/Mask register\n");
>>> +    }
>>
>> INA219 and INA220 do not support alert pin configuration (or, naturally,
>> the mask register in the first place). This will need to be validated.
>>
>> Guenter
>>
> Would "of_property_read_bool" be sufficient to check whether the property exists or not for different chips? It means that if INA219 and INA220 are being used, they will not have a property "alert-polarity" defined in their devicetree so of_property_read_bool will return false and nothing will happen for these chips.
> 

No, that would not be sufficient, because the non-existence of the property
also has a meaning and still should be used to set the polarity.

You have two options: Not evaluate the property in the first place if
the chip doesn't support it, or return an error if the property exists
but the chip does not support it. Personally I'd go the easy route;
something like

	if (chip supports it) {
		ret = ina2xx_set_alert_polarity(dev, regmap);
		if (ret)
			return dev_err_probe(...);
	}

and evaluate the property in ina2xx_set_alert_polarity().

Guenter