diff mbox series

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

Message ID 20230820171145.82662-3-biju.das.jz@bp.renesas.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series Convert enum->pointer for data in the sbs-battery match tables | expand

Commit Message

Biju Das Aug. 20, 2023, 5:11 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Aug. 21, 2023, 8:08 a.m. UTC | #1
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
Biju Das Aug. 21, 2023, 8:20 a.m. UTC | #2
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
Geert Uytterhoeven Aug. 21, 2023, 8:37 a.m. UTC | #3
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
Biju Das Aug. 21, 2023, 8:52 a.m. UTC | #4
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
Geert Uytterhoeven Aug. 21, 2023, 8:56 a.m. UTC | #5
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
Biju Das Aug. 21, 2023, 9:02 a.m. UTC | #6
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
diff mbox series

Patch

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