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