linux-kernel - RE: [PATCH v5] drivers: hwmon: max31827: Add PEC support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: 
 <SA1PR03MB6499B0B15E16917912A24429F7FC2@SA1PR03MB6499.namprd03.prod.outlook.com>
Date: Fri, 31 May 2024 08:26:56 +0000
From: "Sabau, Radu bogdan" <Radu.Sabau@...log.com>
To: "Sabau, Radu bogdan" <Radu.Sabau@...log.com>,
        Jean Delvare
	<jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet
	<corbet@....net>,
        "linux-hwmon@...r.kernel.org"
	<linux-hwmon@...r.kernel.org>,
        "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5] drivers: hwmon: max31827: Add PEC support



> -----Original Message-----
> From: Radu Sabau <radu.sabau@...log.com>
> Sent: Thursday, May 30, 2024 2:25 PM
> To: Jean Delvare <jdelvare@...e.com>; Guenter Roeck <linux@...ck-us.net>; Jonathan Corbet <corbet@....net>; linux-
> hwmon@...r.kernel.org; linux-doc@...r.kernel.org; linux-kernel@...r.kernel.org
> Cc: Sabau, Radu bogdan <Radu.Sabau@...log.com>
> Subject: [PATCH v5] drivers: hwmon: max31827: Add PEC support
> 
> Add support for PEC by configuring chip accordingly to the hwmon core
> PEC attribute handling.
> Add chip_write function to be used by max31827_write when setting
> the attribute.
> 
> Signed-off-by: Radu Sabau <radu.sabau@...log.com>
> ---
> Change log:
> v2:
>  *Rebase on top of v6.9
>  *Attach pec attribute only to i2c device
>  *Fix bug to attach pec attribute to i2c device if the device supports it.
> v3:
>  *Use only one variable to write PEC_EN bit in configuration register
>  *Use regmap_set_bits to set PEC_EN bit when requested instead of
>   regmap_update_bits.
>  *Fix typo in commit message.
> v4:
>  *Use regmap_clear_bits to clear PEC_EN bit when requested instead of
>   regmap_update_bits.
> v5:
>  *Adapt driver to the new hwmon PEC attribute handling from the hwmon core.
> ---
>  Documentation/hwmon/max31827.rst | 13 ++++++++++---
>  drivers/hwmon/max31827.c         | 17 ++++++++++++++++-
>  2 files changed, 26 insertions(+), 4 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..4a5f86afd84e 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)
> @@ -332,6 +333,18 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
>  	return ret;
>  }
> 
> +static int max31827_chip_write(struct *regmap, u32 attr, long val)

regmap parameter is declared wrong here, will send fix in next patch.

> +{
> +	switch (attr) {
> +	case hwmon_chip_pec:
> +		return regmap_update_bits(regmap, MAX31827_CONFIGURATION_REG,
> +					  MAX38127_CONFIGURATION_PEC_EN_MASK,

typo, MAX38127 -> MAX31827

> +					  val ? MAX38127_CONFIGURATION_PEC_EN_MASK : 0);

typo, MAX38127 -> MAX31827

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
>  			  u32 attr, int channel, long val)
>  {
> @@ -340,6 +353,8 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
>  	int ret;
> 
>  	switch (type) {
> +	case hwmon_chip:
> +		return max31827_chip_write(st->regmap, attr, val);
>  	case hwmon_temp:
>  		switch (attr) {
>  		case hwmon_temp_enable:
> @@ -583,7 +598,7 @@ static const struct hwmon_channel_info *max31827_info[] = {
>  					 HWMON_T_MIN_HYST | HWMON_T_MIN_ALARM |
>  					 HWMON_T_MAX | HWMON_T_MAX_HYST |
>  					 HWMON_T_MAX_ALARM),
> -	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL | HWMON_C_PEC),
>  	NULL,
>  };

This patch is wrong due to the fact that hwmon_chip case is duplicated inside the switch, and therefore
I will send another patch fixing this and also modifying max31827_chip_write.

> 
> --
> 2.34.1