Message-ID: 
 <SJ0PR03MB6764A464744E3A8CE18C17238990A@SJ0PR03MB6764.namprd03.prod.outlook.com>
Date: Mon, 18 Dec 2023 09:12:31 +0000
From: "Matyas, Daniel" <Daniel.Matyas@...log.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: RE: [PATCH 1/3] hwmon: max31827: Add PEC support



> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Saturday, December 16, 2023 3:33 AM
> To: Matyas, Daniel <Daniel.Matyas@...log.com>
> Cc: Jean Delvare <jdelvare@...e.com>; Rob Herring
> <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> <conor+dt@...nel.org>; Jonathan Corbet <corbet@....net>; linux-
> hwmon@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-doc@...r.kernel.org
> Subject: Re: [PATCH 1/3] hwmon: max31827: Add PEC support
> 
> [External]
> 
> On 12/15/23 12:28, Matyas, Daniel wrote:
> >
> >
> >> -----Original Message-----
> >> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter
> Roeck
> >> Sent: Thursday, December 14, 2023 6:10 PM
> >> To: Matyas, Daniel <Daniel.Matyas@...log.com>
> >> Cc: Jean Delvare <jdelvare@...e.com>; Rob Herring
> >> <robh+dt@...nel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> >> <conor+dt@...nel.org>; Jonathan Corbet <corbet@....net>; linux-
> >> hwmon@...r.kernel.org; devicetree@...r.kernel.org; linux-
> >> kernel@...r.kernel.org; linux-doc@...r.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@...log.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.
> >
> 
> That is not the point. Again,
> 
>  >> The "PEC" attribute needs to be attached to the I2C device.
>  >> See lm90.c or pmbus_core.c for examples.
> 
> That is not about regmap, it is about the location of the "pec" attribute.
> 

I understand that this is not about regmap. Still, I would argue, that when I am registering the device with groups, the "pec" attribute is attached.

> >> 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.
> >
> 
> regmap uses smbus functions to access the chip if the underlying i2c
> driver supports SMBus word operations. I fail to see the difference to your
> code.
> Sure, max31827_reg_write() executes another read after the write, but
> that is again a 16-bit operation and might we well be implemented as
> another regmap operation to read the status register.
> 
> It would be possible to replace the regmap i2c code, use raw regmap code
> instead, and provide ->read and ->write callbacks in struct regmap_config,
> but I am not convinced that this would be beneficial.
> 
> Either case, sorry, I can not follow your line of argument.
> 
> >> 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.
> >
> 
> You are making unnecessary changes (several unsigned int -> u16 plus this
> one), and claim this would be "so that the changes to the code are kept to
> a minimum". Really ? How does making unnecessary changes keep the
> changes to the code to a minimum ?
> 
> Guenter

Powered by blists - more mailing lists