RE: [PATCH v2 3/3] hwmon: tmp513: Replace tmp51x_ids->max_channels in struct tmp51x_data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Guenter Roeck,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/3] hwmon: tmp513: Replace tmp51x_ids->max_channels
> in struct tmp51x_data
> 
> On Fri, Aug 25, 2023 at 06:44:56AM +0000, Biju Das wrote:
> > Hi Andy Shevchenko,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v2 3/3] hwmon: tmp513: Replace
> > > tmp51x_ids->max_channels in struct tmp51x_data
> > >
> > > On Thu, Aug 24, 2023 at 09:44:56PM +0100, Biju Das wrote:
> > > > The tmp512 chip has 3 channels whereas tmp513 has 4 channels.
> > > > Avoid using tmp51x_ids for this HW difference by replacing
> > > > tmp51x_ids->max_channels in struct tmp51x_data and drop
> > >
> > > You don't replace it, you replaced "id" by it.
> >  You are correct "id->max_channels"
> >
> 
> "replacing tmp51x_ids->max_channels" is a bit difficult to read.

Agreed.


> 
> > >
> > > > enum tmp51x_ids as there is no user.
> > >
> > > ...
> > >
> > > > +#define TMP51X_TEMP_CONFIG_DEFAULT(a) (0x8780 | GENMASK(11 + (a)
> > > > +- 1,
> > > > +11))
> > >
> > > This seems fragile ("a" can't be 0, and can't be > 4) and will give
> > > not
> 
> Not really, because it is the number of channels and well known.
> We should not optimize the code for something that won't ever happen.
> The number of channels is 3 or 4, and the generated mask needs to be
> 0x3800 or 0x7800 depending on the chip. We could maybe have something like
> 	#define CHANNEL_MASK(channels) (...)
> and or in the other bits separately.
> 
> Anyway, maybe "a" is not the best variable name. Maybe use "channels"
> or "n".

OK, will use channels or n.

> 
> 
> > > good code generation (for GENMASK() use), besides it has 0x8780
> > > magic. What is that?
> > > Two bit field masks?
> > >
> > > 	(BIT(15) | (GENMASK((a) - 1, 0) << 11) | GENMASK(10, 7))
> > >
> 
> Probably better to split out resistance correction (bit 10) and conversion
> rate (bit 7..9) instead of a single mask.
> Just for completeness - bit 15 is "continuous conversion".
> Bit 3..6 are unused, and bit 0..2 are used to configure the gpio pin which
> is currently not supported by the driver.

Thanks for the info.

> 
> > > ?
> >
> > Looks good to me.
> >
> > >
> > > Also add a comment to explain "a" and other bits.
> >
> > I don't have access to user manual to explain these bits.
> > May be Guenter/Eric can help on this.
> >
> 
> Configuration register 2 is documented on page 35 and 36.
> 
> If you change this, please consider using defines for the various bits.

OK, will do.

> 
> > >
> > > ...
> > >
> > > >  	case hwmon_temp:
> > > > -		if (data->id == tmp512 && channel == 3)
> > > > +		if (data->max_channels == channel)
> > >
> > > This is not the same as it was previously. And looks like this kind
> > > of fix (if I understood the previous patch correctly) should be done
> there.
> >
> > Logic wise it checks for invalid channels. But newer logic check for
> > invalid channels for both tmp512 and tmp513.
> > compared to only for tmp512 in previous case. For me it looks like an
> > improvement.
> >
> > You mean split this into 2.  First patch with just this logic (channel
> > >= data->max_channels) and keep data->id in remaining Places and Second
> patch is to replace the id and use max_channels instead.
> >
> 
> There is only one field available in struct i2c_device_id.
> Splitting this patch in 2 seems overkill because the first patch would have
> to introduce code (set max_channels based on enum tmp51x_ids) only to
> remove it in the second patch.

The plan was,

First patch will replace id->max_channels in the table
and will fill the id based on max_channels and fix the logic for invalid channels.

The Second patch is to remove id altogether.

I am ok with doing it in the same patch. Please let me know.

> 
> > >
> > > Btw, avoid Yoda style
> > >
> > > 		if (channel == data->max_channels)
> >
> > OK, it should be (channel >= data->max_channels)
> >
> > >
> > > >  			return 0;
> > >
> > > ...
> > >
> > > >  	ret = device_property_read_u32_array(dev, "ti,nfactor",
> nfactor,
> > > > -					    (data->id == tmp513) ? 3 : 2);
> > > > +					    data->max_channels - 1);
> > > >  	if (ret >= 0)
> > > > -		memcpy(data->nfactor, nfactor, (data->id == tmp513) ?
> 3 : 2);
> > > > +		memcpy(data->nfactor, nfactor, data->max_channels - 1);
> > >
> > > It looks like data->nfactor is of the same type as nfactor, right?
> > > Why this can't be simplified to just
> > >
> > > 	device_property_read_u32_array(dev, "ti,nfactor",
> > > 				       data->nfactor, data->max_channels - 1);
> >
> > Yes, I think you are correct. The below code doesn't makes sense. I guess
> this should be another patch.
> >
> 
> "doesn't make sense" is a bit strong, but I agree that the code is
> unnecessarily complex. And, yes, that would be a separate patch.

My bad English. Sorry for that.

Cheers,
Biju

> 
> > 	if (ret >= 0)
> > 		memcpy(data->nfactor, nfactor, data->max_channels - 1);
> >
> > >
> > > ...
> > >
> > > > -	data->temp_config = (data->id == tmp513) ?
> > > > -			TMP513_TEMP_CONFIG_DEFAULT :
> TMP512_TEMP_CONFIG_DEFAULT;
> > >
> > > Are those still being in use?
> >
> > Nope. Will remove it.
> >
> Not sure I understand. The above lines _are_ being removed
> (- in 1st column). What else is there to remove ?
> 
> Thanks,
> Guenter
> 
> > Cheers,
> > Biju
> >
> > >
> > > > +	data->temp_config =
> > > > +TMP51X_TEMP_CONFIG_DEFAULT(data->max_channels);
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> >




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux