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]

 



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(),
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.

> 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] https://lore.kernel.org/all/20230804161728.394920-1-biju.das.jz@xxxxxxxxxxxxxx/
> 

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.

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.

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.

> > 
> > > 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