Re: [PATCH 4/6] hwmon: (lm95234) Convert to with_info hwmon API

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 7/18/24 17:52, Tzung-Bi Shih wrote:
> On Thu, Jul 18, 2024 at 10:47:55AM -0700, Guenter Roeck wrote:
> > On 7/18/24 09:40, Tzung-Bi Shih wrote:
> > > On Wed, Jul 17, 2024 at 08:39:33PM -0700, Guenter Roeck wrote:
> > > > +static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long val)
> > > >    {
> > > [...]
> > > > +	case hwmon_temp_max:
> > > > +		val = clamp_val(val, 0, channel ? 255000 : 127000);
> > >
> > > Perhaps I am misunderstanding, but this looks weird to me.  By applying
> > > the patch, the maximum values are:
> > >
> > > static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 127000
> > > static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 255000
> > > static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
> > > static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
> > > static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
> > >
> > >
> > > However, it was originally:
> > >
> > > static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
> > > 	        const char *buf, size_t count)
> > > {
> > > [...]
> > >       val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
> > >
> > > static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
> > > 	        const char *buf, size_t count)
> > > {
> > > [...]
> > >       val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
> > > 		    1000);
> > >
> > > static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 255000
> > > static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 127000
> > > static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
> > > static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
> > > static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
> > >
> > > > +		val = DIV_ROUND_CLOSEST(val, 1000);
> > > > +		return regmap_write(regmap, lm95234_crit_reg(channel), val);
> >
> > That is indeed a bug. Here is the fix:
> >
> > diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
> > index c3c68c196479..7da6c8f07332 100644
> > --- a/drivers/hwmon/lm95234.c
> > +++ b/drivers/hwmon/lm95234.c
> > @@ -150,7 +150,7 @@ static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long va
> >                  val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
> >                  return regmap_write(regmap, LM95234_REG_OFFSET(channel - 1), val);
> >          case hwmon_temp_max:
> > -               val = clamp_val(val, 0, channel ? 255000 : 127000);
> > +               val = clamp_val(val, 0, channel == 1 ? 127000 : 255000);
> >                  val = DIV_ROUND_CLOSEST(val, 1000);
> >                  return regmap_write(regmap, lm95234_crit_reg(channel), val);
> >          case hwmon_temp_max_hyst:
>
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@xxxxxxxxxx>
Thanks a lot for the reviews!

Guenter