RE: [PATCH v3 1/5] hwmon: max31827: Make code cleaner

[Matyas, Daniel]

> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Saturday, September 16, 2023 2:26 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 v3 1/5] hwmon: max31827: Make code cleaner
> 
> [External]
> 
> On 9/14/23 00:59, Daniel Matyas wrote:
> > Now the wait time for one-shot is 140ms, instead of the old 141
> > (removed the 1ms error).
> >
> 
> It was explicitly documented that the wait time was 140 + 1 milli-seconds,
> presumably to be sure that the conversion is really complete.
> 
> Why was this an error ? It was _documented_ that way.
> 
> Guenter
> 

Well... actually I developed the driver initially and I wrote the documentation, so I know. I decided to remove the error milli-second, because I realized, it isn't really needed. There is no reference about it in the documentation of the chip, and frankly, I didn’t actually encounter any error which would need the 1 milli-second.

This way, the wait time is more exact and the correspondence with the chip documentation becomes quite straightforward.

Daniel

> > Used enums and while loops to replace switch for selecting and getting
> > update interval from conversion rate bits.
> >
> > Divided the write_alarm_val function into 2 functions. The new
> > function is more generic: it can be used not only for alarm writes,
> > but for any kind of writes which require the device to be in shutdown
> mode.
> >
> > Signed-off-by: Daniel Matyas <daniel.matyas@xxxxxxxxxx>
> > ---
> >
> > v2 -> v3: No change.
> >
> > v2: Added patch.
> >
> >   Documentation/hwmon/max31827.rst |   4 +-
> >   drivers/hwmon/max31827.c         | 127 ++++++++++++++-----------------
> >   2 files changed, 58 insertions(+), 73 deletions(-)
> >
> > diff --git a/Documentation/hwmon/max31827.rst
> > b/Documentation/hwmon/max31827.rst
> > index b0971d05b8a4..9a1055a007cf 100644
> > --- a/Documentation/hwmon/max31827.rst
> > +++ b/Documentation/hwmon/max31827.rst
> > @@ -73,8 +73,8 @@ the conversion frequency to 1 conv/s. The
> conversion time varies depending on
> >   the resolution. The conversion time doubles with every bit of increased
> >   resolution. For 10 bit resolution 35ms are needed, while for 12 bit
> resolution
> >   (default) 140ms. When chip is in shutdown mode and a read operation
> > is -requested, one-shot is triggered, the device waits for 140
> > (conversion time) + 1
> > -(error) ms, and only after that is the temperature value register read.
> > +requested, one-shot is triggered, the device waits for 140
> > +(conversion time) ms, and only after that is the temperature value
> register read.
> >
> >   The LSB of the temperature values is 0.0625 degrees Celsius, but the
> values of
> >   the temperatures are displayed in milli-degrees. This means, that
> > some data is diff --git a/drivers/hwmon/max31827.c
> > b/drivers/hwmon/max31827.c index 602f4e4f81ff..f05762219995
> 100644
> > --- a/drivers/hwmon/max31827.c
> > +++ b/drivers/hwmon/max31827.c
> > @@ -25,20 +25,32 @@
> >   #define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK	BIT(14)
> >   #define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK	BIT(15)
> >
> > -#define MAX31827_12_BIT_CNV_TIME	141
> > -
> > -#define MAX31827_CNV_1_DIV_64_HZ	0x1
> > -#define MAX31827_CNV_1_DIV_32_HZ	0x2
> > -#define MAX31827_CNV_1_DIV_16_HZ	0x3
> > -#define MAX31827_CNV_1_DIV_4_HZ		0x4
> > -#define MAX31827_CNV_1_HZ		0x5
> > -#define MAX31827_CNV_4_HZ		0x6
> > -#define MAX31827_CNV_8_HZ		0x7
> > +#define MAX31827_12_BIT_CNV_TIME	140
> >
> >   #define MAX31827_16_BIT_TO_M_DGR(x)	(sign_extend32(x, 15) *
> 1000 / 16)
> >   #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
> >   #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
> >
> > +enum max31827_cnv {
> > +	MAX31827_CNV_1_DIV_64_HZ = 1,
> > +	MAX31827_CNV_1_DIV_32_HZ,
> > +	MAX31827_CNV_1_DIV_16_HZ,
> > +	MAX31827_CNV_1_DIV_4_HZ,
> > +	MAX31827_CNV_1_HZ,
> > +	MAX31827_CNV_4_HZ,
> > +	MAX31827_CNV_8_HZ,
> > +};
> > +
> > +static const u16 max31827_conversions[] = {
> > +	[MAX31827_CNV_1_DIV_64_HZ] = 64000,
> > +	[MAX31827_CNV_1_DIV_32_HZ] = 32000,
> > +	[MAX31827_CNV_1_DIV_16_HZ] = 16000,
> > +	[MAX31827_CNV_1_DIV_4_HZ] = 4000,
> > +	[MAX31827_CNV_1_HZ] = 1000,
> > +	[MAX31827_CNV_4_HZ] = 250,
> > +	[MAX31827_CNV_8_HZ] = 125,
> > +};
> > +
> >   struct max31827_state {
> >   	/*
> >   	 * Prevent simultaneous access to the i2c client.
> > @@ -54,15 +66,13 @@ static const struct regmap_config
> max31827_regmap = {
> >   	.max_register = 0xA,
> >   };
> >
> > -static int write_alarm_val(struct max31827_state *st, unsigned int reg,
> > -			   long val)
> > +static int shutdown_write(struct max31827_state *st, unsigned int reg,
> > +			  unsigned int val)
> >   {
> >   	unsigned int cfg;
> > -	unsigned int tmp;
> > +	unsigned int cnv_rate;
> >   	int ret;
> >
> > -	val = MAX31827_M_DGR_TO_16_BIT(val);
> > -
> >   	/*
> >   	 * Before the Temperature Threshold Alarm and Alarm Hysteresis
> Threshold
> >   	 * register values are changed over I2C, the part must be in
> > shutdown @@ -82,9 +92,10 @@ static int write_alarm_val(struct
> max31827_state *st, unsigned int reg,
> >   	if (ret)
> >   		goto unlock;
> >
> > -	tmp = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
> > +	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, tmp);
> > +	ret = regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, cfg);
> >   	if (ret)
> >   		goto unlock;
> >
> > @@ -92,13 +103,23 @@ static int write_alarm_val(struct
> max31827_state *st, unsigned int reg,
> >   	if (ret)
> >   		goto unlock;
> >
> > -	ret = regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, cfg);
> > +	ret = regmap_update_bits(st->regmap,
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > +				 cnv_rate);
> >
> >   unlock:
> >   	mutex_unlock(&st->lock);
> >   	return ret;
> >   }
> >
> > +static int write_alarm_val(struct max31827_state *st, unsigned int reg,
> > +			   long val)
> > +{
> > +	val = MAX31827_M_DGR_TO_16_BIT(val);
> > +
> > +	return shutdown_write(st, reg, val); }
> > +
> >   static umode_t max31827_is_visible(const void *state,
> >   				   enum hwmon_sensor_types type, u32
> attr,
> >   				   int channel)
> > @@ -243,32 +264,7 @@ static int max31827_read(struct device *dev,
> enum
> > hwmon_sensor_types type,
> >
> >   			uval =
> FIELD_GET(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> >   					 uval);
> > -			switch (uval) {
> > -			case MAX31827_CNV_1_DIV_64_HZ:
> > -				*val = 64000;
> > -				break;
> > -			case MAX31827_CNV_1_DIV_32_HZ:
> > -				*val = 32000;
> > -				break;
> > -			case MAX31827_CNV_1_DIV_16_HZ:
> > -				*val = 16000;
> > -				break;
> > -			case MAX31827_CNV_1_DIV_4_HZ:
> > -				*val = 4000;
> > -				break;
> > -			case MAX31827_CNV_1_HZ:
> > -				*val = 1000;
> > -				break;
> > -			case MAX31827_CNV_4_HZ:
> > -				*val = 250;
> > -				break;
> > -			case MAX31827_CNV_8_HZ:
> > -				*val = 125;
> > -				break;
> > -			default:
> > -				*val = 0;
> > -				break;
> > -			}
> > +			*val = max31827_conversions[uval];
> >   		}
> >   		break;
> >
> > @@ -284,6 +280,7 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> >   			  u32 attr, int channel, long val)
> >   {
> >   	struct max31827_state *st = dev_get_drvdata(dev);
> > +	int res = 1;
> >   	int ret;
> >
> >   	switch (type) {
> > @@ -333,39 +330,27 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> >   			if (!st->enable)
> >   				return -EINVAL;
> >
> > -			switch (val) {
> > -			case 125:
> > -				val = MAX31827_CNV_8_HZ;
> > -				break;
> > -			case 250:
> > -				val = MAX31827_CNV_4_HZ;
> > -				break;
> > -			case 1000:
> > -				val = MAX31827_CNV_1_HZ;
> > -				break;
> > -			case 4000:
> > -				val = MAX31827_CNV_1_DIV_4_HZ;
> > -				break;
> > -			case 16000:
> > -				val = MAX31827_CNV_1_DIV_16_HZ;
> > -				break;
> > -			case 32000:
> > -				val = MAX31827_CNV_1_DIV_32_HZ;
> > -				break;
> > -			case 64000:
> > -				val = MAX31827_CNV_1_DIV_64_HZ;
> > -				break;
> > -			default:
> > -				return -EINVAL;
> > -			}
> > +			/*
> > +			 * Convert the desired conversion rate into
> register
> > +			 * bits. res is already initialized with 1.
> > +			 *
> > +			 * This was inspired by lm73 driver.
> > +			 */
> > +			while (res < ARRAY_SIZE(max31827_conversions)
> &&
> > +			       val < max31827_conversions[res])
> > +				res++;
> > +
> > +			if (res == ARRAY_SIZE(max31827_conversions) ||
> > +			    val != max31827_conversions[res])
> > +				return -EOPNOTSUPP;
> >
> > -			val =
> FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > -					 val);
> > +			res =
> FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > +					 res);
> >
> >   			return regmap_update_bits(st->regmap,
> >
> MAX31827_CONFIGURATION_REG,
> >
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > -						  val);
> > +						  res);
> >   		}
> >   		break;
> >