RE: [PATCH v5 2/4] hwmon: max31827: Add support for max31828 and max31829

[Matyas, Daniel]

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