[PATCH 0/2] hwmon: (adt7475) pin configuration

[PATCH 0/2] hwmon: (adt7475) pin configuration

[Chris Packham]

This series adds support for configuring the ADT7475 based on the
hardware description provided. The ADT7475 driver has always supported
detecting the configuration. This adds support for configuring the chip
based on the hardware design.

Chris Packham (2):
  dt-bindings: hwmon: Document adt7475 pin-function properties
  hwmon: (adt7475) Add support for pin configuration

 .../devicetree/bindings/hwmon/adt7475.yaml    | 22 +++++
 drivers/hwmon/adt7475.c                       | 95 +++++++++++++++++++
 2 files changed, 117 insertions(+)

-- 
2.35.1

[PATCH 1/2] dt-bindings: hwmon: Document adt7475 pin-function properties

[Chris Packham]

The adt7473, adt7475, adt7476 and adt7490 have pins that can be used for
different functions. Add bindings so that it is possible to describe
what pin functions are intended by the hardware design.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 .../devicetree/bindings/hwmon/adt7475.yaml    | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
index 7d9c083632b9..22beb37f1bf1 100644
--- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -61,6 +61,26 @@ patternProperties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [0, 1]
 
+  "adi,pin(5|10)-function":
+    description: |
+      Configures the function for pin 5 on the adi,adt7473 and adi,adt7475. Or
+      pin 10 on the adi,adt7476 and adi,adt7490.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - pwm2
+      - smbalert#
+
+  "adi,pin(9|14)-function":
+    description: |
+      Configures the function for pin 9 on the adi,adt7473 and adi,adt7475. Or
+      pin 14 on the adi,adt7476 and adi,adt7490
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - tach4
+      - therm#
+      - smbalert#
+      - gpio
+
 required:
   - compatible
   - reg
@@ -79,6 +99,8 @@ examples:
         adi,bypass-attenuator-in0 = <1>;
         adi,bypass-attenuator-in1 = <0>;
         adi,pwm-active-state = <1 0 1>;
+        adi,pin10-function = "smbalert#";
+        adi,pin14-function = "tach4";
       };
     };
 
-- 
2.35.1

Re: [PATCH 1/2] dt-bindings: hwmon: Document adt7475 pin-function properties

[Krzysztof Kozlowski]

On 17/03/2022 00:41, Chris Packham wrote:
> The adt7473, adt7475, adt7476 and adt7490 have pins that can be used for
> different functions. Add bindings so that it is possible to describe
> what pin functions are intended by the hardware design.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  .../devicetree/bindings/hwmon/adt7475.yaml    | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof

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

[Chris Packham]

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)
+{
+	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);
+
+		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)
+{
+	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);
+		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;
+
+	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;
+	}
+
+	if (chip != adt7476 && chip != adt7490)
+		return 0;
+
+	err = load_pin10_config(client, conf_prop1);
+	if (err) {
+		dev_err(&client->dev, "failed to configure PIN10\n");
+		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))
-- 
2.35.1

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

[Guenter Roeck]

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.

> +	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).

> +
> +		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.

> +	}
> +
> +	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.

> +	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.

> +		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))

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

[Chris Packham]

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 (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))
>

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

[Guenter Roeck]

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))
>>