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

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

[Biju Das]

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.

This patch series is only compile tested.

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

[PATCH 1/2] hwmon: tmp513: Convert enum->pointer for data in the match tables

[Biju Das]

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.

Introduce struct tmp51x_info with encum chips and replace enum chips with
struct *tmp51x_info in struct lm85_data. Replace enum->struct *tmp51x_info
for data in the match table. Simplify the probe() by replacing
device_get_match_data() and ID lookup for retrieving data by
i2c_get_match_data().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/hwmon/tmp513.c | 43 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c
index 7db5d0fc24a4..22f6000bc50a 100644
--- a/drivers/hwmon/tmp513.c
+++ b/drivers/hwmon/tmp513.c
@@ -156,6 +156,10 @@ enum tmp51x_ids {
 	tmp512, tmp513
 };
 
+struct tmp51x_info {
+	enum tmp51x_ids id;
+};
+
 struct tmp51x_data {
 	u16 shunt_config;
 	u16 pga_gain;
@@ -169,7 +173,7 @@ struct tmp51x_data {
 	u32 curr_lsb_ua;
 	u32 pwr_lsb_uw;
 
-	enum tmp51x_ids id;
+	const struct tmp51x_info *info;
 	struct regmap *regmap;
 };
 
@@ -434,7 +438,7 @@ static umode_t tmp51x_is_visible(const void *_data,
 
 	switch (type) {
 	case hwmon_temp:
-		if (data->id == tmp512 && channel == 4)
+		if (data->info->id == tmp512 && channel == 4)
 			return 0;
 		switch (attr) {
 		case hwmon_temp_input:
@@ -585,7 +589,7 @@ static int tmp51x_init(struct tmp51x_data *data)
 	if (ret < 0)
 		return ret;
 
-	if (data->id == tmp513) {
+	if (data->info->id == tmp513) {
 		ret = regmap_write(data->regmap, TMP513_N_FACTOR_3,
 				   data->nfactor[2] << 8);
 		if (ret < 0)
@@ -600,22 +604,24 @@ static int tmp51x_init(struct tmp51x_data *data)
 	return regmap_read(data->regmap, TMP51X_STATUS, &regval);
 }
 
+static const struct tmp51x_info tmp512_info = {
+	.id = tmp512,
+};
+
+static const struct tmp51x_info tmp513_info = {
+	.id = tmp513,
+};
+
 static const struct i2c_device_id tmp51x_id[] = {
-	{ "tmp512", tmp512 },
-	{ "tmp513", tmp513 },
+	{ "tmp512", (kernel_ulong_t)&tmp512_info },
+	{ "tmp513", (kernel_ulong_t)&tmp513_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tmp51x_id);
 
 static const struct of_device_id tmp51x_of_match[] = {
-	{
-		.compatible = "ti,tmp512",
-		.data = (void *)tmp512
-	},
-	{
-		.compatible = "ti,tmp513",
-		.data = (void *)tmp513
-	},
+	{ .compatible = "ti,tmp512", .data = &tmp512_info },
+	{ .compatible = "ti,tmp513", .data = &tmp513_info },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, tmp51x_of_match);
@@ -674,9 +680,9 @@ static int tmp51x_read_properties(struct device *dev, struct tmp51x_data *data)
 		return ret;
 
 	ret = device_property_read_u32_array(dev, "ti,nfactor", nfactor,
-					    (data->id == tmp513) ? 3 : 2);
+					    (data->info->id == tmp513) ? 3 : 2);
 	if (ret >= 0)
-		memcpy(data->nfactor, nfactor, (data->id == tmp513) ? 3 : 2);
+		memcpy(data->nfactor, nfactor, (data->info->id == tmp513) ? 3 : 2);
 
 	// Check if shunt value is compatible with pga-gain
 	if (data->shunt_uohms > data->pga_gain * 40 * 1000 * 1000) {
@@ -698,7 +704,7 @@ static void tmp51x_use_default(struct tmp51x_data *data)
 static int tmp51x_configure(struct device *dev, struct tmp51x_data *data)
 {
 	data->shunt_config = TMP51X_SHUNT_CONFIG_DEFAULT;
-	data->temp_config = (data->id == tmp513) ?
+	data->temp_config = (data->info->id == tmp513) ?
 			TMP513_TEMP_CONFIG_DEFAULT : TMP512_TEMP_CONFIG_DEFAULT;
 
 	if (dev->of_node)
@@ -720,10 +726,7 @@ static int tmp51x_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	if (client->dev.of_node)
-		data->id = (uintptr_t)device_get_match_data(&client->dev);
-	else
-		data->id = i2c_match_id(tmp51x_id, client)->driver_data;
+	data->info = i2c_get_match_data(client);
 
 	ret = tmp51x_configure(dev, data);
 	if (ret < 0) {
-- 
2.25.1

[PATCH 2/2] hwmon: tmp513: Add temp_config to struct tmp51x_info

[Biju Das]

Move temp_config from struct tmp51x_data to struct tmp51x_info and
remove unnecessary check in tmp51x_configure().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/hwmon/tmp513.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c
index 22f6000bc50a..138e02a99304 100644
--- a/drivers/hwmon/tmp513.c
+++ b/drivers/hwmon/tmp513.c
@@ -158,6 +158,7 @@ enum tmp51x_ids {
 
 struct tmp51x_info {
 	enum tmp51x_ids id;
+	u16 temp_config;
 };
 
 struct tmp51x_data {
@@ -165,7 +166,6 @@ struct tmp51x_data {
 	u16 pga_gain;
 	u32 vbus_range_uvolt;
 
-	u16 temp_config;
 	u32 nfactor[3];
 
 	u32 shunt_uohms;
@@ -574,7 +574,8 @@ static int tmp51x_init(struct tmp51x_data *data)
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_write(data->regmap, TMP51X_TEMP_CONFIG, data->temp_config);
+	ret = regmap_write(data->regmap, TMP51X_TEMP_CONFIG,
+			   data->info->temp_config);
 	if (ret < 0)
 		return ret;
 
@@ -606,10 +607,12 @@ static int tmp51x_init(struct tmp51x_data *data)
 
 static const struct tmp51x_info tmp512_info = {
 	.id = tmp512,
+	.temp_config = TMP512_TEMP_CONFIG_DEFAULT,
 };
 
 static const struct tmp51x_info tmp513_info = {
 	.id = tmp513,
+	.temp_config = TMP513_TEMP_CONFIG_DEFAULT,
 };
 
 static const struct i2c_device_id tmp51x_id[] = {
@@ -704,9 +707,6 @@ static void tmp51x_use_default(struct tmp51x_data *data)
 static int tmp51x_configure(struct device *dev, struct tmp51x_data *data)
 {
 	data->shunt_config = TMP51X_SHUNT_CONFIG_DEFAULT;
-	data->temp_config = (data->info->id == tmp513) ?
-			TMP513_TEMP_CONFIG_DEFAULT : TMP512_TEMP_CONFIG_DEFAULT;
-
 	if (dev->of_node)
 		return tmp51x_read_properties(dev, data);
 
-- 
2.25.1

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

[Guenter Roeck]

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.

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

Thanks,
Guenter

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

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

[Biju Das]

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.

So Andy proposed to convert the valid enums to non-zero or use a pointer.

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@bp.renesas.com/

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

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

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

[Guenter Roeck]

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@bp.renesas.com/
> 

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

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

[Biju Das]

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

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

[Biju Das]

Hi Guenter Roeck,

> Subject: RE: [PATCH 0/2] Convert enum->pointer for data in the tmp51x match
> tables
> 
> Hi Guenter Roeck,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH 0/2] Convert enum->pointer for data in the tmp51x
> > match tables
> >
> > 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.

I guess, the below initial values for temp_config
is chip specific apart from the number of supported channels
as we won't be able to derive from number of channels??

#define TMP512_TEMP_CONFIG_DEFAULT	0xBF80
#define TMP513_TEMP_CONFIG_DEFAULT	0xFF80

Cheers,
Biju

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

[Guenter Roeck]

On Sun, Aug 20, 2023 at 05:36:02PM +0000, Biju Das wrote:
> Hi Guenter Roeck,
> 
> > Subject: RE: [PATCH 0/2] Convert enum->pointer for data in the tmp51x match
> > tables
> > 
> > Hi Guenter Roeck,
> > 
> > Thanks for the feedback.
> > 
> > > Subject: Re: [PATCH 0/2] Convert enum->pointer for data in the tmp51x
> > > match tables
> > >
> > > 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.
> 
> I guess, the below initial values for temp_config
> is chip specific apart from the number of supported channels
> as we won't be able to derive from number of channels??
> 
> #define TMP512_TEMP_CONFIG_DEFAULT	0xBF80
> #define TMP513_TEMP_CONFIG_DEFAULT	0xFF80
> 
Bit 3-6 enable temperature sensors. The difference in bit 6
is that the 3rd external temperature sensor (which only exists for
tmp513) is enabled if the bit is set. So the base value for
TMP513_TEMP_CONFIG_DEFAULT would be 0xBF80 with bit 6 set if
the chip has four temperature sensors.

Guenter

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

[Andy Shevchenko]

On Sun, Aug 20, 2023 at 08:55:18AM -0700, Guenter Roeck wrote:
> On Sun, Aug 20, 2023 at 02:55:08PM +0000, Biju Das wrote:
> > > On Sun, Aug 20, 2023 at 01:49:08PM +0100, Biju Das wrote:

...

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

Are you talking here about the cases where 0 / NULL makes things optional?
Like when driver data is defined, we use its value, otherwise apply some
defaults? If so, where do you see a problem?

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

Those drivers are hackishly abusing OF ID tables. Those, that have _no_ OF ID
tables are okay.

Moreover this approach allows to have the driver data to be const, and keep it
that way which now is problematic and in some cases may even be broken (due to
wrong assumptions made by drivers). I.o.w. this makes code more robust against
the possible mangling of driver data contents at runtime.

-- 
With Best Regards,
Andy Shevchenko

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

[Guenter Roeck]

On Mon, Aug 21, 2023 at 12:17:34PM +0300, Andy Shevchenko wrote:
> On Sun, Aug 20, 2023 at 08:55:18AM -0700, Guenter Roeck wrote:
> > On Sun, Aug 20, 2023 at 02:55:08PM +0000, Biju Das wrote:
> > > > On Sun, Aug 20, 2023 at 01:49:08PM +0100, Biju Das wrote:
> 
> ...
> 
> > > 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.
> 
> Are you talking here about the cases where 0 / NULL makes things optional?
> Like when driver data is defined, we use its value, otherwise apply some
> defaults? If so, where do you see a problem?
> 

The fact that you have to change lots of drivers to make this work should prove
my point.

> > > 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.
> 
> Those drivers are hackishly abusing OF ID tables. Those, that have _no_ OF ID
> tables are okay.

This is what you are saying. That doesn't make it true. With the same logic
I could claim that drivers providing a pointer in i2c_device_id would
hackishly abuse i2c_device_id tables.

Guenter

> 
> Moreover this approach allows to have the driver data to be const, and keep it
> that way which now is problematic and in some cases may even be broken (due to
> wrong assumptions made by drivers). I.o.w. this makes code more robust against
> the possible mangling of driver data contents at runtime.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 05:59:57AM -0700, Guenter Roeck wrote:
> On Mon, Aug 21, 2023 at 12:17:34PM +0300, Andy Shevchenko wrote:
> > On Sun, Aug 20, 2023 at 08:55:18AM -0700, Guenter Roeck wrote:

...

> > Those drivers are hackishly abusing OF ID tables. Those, that have _no_ OF ID
> > tables are okay.
> 
> This is what you are saying. That doesn't make it true. With the same logic
> I could claim that drivers providing a pointer in i2c_device_id would
> hackishly abuse i2c_device_id tables.

...because kernel_ulong_t was poorly chosen type for that to begin with?

Abusing pointer is much worse than using long as a pointer holder. Moreover
the latter is valid case (okay, not as kernel_ulong_t, but unsigned long)
in many places in the kernel, starting from the memory manager, so your
argument doesn't look like a good one to me.

-- 
With Best Regards,
Andy Shevchenko

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

[Biju Das]

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. see [1]

This patch series extend support for retrieving match data for ID lookup.
The first patch fixes the driver_data for ID table and second patch
convert enum->pointer to avoid non zero values as the proposed
solution returns NULL for non-match.

This patch series is only compile tested.

[1] https://lore.kernel.org/all/20230804161728.394920-1-biju.das.jz@bp.renesas.com/

Biju Das (2):
  power: supply: sbs-battery: Make similar OF and ID table
  power: supply: sbs-battery: Convert enum->pointer for data in the
    match tables

 drivers/power/supply/sbs-battery.c | 42 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 16 deletions(-)

-- 
2.25.1