| Message ID | 20240523121057.5689-1-radu.sabau@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] drivers: hwmon: max31827: Add PEC support | expand |
On Thu, May 23, 2024 at 03:10:56PM +0300, Radu Sabau wrote: > Add support for PEC by attaching PEC attribute to the i2c device. > Add pec_store and pec_show function for accesing the "pec" file. > > Signed-off-by: Radu Sabau <radu.sabau@analog.com> > --- Change log missing. > Documentation/hwmon/max31827.rst | 13 +++++-- > drivers/hwmon/max31827.c | 64 ++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+), 3 deletions(-) > > diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst > index 44ab9dc064cb..9c11a9518c67 100644 > --- a/Documentation/hwmon/max31827.rst > +++ b/Documentation/hwmon/max31827.rst > @@ -131,7 +131,14 @@ 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 Support > +----------- > + > +When reading a register value, the PEC byte is computed and sent by the chip. > + > +PEC on word data transaction respresents a signifcant increase in bandwitdh > +usage (+33% for both write and reads) in normal conditions. > > -PEC is not implemented. > +Since this operation implies there will be an extra delay to each > +transaction, PEC can be disabled or enabled through sysfs. > +Just write 1 to the "pec" file for enabling PEC and 0 for disabling it. > diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c > index f8a13b30f100..e86f8890ee72 100644 > --- a/drivers/hwmon/max31827.c > +++ b/drivers/hwmon/max31827.c > @@ -24,6 +24,7 @@ > > #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) > @@ -475,6 +476,54 @@ static ssize_t temp1_resolution_store(struct device *dev, > > static DEVICE_ATTR_RW(temp1_resolution); > > +static ssize_t pec_show(struct device *dev, struct device_attribute *devattr, > + char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + > + 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 = to_i2c_client(dev); > + unsigned int val; > + int err; > + > + err = kstrtouint(buf, 10, &val); > + if (err < 0) > + return err; > + > + switch (val) { > + case 0: > + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, > + MAX31827_CONFIGURATION_PEC_EN_MASK, > + val); While correct, this is misleading. Should write 0. > + if (err) > + return err; > + > + client->flags &= ~I2C_CLIENT_PEC; > + break; > + case 1: > + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, > + MAX31827_CONFIGURATION_PEC_EN_MASK, > + val); This is wrong. s/val/MAX31827_CONFIGURATION_PEC_EN_MASK/ Guenter
On Thu, 2024-05-23 at 07:19 -0700, Guenter Roeck wrote: > On Thu, May 23, 2024 at 03:10:56PM +0300, Radu Sabau wrote: > > Add support for PEC by attaching PEC attribute to the i2c device. > > Add pec_store and pec_show function for accesing the "pec" file. > > > > Signed-off-by: Radu Sabau <radu.sabau@analog.com> > > --- > > Change log missing. > > > Documentation/hwmon/max31827.rst | 13 +++++-- > > drivers/hwmon/max31827.c | 64 ++++++++++++++++++++++++++++++++ > > 2 files changed, 74 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst > > index 44ab9dc064cb..9c11a9518c67 100644 > > --- a/Documentation/hwmon/max31827.rst > > +++ b/Documentation/hwmon/max31827.rst > > @@ -131,7 +131,14 @@ 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 Support > > +----------- > > + > > +When reading a register value, the PEC byte is computed and sent by the chip. > > + > > +PEC on word data transaction respresents a signifcant increase in bandwitdh > > +usage (+33% for both write and reads) in normal conditions. > > > > -PEC is not implemented. > > +Since this operation implies there will be an extra delay to each > > +transaction, PEC can be disabled or enabled through sysfs. > > +Just write 1 to the "pec" file for enabling PEC and 0 for disabling it. > > diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c > > index f8a13b30f100..e86f8890ee72 100644 > > --- a/drivers/hwmon/max31827.c > > +++ b/drivers/hwmon/max31827.c > > @@ -24,6 +24,7 @@ > > > > #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) > > @@ -475,6 +476,54 @@ static ssize_t temp1_resolution_store(struct device *dev, > > > > static DEVICE_ATTR_RW(temp1_resolution); > > > > +static ssize_t pec_show(struct device *dev, struct device_attribute *devattr, > > + char *buf) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + > > + 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 = to_i2c_client(dev); > > + unsigned int val; > > + int err; > > + > > + err = kstrtouint(buf, 10, &val); > > + if (err < 0) > > + return err; > > + > > + switch (val) { > > + case 0: > > + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, > > + MAX31827_CONFIGURATION_PEC_EN_MASK, > > + val); > > While correct, this is misleading. Should write 0. > > > + if (err) > > + return err; > > + > > + client->flags &= ~I2C_CLIENT_PEC; > > + break; > > + case 1: > > + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, > > + MAX31827_CONFIGURATION_PEC_EN_MASK, > > + val); > > This is wrong. s/val/MAX31827_CONFIGURATION_PEC_EN_MASK/ > > Then, maybe use regmap_set_bits()... - Nuno Sá
On 5/24/24 00:00, Nuno Sá wrote: > On Thu, 2024-05-23 at 07:19 -0700, Guenter Roeck wrote: >> On Thu, May 23, 2024 at 03:10:56PM +0300, Radu Sabau wrote: >>> Add support for PEC by attaching PEC attribute to the i2c device. >>> Add pec_store and pec_show function for accesing the "pec" file. >>> >>> Signed-off-by: Radu Sabau <radu.sabau@analog.com> >>> --- >> >> Change log missing. >> >>> Documentation/hwmon/max31827.rst | 13 +++++-- >>> drivers/hwmon/max31827.c | 64 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 74 insertions(+), 3 deletions(-) >>> >>> diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst >>> index 44ab9dc064cb..9c11a9518c67 100644 >>> --- a/Documentation/hwmon/max31827.rst >>> +++ b/Documentation/hwmon/max31827.rst >>> @@ -131,7 +131,14 @@ 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 Support >>> +----------- >>> + >>> +When reading a register value, the PEC byte is computed and sent by the chip. >>> + >>> +PEC on word data transaction respresents a signifcant increase in bandwitdh >>> +usage (+33% for both write and reads) in normal conditions. >>> >>> -PEC is not implemented. >>> +Since this operation implies there will be an extra delay to each >>> +transaction, PEC can be disabled or enabled through sysfs. >>> +Just write 1 to the "pec" file for enabling PEC and 0 for disabling it. >>> diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c >>> index f8a13b30f100..e86f8890ee72 100644 >>> --- a/drivers/hwmon/max31827.c >>> +++ b/drivers/hwmon/max31827.c >>> @@ -24,6 +24,7 @@ >>> >>> #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) >>> @@ -475,6 +476,54 @@ static ssize_t temp1_resolution_store(struct device *dev, >>> >>> static DEVICE_ATTR_RW(temp1_resolution); >>> >>> +static ssize_t pec_show(struct device *dev, struct device_attribute *devattr, >>> + char *buf) >>> +{ >>> + struct i2c_client *client = to_i2c_client(dev); >>> + >>> + 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 = to_i2c_client(dev); >>> + unsigned int val; >>> + int err; >>> + >>> + err = kstrtouint(buf, 10, &val); >>> + if (err < 0) >>> + return err; >>> + >>> + switch (val) { >>> + case 0: >>> + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, >>> + MAX31827_CONFIGURATION_PEC_EN_MASK, >>> + val); >> >> While correct, this is misleading. Should write 0. >> >>> + if (err) >>> + return err; >>> + >>> + client->flags &= ~I2C_CLIENT_PEC; >>> + break; >>> + case 1: >>> + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, >>> + MAX31827_CONFIGURATION_PEC_EN_MASK, >>> + val); >> >> This is wrong. s/val/MAX31827_CONFIGURATION_PEC_EN_MASK/ >> >> > > Then, maybe use regmap_set_bits()... Good point, and regmap_clear_bits() above. Guenter > > - Nuno Sá >
diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst index 44ab9dc064cb..9c11a9518c67 100644 --- a/Documentation/hwmon/max31827.rst +++ b/Documentation/hwmon/max31827.rst @@ -131,7 +131,14 @@ 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 Support +----------- + +When reading a register value, the PEC byte is computed and sent by the chip. + +PEC on word data transaction respresents a signifcant increase in bandwitdh +usage (+33% for both write and reads) in normal conditions. -PEC is not implemented. +Since this operation implies there will be an extra delay to each +transaction, PEC can be disabled or enabled through sysfs. +Just write 1 to the "pec" file for enabling PEC and 0 for disabling it. diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c index f8a13b30f100..e86f8890ee72 100644 --- a/drivers/hwmon/max31827.c +++ b/drivers/hwmon/max31827.c @@ -24,6 +24,7 @@ #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) @@ -475,6 +476,54 @@ static ssize_t temp1_resolution_store(struct device *dev, static DEVICE_ATTR_RW(temp1_resolution); +static ssize_t pec_show(struct device *dev, struct device_attribute *devattr, + char *buf) +{ + struct i2c_client *client = to_i2c_client(dev); + + 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 = to_i2c_client(dev); + unsigned int val; + int err; + + err = kstrtouint(buf, 10, &val); + if (err < 0) + return err; + + switch (val) { + case 0: + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, + MAX31827_CONFIGURATION_PEC_EN_MASK, + val); + if (err) + return err; + + client->flags &= ~I2C_CLIENT_PEC; + break; + case 1: + err = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG, + MAX31827_CONFIGURATION_PEC_EN_MASK, + val); + if (err) + return err; + + client->flags |= I2C_CLIENT_PEC; + break; + default: + return -EINVAL; + } + + return count; +} + +static DEVICE_ATTR_RW(pec); + static struct attribute *max31827_attrs[] = { &dev_attr_temp1_resolution.attr, NULL @@ -578,6 +627,11 @@ static int max31827_init_client(struct max31827_state *st, return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res); } +static void max31827_remove_pec(void *dev) +{ + device_remove_file(dev, &dev_attr_pec); +} + static const struct hwmon_channel_info *max31827_info[] = { HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MIN_HYST | HWMON_T_MIN_ALARM | @@ -627,6 +681,16 @@ static int max31827_probe(struct i2c_client *client) if (err) return err; + if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_PEC)) { + err = device_create_file(dev, &dev_attr_pec); + if (err) + return err; + + err = devm_add_action_or_reset(dev, max31827_remove_pec, dev); + if (err) + return err; + } + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, st, &max31827_chip_info, max31827_groups);
Add support for PEC by attaching PEC attribute to the i2c device. Add pec_store and pec_show function for accesing the "pec" file. Signed-off-by: Radu Sabau <radu.sabau@analog.com> --- Documentation/hwmon/max31827.rst | 13 +++++-- drivers/hwmon/max31827.c | 64 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-)