> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Saturday, December 16, 2023 3:33 AM
> To: Matyas, Daniel <Daniel.Matyas@xxxxxxxxxx>
> Cc: Jean Delvare <jdelvare@xxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; linux-
> hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx> On Behalf Of Guenter
> Roeck
> >> Sent: Thursday, December 14, 2023 6:10 PM
> >> To: Matyas, Daniel <Daniel.Matyas@xxxxxxxxxx>
> >> Cc: Jean Delvare <jdelvare@xxxxxxxx>; Rob Herring
> >> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> >> <conor+dt@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; linux-
> >> hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> >> kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxx>
> >>
> >> 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
