Messages in this thread | | | Date | Thu, 17 Mar 2022 06:28:53 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH 2/2] hwmon: (adt7475) Add support for pin configuration |
| |
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))
| |