RE: [PATCH 1/3] hwmon: max31827: Add PEC support

From: "Matyas, Daniel" <Daniel.Matyas@analog.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: RE: [PATCH 1/3] hwmon: max31827: Add PEC support
Date: Fri, 15 Dec 2023 20:28:07 +0000	[thread overview]
Message-ID: <PH0PR03MB6771AD7164ABEEB02650CA908993A@PH0PR03MB6771.namprd03.prod.outlook.com> (raw)
In-Reply-To: <2e0bf1cf-824d-40c6-9450-7ed4740f2f46@roeck-us.net>

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, December 14, 2023 6:10 PM
> To: Matyas, Daniel <Daniel.Matyas@analog.com>
> Cc: Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; linux-
> hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH 1/3] hwmon: max31827: Add PEC support
> 
> [External]
> 
> On 12/14/23 06:36, Daniel Matyas wrote:
> > Removed regmap and used my functions to read, write and update bits.
> > In these functions i2c_smbus_ helper functions are used. These check
> > if there were any PEC errors during read. In the write function, if
> > PEC is enabled, I check for PEC Error bit, to see if there were any errors.
> >
> > Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> 
> The "PEC" attribute needs to be attached to the I2C device.
> See lm90.c or pmbus_core.c for examples.
> 

I added pec_show() and pec_store() functions and created the pec file within the max31827_groups.
I did not set the flags, because I want them to be set only in pec_store. By default the PEC flag should not be set.

> The changes, if indeed needed, do not warant dropping regmap.
> You will need to explain why the reg_read and reg_write callbacks
> provideed by regmap can not be used.
> 
> On top of that, it is not clear why regmap can't be used in the first place.
> It seems that the major change is that one needs to read the configuration
> register after a write to see if there was a PEC error. It is not immediately
> obvious why that additional read (if indeed necessary) would require
> regmap support to be dropped.
> 

So I am not sure about this, but it seems to me, that regmap_write is not sending a PEC byte and regmap_read is not checking the PEC byte by default. My rationale was: i2c_smbus_write_word_data() is using the i2c_xfer function, which has as argument the client->flags. So, if I2C_CLIENT_PEC is set in client->flags, that would be transmitted by i2c_xfer. I am not convinced about this, but lm90 and pmbus_core are not using regmap, so I went ahead and replaced it.

If what I am thinking is wrong, please correct me.

