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