| 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
| ||
|
Message-ID: <cbc028bd-8b4f-5cc1-3bcf-a195ae7cebd9@roeck-us.net>
Date: Thu, 17 Mar 2022 15:01:07 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Chris Packham <Chris.Packham@...iedtelesis.co.nz>,
"jdelvare@...e.com" <jdelvare@...e.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>
Cc: "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] hwmon: (adt7475) Add support for pin configuration
On 3/17/22 14:35, Chris Packham wrote:
>
> On 18/03/22 02:28, Guenter Roeck wrote:
>> On 3/16/22 16:41, Chris Packham wrote:
>>> The adt7473, adt7475, adt7476 and adt7490 have pins that can be used for
>>> different functions. On the adt7473 and adt7475 this is pins 5 and 9.
>>> On the adt7476 and adt7490 this is pins 10 and 14.
>>>
>>> The first pin can either be PWM2(default) or SMBALERT#. The second pin
>>> can be TACH4(default), THERM#, SMBALERT# or GPIO.
>>>
>>> The adt7475 driver has always been able to detect the configuration if
>>> it had been done by an earlier boot stage. Add support for configuring
>>> the pins based on the hardware description in the device tree.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>>> ---
>>> drivers/hwmon/adt7475.c | 95 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 95 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
>>> index 9d5b019651f2..ad5e5a7a844b 100644
>>> --- a/drivers/hwmon/adt7475.c
>>> +++ b/drivers/hwmon/adt7475.c
>>> @@ -112,6 +112,8 @@
>>> #define CONFIG3_THERM 0x02
>>> #define CONFIG4_PINFUNC 0x03
>>> +#define CONFIG4_THERM 0x01
>>> +#define CONFIG4_SMBALERT 0x02
>>> #define CONFIG4_MAXDUTY 0x08
>>> #define CONFIG4_ATTN_IN10 0x30
>>> #define CONFIG4_ATTN_IN43 0xC0
>>> @@ -1460,6 +1462,95 @@ static int adt7475_update_limits(struct
>>> i2c_client *client)
>>> return 0;
>>> }
>>> +static int load_pin10_config(const struct i2c_client *client,
>>> const char *propname)
>>> +{
>>
>> A better function name would probably be load_config3() or similar.
>
> Yep that'd be a better name.
>
>>
>>> + const char *function;
>>> + u8 config3;
>>> + int err;
>>> +
>>> + err = of_property_read_string(client->dev.of_node, propname,
>>> &function);
>>> + if (!err) {
>>> + config3 = adt7475_read(REG_CONFIG3);
>>
>> error check missing (I see the driver is notorious for that, but that
>> is not
>> a reason to keep doing it).
>
> Ikegami-san and Dan did to some good work to address some of that. The
> probe function is still quite careless.
>
> I'll see what I can do to make sure my additions don't make it worse.
>>
>>> +
>>> + if (!strcmp("pwm2", function))
>>> + config3 &= ~CONFIG3_SMBALERT;
>>> + else if (!strcmp("smbalert#", function))
>>> + config3 |= CONFIG3_SMBALERT;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + return i2c_smbus_write_byte_data(client, REG_CONFIG3, config3);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int load_pin14_config(const struct i2c_client *client, const
>>> char *propname)
>>> +{
>>
>> load_config4() ?
>>
>>> + const char *function;
>>> + u8 config4;
>>> + int err;
>>> +
>>> + err = of_property_read_string(client->dev.of_node, propname,
>>> &function);
>>> + if (!err) {
>>> + config4 = adt7475_read(REG_CONFIG4);
>>
>> error check
>>
>>> + config4 &= ~CONFIG4_PINFUNC;
>>> +
>>> + if (!strcmp("tach4", function))
>>> + ;
>>> + else if (!strcmp("therm#", function))
>>> + config4 |= CONFIG4_THERM;
>>> + else if (!strcmp("smbalert#", function))
>>> + config4 |= CONFIG4_SMBALERT;
>>> + else if (!strcmp("gpio", function))
>>> + config4 |= CONFIG4_PINFUNC;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + return i2c_smbus_write_byte_data(client, REG_CONFIG4, config4);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int load_config(const struct i2c_client *client, int chip)
>>> +{
>>> + int err;
>>> + const char *conf_prop1, *conf_prop2;
>>
>> conf_ prefix is unnecessary.
>>
>>> +
>>> + switch (chip) {
>>> + case adt7473:
>>> + case adt7475:
>>> + conf_prop1 = "adi,pin5-function";
>>> + conf_prop2 = "adi,pin9-function";
>>> + break;
>>> + case adt7476:
>>> + case adt7490:
>>> + conf_prop1 = "adi,pin10-function";
>>> + conf_prop2 = "adi,pin14-function";
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>
>> It doesn't seem right to return -EINVAL here.
>>
> Have you got a better suggestion? I was trying to avoid someone
> specifying compatible = "adi,adt7476" with "adi,pin5-function". Is your
> concern that I should use -ENODEV or that I should just pick more
> generic names for the configurable pins (naming things is hard).
>
> Or perhaps just dev_warn() and return 0?
>
If you use "enum chips" as function parameter you should not need
a default: case. Otherwise -EINVAL is fine _if_ the code below is
removed. I didn't understand what you wanted to accomplish by
returning 0 for known (but unsupported) devices but -EINVAL for
non-existing ones.
Guenter
>>> + }
>>> +
>>> + if (chip != adt7476 && chip != adt7490)
>>> + return 0;
>>> +
>>
>> Why not check this first, and what is the point of assigning values to
>> conf_prop1 and conf_prop2 for the other chips in the case statement above
>> only to return 0 here ? It would be much simpler to drop the other chips
>> from the case statement and have default: return 0.
>>
> Sorry that is old. I initially was under the impression that only these
> 2 had configurable pins but then I read the other datasheets more closely.
>>> + err = load_pin10_config(client, conf_prop1);
>>> + if (err) {
>>> + dev_err(&client->dev, "failed to configure PIN10\n");
>>
>> The messages are misleading. This isn't always pin 10/14.
>>
> Now I've got the prop names I can use that instead.
>>> + return err;
>>> + }
>>> +
>>> + err = load_pin14_config(client, conf_prop2);
>>> + if (err) {
>>> + dev_err(&client->dev, "failed to configure PIN14\n");
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int set_property_bit(const struct i2c_client *client, char
>>> *property,
>>> u8 *config, u8 bit_index)
>>> {
>>> @@ -1585,6 +1676,10 @@ static int adt7475_probe(struct i2c_client
>>> *client)
>>> revision = adt7475_read(REG_DEVID2) & 0x07;
>>> }
>>> + ret = load_config(client, chip);
>>> + if (ret)
>>> + return ret;
>>> +
>>> config3 = adt7475_read(REG_CONFIG3);
>>> /* Pin PWM2 may alternatively be used for ALERT output */
>>> if (!(config3 & CONFIG3_SMBALERT))
>>
Powered by blists - more mailing lists