RE: [PATCH 0/2] Convert enum->pointer for data in the tmp51x match tables

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

 



Hi Guenter Roeck,

Thanks for the feedback.

> Subject: Re: [PATCH 0/2] Convert enum->pointer for data in the tmp51x match
> tables
> 
> On Sun, Aug 20, 2023 at 02:55:08PM +0000, Biju Das wrote:
> > Hi Guenter Roeck,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH 0/2] Convert enum->pointer for data in the
> > > tmp51x match tables
> > >
> > > On Sun, Aug 20, 2023 at 01:49:08PM +0100, Biju Das wrote:
> > > > Convert enum->pointer for data in the match tables, so that
> > > > device_get_match_data() can do match against OF/ACPI/I2C tables,
> > > > once i2c bus type match support added to it.
> > > >
> > >
> > > I don't see why this would be necessary. You don't explain why the
> > > current implementation would no longer work. Various other drivers
> > > implement the same mechanism as this driver, i.e., type cast the
> > > return value of
> > > device_get_match_data() to a non-pointer. I'd argue that changing
> > > the functionality of device_get_match_data() such that this is no
> > > longer possible would be inherently flawed and would introduce
> > > unnecessary complexity to drivers using that mechanism today. If
> > > device_get_match_data() is enhanced to include the functionality of
> > > i2c_match_id(), it should be done in a way that doesn't mandate such
> > > an API change.
> >
> > Currently nothing is broken. There is a helper function to do
> > OF/ACPI/ID match(i2c_get_match_data). But Dmitry proposed to extend
> > device_get_match_data() to buses  so that we can get rid of
> > i2c_get_match_data  and future it can be extended to SPI aswell. see [1].
> >
> > While doing this Jonathan found an issue where it won't work if OF
> > table is using pointer and ID is using enum in the match data.
> Moreover,the proposed API returns NULL for non-match.
> 
> I think that is may be problem because many drivers don't have a value in
> the driver_data field. Maybe that doesn't matter because such drivers would
> normally not call device_get_match_data() or i2c_match_id(),

Yes, that is correct.

 but it would
> create some ambiguity because those functions would no longer work for all
> cases.
> 
> >
> > So Andy proposed to convert the valid enums to non-zero or use a pointer.
> >
> There are _lots_ of drivers where the chip ID is in driver_data and starts
> with 0, or with the field not used. It is not my call to make, but I really
> think that demanding that this field is always != 0 is just wrong.

For hwmon subsystem, this is the only i2c client driver using device_match_data(), other hwmon drivers are using of_device_get_match_data() and ID lookup.

> 
> > In this case, pointer makes sense as the hardware differences between
> > the chips are compared against type rather than using feature and
> > driver data. In the second patch, I just used a driver data to avoid
> > one such case.
> >
> > [1]
> >
> 
> The suggested changes do make some sense independently of the API change,
> but not as suggested. I'd be inclined to accept the changes if the number
> of channels is kept in the configuration data. With that, the chip ID would
> no longer be needed. TMP513_TEMP_CONFIG_DEFAULT and
> TMP512_TEMP_CONFIG_DEFAULT are not really needed in the configuration data
> since the value can be derived from the number of channels instead of the
> chip type.

Yep, instead of using chip type for HW differences,
we can use a macro to get default value based on the number of channels supported by the chip.

> 
> According to what you said earlier, that suggests that the only necessary
> change would be to report the number of channels in driver_data / data
> because that would never be 0.

OK. Since the value is less than 16, there won't be any issue.
device_get_match_data() first look for match against firmware
nodes, and will fallback to ID lookup().

> 
> Either case, I would want to keep temp_config and the number of channels in
> struct tmp51x_data to avoid repeated double dereferences and because
> temp_config could change (resistance correction enabled/disabled should be
> a devicetree property, conversion rate as well as channel enable should be
> sysfs attributes, and channel enable/disable should also be devicetree
> properties). The value could also be used to support suspend/resume in the
> driver if someone wants to add that at some point.
> 
> In this context,
> 		if (data->id == tmp512 && channel == 4)
> 			return 0;
> is wrong because there are 3 or 4 channels, meaning the channel numbers are
> 0..3 and there is no channel 4. This should be "channel == 3"
> to disable the 4th channel on tmp512. Interesting that no one (including
> me ;-) noticed this.

If I am correct, as per above, only max number channels
supported by the chip can go to device data. That is
the only HW difference between these 2 chips and other
chip specific data can be derived from this.

Cheers,
Biju

> 
> > >
> > > > This patch series is only compile tested.
> > >
> > > I assume that means you don't have access to the chip. Is this
> correct ?
> > > Just asking, because it would be great to have a register dump which
> > > would enable me to write unit test code.
> >
> > That is correct, I don't have access to the chip.
> >
> 
> Too bad. Eric, if you are still around, would it be possible to send me
> register dumps ?
> 
> Thanks,
> Guenter
> 
> 
> > Cheers,
> > Biju
> >
> > >
> > > >
> > > > Biju Das (2):
> > > >   hwmon: tmp513: Convert enum->pointer for data in the match tables
> > > >   hwmon: tmp513: Add temp_config to struct tmp51x_info
> > > >
> > > >  drivers/hwmon/tmp513.c | 51
> > > > ++++++++++++++++++++++--------------------
> > > >  1 file changed, 27 insertions(+), 24 deletions(-)
> > > >
> > > > --
> > > > 2.25.1
> > > >




[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