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