-----Original Message-----
From: Matyas, Daniel
Sent: Friday, October 27, 2023 4:01 PM
To: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: no To-header on input <;>; Jean Delvare <jdelvare@xxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: RE: [PATCH v5 2/4] hwmon: max31827: Add support for max31828 and max31829
> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 8:02 AM
> To: Matyas, Daniel <Daniel.Matyas@xxxxxxxxxx>
> Cc: no To-header on input <;>; Jean Delvare <jdelvare@xxxxxxxx>;
> Jonathan Corbet <corbet@xxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 2/4] hwmon: max31827: Add support for
> max31828 and max31829
>
> [External]
>
> On Thu, Oct 26, 2023 at 05:44:02PM +0300, Daniel Matyas wrote:
> > When adi,flt-q and/or adi,alrm-pol are not mentioned, the default
> > configuration is loaded.
> >
> That isn't really a complete patch description.
> It should include (even if repeated) that support for additional chips
> is added.
>
> > Signed-off-by: Daniel Matyas <daniel.matyas@xxxxxxxxxx>
> > ---
> >
> > v4 -> v5: Passed i2c_client to init_client(), because I needed it to
> > retrieve device id.
> > Used a simple if to load default configuration. No more switch.
> >
> > v3 -> v4: No change.
> >
> > v3: Added patch.
> >
> > drivers/hwmon/max31827.c | 51
> > +++++++++++++++++++++++++++++++---------
> > 1 file changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index
> > 7976d668ffd4..446232fa1710 100644
> > --- a/drivers/hwmon/max31827.c
> > +++ b/drivers/hwmon/max31827.c
> > @@ -39,10 +39,15 @@
> >
> > #define MAX31827_12_BIT_CNV_TIME 140
> >
> > +#define MAX31827_ALRM_POL_HIGH 0x1
> > +#define MAX31827_FLT_Q_4 0x2
> > +
> > #define MAX31827_16_BIT_TO_M_DGR(x) (sign_extend32(x, 15) *
> 1000 / 16)
> > #define MAX31827_M_DGR_TO_16_BIT(x) (((x) << 4) / 1000)
> > #define MAX31827_DEVICE_ENABLE(x) ((x) ? 0xA : 0x0)
> >
> > +enum chips { max31827, max31828, max31829 };
> > +
> > enum max31827_cnv {
> > MAX31827_CNV_1_DIV_64_HZ = 1,
> > MAX31827_CNV_1_DIV_32_HZ,
> > @@ -373,12 +378,22 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> > return -EOPNOTSUPP;
> > }
> >
> > +static const struct i2c_device_id max31827_i2c_ids[] = {
> > + { "max31827", max31827 },
> > + { "max31828", max31828 },
> > + { "max31829", max31829 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> > +
> > static int max31827_init_client(struct max31827_state *st,
> > - struct device *dev)
> > + struct i2c_client *client)
> > {
> > + struct device *dev = &client->dev;
> > struct fwnode_handle *fwnode;
> > unsigned int res = 0;
> > u32 data, lsb_idx;
> > + enum chips type;
> > bool prop;
> > int ret;
> >
> > @@ -395,13 +410,20 @@ static int max31827_init_client(struct
> max31827_state *st,
> > prop = fwnode_property_read_bool(fwnode, "adi,timeout-
> enable");
> > res |=
> FIELD_PREP(MAX31827_CONFIGURATION_TIMEOUT_MASK, !prop);
> >
> > + if (dev->of_node)
> > + type = (enum chips)of_device_get_match_data(dev);
> > + else
> > + type = i2c_match_id(max31827_i2c_ids, client)-
> >driver_data;
> > +
>
> This should be something like
>
> type = (enum chips)(uintptr_t)device_get_match_data(dev);
>
> though that requires that the enum values start with 1. This avoids
> having to pass client to the function and is more generic.
>
> > if (fwnode_property_present(fwnode, "adi,alarm-pol")) {
> > ret = fwnode_property_read_u32(fwnode, "adi,alarm-
> pol", &data);
> > if (ret)
> > return ret;
> >
> > res |=
> FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> > - }
> > + } else if (type == max31829)
>
> Not sure why checkpatch ignores this (maybe because of 'else if'), but
> the else path should also be in {}.
>
> But why is this only for max31829 ? I mean, sure, the default for that
> chip is different, but that doesn't mean the other chips have that bit
> set. There is no guarantee that the chip is in its default state when
> the driver is loaded.
>
> > + res |=
> FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> > + MAX31827_ALRM_POL_HIGH);
> >
> > if (fwnode_property_present(fwnode, "adi,fault-q")) {
> > ret = fwnode_property_read_u32(fwnode, "adi,fault-q",
> &data); @@
> > -418,7 +440,9 @@ static int max31827_init_client(struct
> max31827_state *st,
> > }
> >
> > res |=
> FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> > - }
> > + } else if ((type == max31828) || (type == max31829))
>
> I _really_ dislike the notion of excessive ( ). Also, {} for the else branch.
>
> I also don't understand why that would be chip specific. I don't see
> anything along that line in the datasheet.
>
> Ah, wait ... I guess that is supposed to reflect the chip default.
> I don't see why the chip default makes a difference - a well defined
> default must be set either way. Again, there is no guarantee that the
> chip is in its default state when the driver is loaded.
The well defined default was set in v4, but I deleted it, because the default value in hex for max31827 and max31828 alarm polarity, and max31827 fault queue is 0x0. I had 2 #defines for these values, but you said:
" Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do anything and just pollutes the code. "
So, I thought I should remove it altogether, since res is set to 0 in the beginning and the default value of these chips (i.e. 0) is implicitly set.
>
> Also, why are the default values added in this patch and not in the
> previous patch ?
>
In v4 these default values were set in the previous patch.
> > + res |=
> FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> > + MAX31827_FLT_Q_4);
> >
> > return regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, res); }
> > @@ -464,7 +488,7 @@ static int max31827_probe(struct i2c_client
> *client)
> > return dev_err_probe(dev, PTR_ERR(st->regmap),
> > "Failed to allocate regmap.\n");
> >
> > - err = max31827_init_client(st, dev);
> > + err = max31827_init_client(st, client);
> > if (err)
> > return err;
> >
> > @@ -475,14 +499,19 @@ static int max31827_probe(struct i2c_client
> *client)
> > return PTR_ERR_OR_ZERO(hwmon_dev); }
> >
> > -static const struct i2c_device_id max31827_i2c_ids[] = {
> > - { "max31827", 0 },
> > - { }
> > -};
> > -MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> > -
> > static const struct of_device_id max31827_of_match[] = {
> > - { .compatible = "adi,max31827" },
> > + {
> > + .compatible = "adi,max31827",
> > + .data = (void *)max31827
> > + },
> > + {
> > + .compatible = "adi,max31828",
> > + .data = (void *)max31828
> > + },
> > + {
> > + .compatible = "adi,max31829",
> > + .data = (void *)max31829
> > + },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, max31827_of_match);
