Re: [PATCH 2/2] hwmon: (adt7475) Add support for pin configuration

[Guenter Roeck <linux@roeck-us.net>]

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@alliedtelesis.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))
>>