> Regarding "if indeed necessary": There needs to be a comment explaining
> that the device will return ACK even after a packet with bad PEC is
> received.
> 
> > ---
> >   Documentation/hwmon/max31827.rst |  14 +-
> >   drivers/hwmon/max31827.c         | 219 +++++++++++++++++++++++-----
> ---
> >   2 files changed, 171 insertions(+), 62 deletions(-)
> >
> > diff --git a/Documentation/hwmon/max31827.rst
> > b/Documentation/hwmon/max31827.rst
> > index 44ab9dc064cb..ecbc1ddba6a7 100644
> > --- a/Documentation/hwmon/max31827.rst
> > +++ b/Documentation/hwmon/max31827.rst
> > @@ -131,7 +131,13 @@ The Fault Queue bits select how many
> consecutive temperature faults must occur
> >   before overtemperature or undertemperature faults are indicated in
> the
> >   corresponding status bits.
> >
> > -Notes
> > ------
> > -
> > -PEC is not implemented.
> > +PEC (packet error checking) can be enabled from the "pec" device
> attribute.
> > +If PEC is enabled, a PEC byte is appended to the end of each message
> transfer.
> > +This is a CRC-8 byte that is calculated on all of the message bytes
> > +(including the address/read/write byte). The last device to transmit
> > +a data byte also transmits the PEC byte. The master transmits the PEC
> > +byte after a write transaction, and the MAX31827 transmits the PEC
> byte after a read transaction.
> > +
> > +The read PEC error is handled inside the
> i2c_smbus_read_word_swapped() function.
> > +To check if the write had any PEC error a read is performed on the
> > +configuration register, to check the PEC Error bit.
> > \ No newline at end of file
> > diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index
> > 71ad3934dfb6..db93492193bd 100644
> > --- a/drivers/hwmon/max31827.c
> > +++ b/drivers/hwmon/max31827.c
> > @@ -11,8 +11,8 @@
> >   #include <linux/hwmon.h>
> >   #include <linux/i2c.h>
> >   #include <linux/mutex.h>
> > -#include <linux/regmap.h>
> >   #include <linux/regulator/consumer.h>
> > +#include <linux/hwmon-sysfs.h>
> >   #include <linux/of_device.h>
> >
> >   #define MAX31827_T_REG			0x0
> > @@ -24,11 +24,13 @@
> >
> >   #define MAX31827_CONFIGURATION_1SHOT_MASK	BIT(0)
> >   #define MAX31827_CONFIGURATION_CNV_RATE_MASK
> 	GENMASK(3, 1)
> > +#define MAX31827_CONFIGURATION_PEC_EN_MASK	BIT(4)
> >   #define MAX31827_CONFIGURATION_TIMEOUT_MASK	BIT(5)
> >   #define MAX31827_CONFIGURATION_RESOLUTION_MASK
> 	GENMASK(7, 6)
> >   #define MAX31827_CONFIGURATION_ALRM_POL_MASK	BIT(8)
> >   #define MAX31827_CONFIGURATION_COMP_INT_MASK	BIT(9)
> >   #define MAX31827_CONFIGURATION_FLT_Q_MASK	GENMASK(11, 10)
> > +#define MAX31827_CONFIGURATION_PEC_ERR_MASK	BIT(13)
> >   #define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK	BIT(14)
> >   #define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK	BIT(15)
> >
> > @@ -94,23 +96,67 @@ struct max31827_state {
> >   	 * Prevent simultaneous access to the i2c client.
> >   	 */
> >   	struct mutex lock;
> > -	struct regmap *regmap;
> >   	bool enable;
> >   	unsigned int resolution;
> >   	unsigned int update_interval;
> > +	struct i2c_client *client;
> >   };
> >
> > -static const struct regmap_config max31827_regmap = {
> > -	.reg_bits = 8,
> > -	.val_bits = 16,
> > -	.max_register = 0xA,
> > -};
> > +static int max31827_reg_read(struct i2c_client *client, u8 reg, u16
> > +*val) {
> > +	u16 tmp = i2c_smbus_read_word_swapped(client, reg);
> > +
> > +	if (tmp < 0)
> 
> An u16 variable will never be negative.
> 
> > +		return tmp;
> > +
> > +	*val = tmp;
> > +	return 0;
> > +}
> 
> If regmap can indeed not be used, it is unnecessary to provide a pointer to
> the return value. Instead, just like with smbus calls, the error return and
> the return value can be combined. Adding this function just to separate
> error from return value adds zero value (and, as can be seen from the
> above, actually adds an opportunity to introduce bugs).
> 
> > +
> > +static int max31827_reg_write(struct i2c_client *client, u8 reg, u16
> > +val) {
> > +	u16 cfg;
> > +	int ret;
> > +
> > +	ret = i2c_smbus_write_word_swapped(client, reg, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	// If PEC is not enabled, return with success
> 
> Do not mix comment styles. The rest of the driver doesn't use C++
> comments.
> Besides, the comment does not add any value.
> 
> > +	if (!(client->flags & I2C_CLIENT_PEC))
> > +		return 0;
> > +
> > +	ret = max31827_reg_read(client,
> MAX31827_CONFIGURATION_REG, &cfg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (cfg & MAX31827_CONFIGURATION_PEC_ERR_MASK)
> > +		return -EBADMSG;
> > +
> 
> EBADMSG is "Not a data message". I don't think that is appropriate here.
> 
> I would very much prefer
> 	if (client->flags & I2C_CLIENT_PEC) {
> 		...
> 	}
> 
> > +	return 0;
> > +}
> > +
> > +static int max31827_update_bits(struct i2c_client *client, u8 reg,
> > +				u16 mask, u16 val)
> > +{
> > +	u16 tmp;
> > +	int ret;
> > +
> > +	ret = max31827_reg_read(client, reg, &tmp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tmp = (tmp & ~mask) | (val & mask);
> > +	ret = max31827_reg_write(client, reg, tmp);
> > +
> > +	return ret;
> > +}
> >
> >   static int shutdown_write(struct max31827_state *st, unsigned int reg,
> >   			  unsigned int mask, unsigned int val)
> >   {
> > -	unsigned int cfg;
> > -	unsigned int cnv_rate;
> > +	u16 cfg;
> > +	u16 cnv_rate;
> 
> I really do not see the point of those changes. u16 is more expensive than
> unsigned int on many architectures. If retained, you'll have to explain why
> this is needed and beneficial.
> 
> >   	int ret;
> >
> >   	/*
> > @@ -125,34 +171,34 @@ static int shutdown_write(struct
> max31827_state
> > *st, unsigned int reg,
> >
> >   	if (!st->enable) {
> >   		if (!mask)
> > -			ret = regmap_write(st->regmap, reg, val);
> > +			ret = max31827_reg_write(st->client, reg, val);
> >   		else
> > -			ret = regmap_update_bits(st->regmap, reg, mask,
> val);
> > +			ret = max31827_update_bits(st->client, reg,
> mask, val);
> >   		goto unlock;
> >   	}
> >
> > -	ret = regmap_read(st->regmap,
> MAX31827_CONFIGURATION_REG, &cfg);
> > +	ret = max31827_reg_read(st->client,
> MAX31827_CONFIGURATION_REG,
> > +&cfg);
> >   	if (ret)
> >   		goto unlock;
> >
> >   	cnv_rate = MAX31827_CONFIGURATION_CNV_RATE_MASK & cfg;
> >   	cfg = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
> >   		      MAX31827_CONFIGURATION_CNV_RATE_MASK);
> > -	ret = regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, cfg);
> > +	ret = max31827_reg_write(st->client,
> MAX31827_CONFIGURATION_REG,
> > +cfg);
> >   	if (ret)
> >   		goto unlock;
> >
> >   	if (!mask)
> > -		ret = regmap_write(st->regmap, reg, val);
> > +		ret = max31827_reg_write(st->client, reg, val);
> >   	else
> > -		ret = regmap_update_bits(st->regmap, reg, mask, val);
> > +		ret = max31827_update_bits(st->client, reg, mask, val);
> >
> >   	if (ret)
> >   		goto unlock;
> >
> > -	ret = regmap_update_bits(st->regmap,
> MAX31827_CONFIGURATION_REG,
> > -
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > -				 cnv_rate);
> > +	ret = max31827_update_bits(st->client,
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > +				   cnv_rate);
> >
> >   unlock:
> >   	mutex_unlock(&st->lock);
> > @@ -198,15 +244,16 @@ static int max31827_read(struct device *dev,
> enum hwmon_sensor_types type,
> >   			 u32 attr, int channel, long *val)
> >   {
> >   	struct max31827_state *st = dev_get_drvdata(dev);
> > -	unsigned int uval;
> > +	u16 uval;
> >   	int ret = 0;
> >
> >   	switch (type) {
> >   	case hwmon_temp:
> >   		switch (attr) {
> >   		case hwmon_temp_enable:
> > -			ret = regmap_read(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> > +
> 	MAX31827_CONFIGURATION_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> > @@ -226,10 +273,10 @@ static int max31827_read(struct device *dev,
> enum hwmon_sensor_types type,
> >   				 * be changed during the conversion
> process.
> >   				 */
> >
> > -				ret = regmap_update_bits(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG,
> > -
> MAX31827_CONFIGURATION_1SHOT_MASK,
> > -							 1);
> > +				ret = max31827_update_bits(st->client,
> > +
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_1SHOT_MASK,
> > +							   1);
> >   				if (ret) {
> >   					mutex_unlock(&st->lock);
> >   					return ret;
> > @@ -246,7 +293,8 @@ static int max31827_read(struct device *dev,
> enum hwmon_sensor_types type,
> >   			    st->update_interval == 125)
> >   				usleep_range(15000, 20000);
> >
> > -			ret = regmap_read(st->regmap,
> MAX31827_T_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> MAX31827_T_REG,
> > +						&uval);
> >
> >   			mutex_unlock(&st->lock);
> >
> > @@ -257,23 +305,26 @@ static int max31827_read(struct device *dev,
> > enum hwmon_sensor_types type,
> >
> >   			break;
> >   		case hwmon_temp_max:
> > -			ret = regmap_read(st->regmap,
> MAX31827_TH_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> MAX31827_TH_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> >   			*val = MAX31827_16_BIT_TO_M_DGR(uval);
> >   			break;
> >   		case hwmon_temp_max_hyst:
> > -			ret = regmap_read(st->regmap,
> MAX31827_TH_HYST_REG,
> > -					  &uval);
> > +			ret = max31827_reg_read(st->client,
> > +
> 	MAX31827_TH_HYST_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> >   			*val = MAX31827_16_BIT_TO_M_DGR(uval);
> >   			break;
> >   		case hwmon_temp_max_alarm:
> > -			ret = regmap_read(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> > +
> 	MAX31827_CONFIGURATION_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> > @@ -281,23 +332,25 @@ static int max31827_read(struct device *dev,
> enum hwmon_sensor_types type,
> >   					 uval);
> >   			break;
> >   		case hwmon_temp_min:
> > -			ret = regmap_read(st->regmap,
> MAX31827_TL_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> MAX31827_TL_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> >   			*val = MAX31827_16_BIT_TO_M_DGR(uval);
> >   			break;
> >   		case hwmon_temp_min_hyst:
> > -			ret = regmap_read(st->regmap,
> MAX31827_TL_HYST_REG,
> > -					  &uval);
> > +			ret = max31827_reg_read(st->client,
> MAX31827_TL_HYST_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> >   			*val = MAX31827_16_BIT_TO_M_DGR(uval);
> >   			break;
> >   		case hwmon_temp_min_alarm:
> > -			ret = regmap_read(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> > +
> 	MAX31827_CONFIGURATION_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> > @@ -313,8 +366,9 @@ static int max31827_read(struct device *dev,
> enum
> > hwmon_sensor_types type,
> >
> >   	case hwmon_chip:
> >   		if (attr == hwmon_chip_update_interval) {
> > -			ret = regmap_read(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG, &uval);
> > +			ret = max31827_reg_read(st->client,
> > +
> 	MAX31827_CONFIGURATION_REG,
> > +						&uval);
> >   			if (ret)
> >   				break;
> >
> > @@ -355,11 +409,11 @@ static int max31827_write(struct device *dev,
> > enum hwmon_sensor_types type,
> >
> >   			st->enable = val;
> >
> > -			ret = regmap_update_bits(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG,
> > -
> MAX31827_CONFIGURATION_1SHOT_MASK |
> > -
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > -
> MAX31827_DEVICE_ENABLE(val));
> > +			ret = max31827_update_bits(st->client,
> > +
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_1SHOT_MASK |
> > +
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > +
> MAX31827_DEVICE_ENABLE(val));
> >
> >   			mutex_unlock(&st->lock);
> >
> > @@ -402,10 +456,10 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> >   			res =
> FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> >   					 res);
> >
> > -			ret = regmap_update_bits(st->regmap,
> > -
> MAX31827_CONFIGURATION_REG,
> > -
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > -						 res);
> > +			ret = max31827_update_bits(st->client,
> > +
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > +						   res);
> >   			if (ret)
> >   				return ret;
> >
> > @@ -425,10 +479,10 @@ static ssize_t temp1_resolution_show(struct
> device *dev,
> >   				     char *buf)
> >   {
> >   	struct max31827_state *st = dev_get_drvdata(dev);
> > -	unsigned int val;
> > +	u16 val;
> >   	int ret;
> >
> > -	ret = regmap_read(st->regmap,
> MAX31827_CONFIGURATION_REG, &val);
> > +	ret = max31827_reg_read(st->client,
> MAX31827_CONFIGURATION_REG,
> > +&val);
> >   	if (ret)
> >   		return ret;
> >
> > @@ -473,10 +527,63 @@ static ssize_t temp1_resolution_store(struct
> device *dev,
> >   	return ret ? ret : count;
> >   }
> >
> > +static ssize_t pec_show(struct device *dev, struct device_attribute
> *devattr,
> > +			char *buf)
> > +{
> > +	struct max31827_state *st = dev_get_drvdata(dev);
> > +	struct i2c_client *client = st->client;
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "%d\n", !!(client->flags &
> > +I2C_CLIENT_PEC)); }
> > +
> > +static ssize_t pec_store(struct device *dev, struct device_attribute
> *devattr,
> > +			 const char *buf, size_t count)
> > +{
> > +	struct max31827_state *st = dev_get_drvdata(dev);
> > +	struct i2c_client *client = st->client;
> > +	unsigned int val;
> > +	u16 val2;
> > +	int err;
> > +
> > +	err = kstrtouint(buf, 10, &val);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	val2 = FIELD_PREP(MAX31827_CONFIGURATION_PEC_EN_MASK,
> val);
> > +
> > +	if (err)
> > +		return err;
> > +
> > +	switch (val) {
> > +	case 0:
> > +		client->flags &= ~I2C_CLIENT_PEC;
> > +		err = max31827_update_bits(client,
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_PEC_EN_MASK,
> > +					   val2);
> > +		if (err)
> > +			return err;
> > +		break;
> > +	case 1:
> > +		err = max31827_update_bits(client,
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_PEC_EN_MASK,
> > +					   val2);
> > +		if (err)
> > +			return err;
> > +		client->flags |= I2C_CLIENT_PEC;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> >   static DEVICE_ATTR_RW(temp1_resolution);
> > +static DEVICE_ATTR_RW(pec);
> >
> >   static struct attribute *max31827_attrs[] = {
> >   	&dev_attr_temp1_resolution.attr,
> > +	&dev_attr_pec.attr,
> >   	NULL
> >   };
> >   ATTRIBUTE_GROUPS(max31827);
> > @@ -489,9 +596,9 @@ static const struct i2c_device_id
> max31827_i2c_ids[] = {
> >   };
> >   MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> >
> > -static int max31827_init_client(struct max31827_state *st,
> > -				struct device *dev)
> > +static int max31827_init_client(struct max31827_state *st)
> >   {
> > +	struct device *dev = &st->client->dev;
> 
> Now we are absolutely down to personal preference changes.
> I am not at all inclined to accept such changes, sorry.
> 
> Including such changes means I'll have to put extra scrutiny on your patch
> submissions in the future to ensure that you don't try to sneak in similar
> changes, which I find quite frustrating. Is that really necessary ?
> 
> Guenter
> 

Sorry for this! I thought, if I am adding client to the private structure I might as well delete the second parameter of init_client, because I can easily retrieve the device structure from client. I added this line so that the changes to the code are kept to a minimum.

> >   	struct fwnode_handle *fwnode;
> >   	unsigned int res = 0;
> >   	u32 data, lsb_idx;
> > @@ -575,7 +682,7 @@ static int max31827_init_client(struct
> max31827_state *st,
> >   		}
> >   	}
> >
> > -	return regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, res);
> > +	return max31827_reg_write(st->client,
> MAX31827_CONFIGURATION_REG,
> > +res);
> >   }
> >
> >   static const struct hwmon_channel_info *max31827_info[] = { @@
> > -613,17 +720,13 @@ static int max31827_probe(struct i2c_client
> *client)
> >   		return -ENOMEM;
> >
> >   	mutex_init(&st->lock);
> > -
> > -	st->regmap = devm_regmap_init_i2c(client,
> &max31827_regmap);
> > -	if (IS_ERR(st->regmap))
> > -		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > -				     "Failed to allocate regmap.\n");
> > +	st->client = client;
> >
> >   	err = devm_regulator_get_enable(dev, "vref");
> >   	if (err)
> >   		return dev_err_probe(dev, err, "failed to enable
> regulator\n");
> >
> > -	err = max31827_init_client(st, dev);
> > +	err = max31827_init_client(st);
> >   	if (err)
> >   		return err;
> >