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

[PATCH 0/2 RESEND] Convert enum->pointer for data in the sbs-battery 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

[PATCH 1/2 RESEND] power: supply: sbs-battery: Make similar OF and ID table

[Biju Das]

Make similar OF and ID table to extend support for ID match
using i2c_match_data()/device_get_match_data() later. Currently
it works only for OF match tables as the driver_data is wrong for
ID match.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/power/supply/sbs-battery.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index cdfc8466d129..50b11b6df1b3 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -1253,9 +1253,9 @@ static SIMPLE_DEV_PM_OPS(sbs_pm_ops, sbs_suspend, NULL);
 #endif
 
 static const struct i2c_device_id sbs_id[] = {
-	{ "bq20z65", 0 },
-	{ "bq20z75", 0 },
-	{ "sbs-battery", 1 },
+	{ "bq20z65", SBS_FLAGS_TI_BQ20ZX5 },
+	{ "bq20z75", SBS_FLAGS_TI_BQ20ZX5 },
+	{ "sbs-battery", 0 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, sbs_id);
-- 
2.25.1

[PATCH 2/2 RESEND] power: supply: sbs-battery: 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 and it returns NULL for non-match.

Therefore it is better to convert enum->pointer for data match and extend
match support for both ID and OF tables using i2c_get_match_data() by
adding struct sbs_data with flags variable and replacing flags->data in
struct sbs_info.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/power/supply/sbs-battery.c | 42 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 50b11b6df1b3..06df188c9229 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -201,6 +201,10 @@ static const enum power_supply_property string_properties[] = {
 
 #define NR_STRING_BUFFERS	ARRAY_SIZE(string_properties)
 
+struct sbs_data {
+	u32 flags;
+};
+
 struct sbs_info {
 	struct i2c_client		*client;
 	struct power_supply		*power_supply;
@@ -213,7 +217,7 @@ struct sbs_info {
 	u32				poll_retry_count;
 	struct delayed_work		work;
 	struct mutex			mode_lock;
-	u32				flags;
+	const struct sbs_data		*data;
 	int				technology;
 	char				strings[NR_STRING_BUFFERS][I2C_SMBUS_BLOCK_MAX + 1];
 };
@@ -579,7 +583,7 @@ static int sbs_get_battery_presence_and_health(
 	struct sbs_info *chip = i2c_get_clientdata(client);
 	int ret;
 
-	if (chip->flags & SBS_FLAGS_TI_BQ20ZX5)
+	if (chip->data->flags & SBS_FLAGS_TI_BQ20ZX5)
 		return sbs_get_ti_battery_presence_and_health(client, psp, val);
 
 	/* Dummy command; if it succeeds, battery is present. */
@@ -1135,7 +1139,7 @@ static int sbs_probe(struct i2c_client *client)
 	if (!chip)
 		return -ENOMEM;
 
-	chip->flags = (u32)(uintptr_t)device_get_match_data(&client->dev);
+	chip->data = i2c_get_match_data(client);
 	chip->client = client;
 	psy_cfg.of_node = client->dev.of_node;
 	psy_cfg.drv_data = chip;
@@ -1233,7 +1237,7 @@ static int sbs_suspend(struct device *dev)
 	if (chip->poll_time > 0)
 		cancel_delayed_work_sync(&chip->work);
 
-	if (chip->flags & SBS_FLAGS_TI_BQ20ZX5) {
+	if (chip->data->flags & SBS_FLAGS_TI_BQ20ZX5) {
 		/* Write to manufacturer access with sleep command. */
 		ret = sbs_write_word_data(client,
 					  sbs_data[REG_MANUFACTURER_DATA].addr,
@@ -1252,24 +1256,30 @@ static SIMPLE_DEV_PM_OPS(sbs_pm_ops, sbs_suspend, NULL);
 #define SBS_PM_OPS NULL
 #endif
 
+static const struct sbs_data bq20z65 = {
+	.flags = SBS_FLAGS_TI_BQ20ZX5,
+};
+
+static const struct sbs_data bq20z75 = {
+	.flags = SBS_FLAGS_TI_BQ20ZX5,
+};
+
+static const struct sbs_data sbs_battery = {
+	.flags = 0,
+};
+
 static const struct i2c_device_id sbs_id[] = {
-	{ "bq20z65", SBS_FLAGS_TI_BQ20ZX5 },
-	{ "bq20z75", SBS_FLAGS_TI_BQ20ZX5 },
-	{ "sbs-battery", 0 },
+	{ "bq20z65", (kernel_ulong_t)&bq20z65 },
+	{ "bq20z75", (kernel_ulong_t)&bq20z75 },
+	{ "sbs-battery", (kernel_ulong_t)&sbs_battery },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, sbs_id);
 
 static const struct of_device_id sbs_dt_ids[] = {
-	{ .compatible = "sbs,sbs-battery" },
-	{
-		.compatible = "ti,bq20z65",
-		.data = (void *)SBS_FLAGS_TI_BQ20ZX5,
-	},
-	{
-		.compatible = "ti,bq20z75",
-		.data = (void *)SBS_FLAGS_TI_BQ20ZX5,
-	},
+	{ .compatible = "sbs,sbs-battery", .data = &sbs_battery },
+	{ .compatible = "ti,bq20z65", .data = &bq20z65 },
+	{ .compatible = "ti,bq20z75", .data = &bq20z75 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sbs_dt_ids);
-- 
2.25.1

Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum->pointer for data in the match tables

[Geert Uytterhoeven]

Hi Biju,

On Sun, Aug 20, 2023 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> 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 and it returns NULL for non-match.
>
> Therefore it is better to convert enum->pointer for data match and extend
> match support for both ID and OF tables using i2c_get_match_data() by
> adding struct sbs_data with flags variable and replacing flags->data in
> struct sbs_info.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -201,6 +201,10 @@ static const enum power_supply_property string_properties[] = {
>
>  #define NR_STRING_BUFFERS      ARRAY_SIZE(string_properties)
>
> +struct sbs_data {
> +       u32 flags;
> +};

Unless you plan to add more members to struct sbs_data, I see no point
in this patch: it only increases kernel size.

The various "data" members in <foo>_id structures are intended to
contain either a pointer or a single integral value.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum->pointer for data in the match tables

[Biju Das]

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum-
> >pointer for data in the match tables
> 
> Hi Biju,
> 
> On Sun, Aug 20, 2023 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com>
> 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 and it returns NULL for non-match.
> >
> > Therefore it is better to convert enum->pointer for data match and
> > extend match support for both ID and OF tables using
> > i2c_get_match_data() by adding struct sbs_data with flags variable and
> > replacing flags->data in struct sbs_info.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/power/supply/sbs-battery.c
> > +++ b/drivers/power/supply/sbs-battery.c
> > @@ -201,6 +201,10 @@ static const enum power_supply_property
> > string_properties[] = {
> >
> >  #define NR_STRING_BUFFERS      ARRAY_SIZE(string_properties)
> >
> > +struct sbs_data {
> > +       u32 flags;
> > +};
> 
> Unless you plan to add more members to struct sbs_data, I see no point in
> this patch: it only increases kernel size.
> 
> The various "data" members in <foo>_id structures are intended to contain
> either a pointer or a single integral value.

The match data value for sbs_battery is 0. Here the API returns
NULL for a non-match. That is the reason it is converted to pointer.

So, we cannot differentiate actual matched data and error in this case.

Not sure, cases like this to be split into individual matches like
of_device_get_match(), acpi_match and ID look up instead so that majority of the drivers using uniform OF/ACPI/ID tables and pointers/enums with NoN zero are getting benefitted from proposed device_get_match_data()??

Cheers,
Biju

Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum->pointer for data in the match tables

[Geert Uytterhoeven]

Hi Biju,

On Mon, Aug 21, 2023 at 10:21 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > On Sun, Aug 20, 2023 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com>
> > 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 and it returns NULL for non-match.
> > >
> > > Therefore it is better to convert enum->pointer for data match and
> > > extend match support for both ID and OF tables using
> > > i2c_get_match_data() by adding struct sbs_data with flags variable and
> > > replacing flags->data in struct sbs_info.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > > --- a/drivers/power/supply/sbs-battery.c
> > > +++ b/drivers/power/supply/sbs-battery.c
> > > @@ -201,6 +201,10 @@ static const enum power_supply_property
> > > string_properties[] = {
> > >
> > >  #define NR_STRING_BUFFERS      ARRAY_SIZE(string_properties)
> > >
> > > +struct sbs_data {
> > > +       u32 flags;
> > > +};
> >
> > Unless you plan to add more members to struct sbs_data, I see no point in
> > this patch: it only increases kernel size.
> >
> > The various "data" members in <foo>_id structures are intended to contain
> > either a pointer or a single integral value.
>
> The match data value for sbs_battery is 0. Here the API returns
> NULL for a non-match. That is the reason it is converted to pointer.
>
> So, we cannot differentiate actual matched data and error in this case.

If the driver's .probe() method is called, there must have been a
valid match, so i2c_get_match_data() will never return NULL due to
a non-match.

BTW, the driver does not check for a NULL return value from
*_get_match_data() anyway (and there is no reason to change this!).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum->pointer for data in the match tables

[Biju Das]

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum-
> >pointer for data in the match tables
> 
> Hi Biju,
> 
> On Mon, Aug 21, 2023 at 10:2 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > On Sun, Aug 20, 2023 at 7:12 PM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > 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 and it returns NULL for
> non-match.
> > > >
> > > > Therefore it is better to convert enum->pointer for data match and
> > > > extend match support for both ID and OF tables using
> > > > i2c_get_match_data() by adding struct sbs_data with flags variable
> > > > and replacing flags->data in struct sbs_info.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > > --- a/drivers/power/supply/sbs-battery.c
> > > > +++ b/drivers/power/supply/sbs-battery.c
> > > > @@ -201,6 +201,10 @@ static const enum power_supply_property
> > > > string_properties[] = {
> > > >
> > > >  #define NR_STRING_BUFFERS      ARRAY_SIZE(string_properties)
> > > >
> > > > +struct sbs_data {
> > > > +       u32 flags;
> > > > +};
> > >
> > > Unless you plan to add more members to struct sbs_data, I see no
> > > point in this patch: it only increases kernel size.
> > >
> > > The various "data" members in <foo>_id structures are intended to
> > > contain either a pointer or a single integral value.
> >
> > The match data value for sbs_battery is 0. Here the API returns NULL
> > for a non-match. That is the reason it is converted to pointer.
> >
> > So, we cannot differentiate actual matched data and error in this case.
> 
> If the driver's .probe() method is called, there must have been a valid
> match, so i2c_get_match_data() will never return NULL due to a non-match.

I agree. but "return data" can be 0,if the matched value is 0 (for eg: here "sbs_battery").

> 
> BTW, the driver does not check for a NULL return value from
> *_get_match_data() anyway (and there is no reason to change this!).

I agree, only drivers checking NULL return
value from *_get_match_data() will have this issue.

Cheers,
Biju

Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum->pointer for data in the match tables

[Geert Uytterhoeven]

Hi Biju,

On Mon, Aug 21, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum-
> > >pointer for data in the match tables
> > On Mon, Aug 21, 2023 at 10:2 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > On Sun, Aug 20, 2023 at 7:12 PM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > > 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 and it returns NULL for
> > non-match.
> > > > >
> > > > > Therefore it is better to convert enum->pointer for data match and
> > > > > extend match support for both ID and OF tables using
> > > > > i2c_get_match_data() by adding struct sbs_data with flags variable
> > > > > and replacing flags->data in struct sbs_info.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > >
> > > > > --- a/drivers/power/supply/sbs-battery.c
> > > > > +++ b/drivers/power/supply/sbs-battery.c
> > > > > @@ -201,6 +201,10 @@ static const enum power_supply_property
> > > > > string_properties[] = {
> > > > >
> > > > >  #define NR_STRING_BUFFERS      ARRAY_SIZE(string_properties)
> > > > >
> > > > > +struct sbs_data {
> > > > > +       u32 flags;
> > > > > +};
> > > >
> > > > Unless you plan to add more members to struct sbs_data, I see no
> > > > point in this patch: it only increases kernel size.
> > > >
> > > > The various "data" members in <foo>_id structures are intended to
> > > > contain either a pointer or a single integral value.
> > >
> > > The match data value for sbs_battery is 0. Here the API returns NULL
> > > for a non-match. That is the reason it is converted to pointer.
> > >
> > > So, we cannot differentiate actual matched data and error in this case.
> >
> > If the driver's .probe() method is called, there must have been a valid
> > match, so i2c_get_match_data() will never return NULL due to a non-match.
>
> I agree. but "return data" can be 0,if the matched value is 0 (for eg: here "sbs_battery").

Yes, so what is the problem?
Zero is the expected value for these.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum->pointer for data in the match tables

[Biju Das]

Hi Geert Uytterhoeven,

> Subject: Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert enum-
> >pointer for data in the match tables
> 
> Hi Biju,
> 
> On Mon, Aug 21, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH 2/2 RESEND] power: supply: sbs-battery: Convert
> > > enum-
> > > >pointer for data in the match tables
> > > On Mon, Aug 21, 2023 at 10:2 AM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > On Sun, Aug 20, 2023 at 7:12 PM Biju Das
> > > > > <biju.das.jz@bp.renesas.com>
> > > > > 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 and it
> > > > > > returns NULL for
> > > non-match.
> > > > > >
> > > > > > Therefore it is better to convert enum->pointer for data match
> > > > > > and extend match support for both ID and OF tables using
> > > > > > i2c_get_match_data() by adding struct sbs_data with flags
> > > > > > variable and replacing flags->data in struct sbs_info.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > >
> > > > > > --- a/drivers/power/supply/sbs-battery.c
> > > > > > +++ b/drivers/power/supply/sbs-battery.c
> > > > > > @@ -201,6 +201,10 @@ static const enum power_supply_property
> > > > > > string_properties[] = {
> > > > > >
> > > > > >  #define NR_STRING_BUFFERS      ARRAY_SIZE(string_properties)
> > > > > >
> > > > > > +struct sbs_data {
> > > > > > +       u32 flags;
> > > > > > +};
> > > > >
> > > > > Unless you plan to add more members to struct sbs_data, I see no
> > > > > point in this patch: it only increases kernel size.
> > > > >
> > > > > The various "data" members in <foo>_id structures are intended
> > > > > to contain either a pointer or a single integral value.
> > > >
> > > > The match data value for sbs_battery is 0. Here the API returns
> > > > NULL for a non-match. That is the reason it is converted to pointer.
> > > >
> > > > So, we cannot differentiate actual matched data and error in this
> case.
> > >
> > > If the driver's .probe() method is called, there must have been a
> > > valid match, so i2c_get_match_data() will never return NULL due to a
> non-match.
> >
> > I agree. but "return data" can be 0,if the matched value is 0 (for eg:
> here "sbs_battery").
> 
> Yes, so what is the problem?
> Zero is the expected value for these.

Thanks for the explanation. I agree, for this driver 
there is no need for enum->pointer conversion
as Zero is expected value.

Cheers,
Biju