| Message ID | 20230716154442.93908-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Use i2c_get_match_data() | expand |
On 7/16/23 17:44, Biju Das wrote: > The device_get_match_data(), is to get match data for firmware interfaces > such as just OF/ACPI. This driver has I2C matching table as well. Use > i2c_get_match_data() to get match data for I2C, ACPI and DT-based > matching. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Hi Biju, On Sun, Jul 16, 2023 at 5:44 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > The device_get_match_data(), is to get match data for firmware interfaces > such as just OF/ACPI. This driver has I2C matching table as well. Use > i2c_get_match_data() to get match data for I2C, ACPI and DT-based > matching. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -956,7 +956,9 @@ static int vc5_probe(struct i2c_client *client) > > i2c_set_clientdata(client, vc5); > vc5->client = client; > - vc5->chip_info = device_get_match_data(&client->dev); > + vc5->chip_info = i2c_get_match_data(client); > + if (!vc5->chip_info) > + return -ENODEV; Can this actually happen? All tables have data pointers. > vc5->pin_xin = devm_clk_get(&client->dev, "xin"); > if (PTR_ERR(vc5->pin_xin) == -EPROBE_DEFER) Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the review. > Subject: Re: [PATCH 1/2] clk: vc5: Use i2c_get_match_data() instead of > device_get_match_data() > > Hi Biju, > > On Sun, Jul 16, 2023 at 5:44 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > The device_get_match_data(), is to get match data for firmware > > interfaces such as just OF/ACPI. This driver has I2C matching table as > > well. Use > > i2c_get_match_data() to get match data for I2C, ACPI and DT-based > > matching. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- a/drivers/clk/clk-versaclock5.c > > +++ b/drivers/clk/clk-versaclock5.c > > @@ -956,7 +956,9 @@ static int vc5_probe(struct i2c_client *client) > > > > i2c_set_clientdata(client, vc5); > > vc5->client = client; > > - vc5->chip_info = device_get_match_data(&client->dev); > > + vc5->chip_info = i2c_get_match_data(client); > > + if (!vc5->chip_info) > > + return -ENODEV; > > Can this actually happen? All tables have data pointers. It is not needed. I just want to avoid people sending patches as this function can return NULL, so add a check. Please let me know, whether I should remove this? I am happy to send V2 taking out this check. Cheers, Biju > > > vc5->pin_xin = devm_clk_get(&client->dev, "xin"); > > if (PTR_ERR(vc5->pin_xin) == -EPROBE_DEFER) > > 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
Hello Biju, On Mon, 17 Jul 2023 07:46:34 +0000 Biju Das <biju.das.jz@bp.renesas.com> wrote: > Hi Geert, > > Thanks for the review. > > > Subject: Re: [PATCH 1/2] clk: vc5: Use i2c_get_match_data() instead of > > device_get_match_data() > > > > Hi Biju, > > > > On Sun, Jul 16, 2023 at 5:44 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > The device_get_match_data(), is to get match data for firmware > > > interfaces such as just OF/ACPI. This driver has I2C matching table as > > > well. Use > > > i2c_get_match_data() to get match data for I2C, ACPI and DT-based > > > matching. Good point, thanks! > > > --- a/drivers/clk/clk-versaclock5.c > > > +++ b/drivers/clk/clk-versaclock5.c > > > @@ -956,7 +956,9 @@ static int vc5_probe(struct i2c_client *client) > > > > > > i2c_set_clientdata(client, vc5); > > > vc5->client = client; > > > - vc5->chip_info = device_get_match_data(&client->dev); > > > + vc5->chip_info = i2c_get_match_data(client); > > > + if (!vc5->chip_info) > > > + return -ENODEV; > > > > Can this actually happen? All tables have data pointers. > > It is not needed. I just want to avoid people sending > patches as this function can return NULL, so add a check. > > Please let me know, whether I should remove this? > I am happy to send V2 taking out this check. I cannot foresee any sensible future use case for adding an entry without a data pointer as the whole driver is now heavily based on this data to handle so many variants. Also, the error checking did not exist before and the i2c match table is not introducing anything new in terms of .driver_data values. Thus I vote for not adding any error checking here. Otherwise looks good. Luca
Hi Luca, Thanks for the feedback. > -----Original Message----- > From: Luca Ceresoli <luca.ceresoli@bootlin.com> > Sent: Monday, July 17, 2023 1:56 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>; Michael Turquette > <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; linux- > clk@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Marek > Vasut <marex@denx.de>; Alexander Helms <alexander.helms.jy@renesas.com>; > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux- > renesas-soc@vger.kernel.org > Subject: Re: [PATCH 1/2] clk: vc5: Use i2c_get_match_data() instead of > device_get_match_data() > > Hello Biju, > > On Mon, 17 Jul 2023 07:46:34 +0000 > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Hi Geert, > > > > Thanks for the review. > > > > > Subject: Re: [PATCH 1/2] clk: vc5: Use i2c_get_match_data() instead > > > of > > > device_get_match_data() > > > > > > Hi Biju, > > > > > > On Sun, Jul 16, 2023 at 5:44 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > The device_get_match_data(), is to get match data for firmware > > > > interfaces such as just OF/ACPI. This driver has I2C matching > > > > table as well. Use > > > > i2c_get_match_data() to get match data for I2C, ACPI and DT-based > > > > matching. > > Good point, thanks! > > > > > --- a/drivers/clk/clk-versaclock5.c > > > > +++ b/drivers/clk/clk-versaclock5.c > > > > @@ -956,7 +956,9 @@ static int vc5_probe(struct i2c_client > > > > *client) > > > > > > > > i2c_set_clientdata(client, vc5); > > > > vc5->client = client; > > > > - vc5->chip_info = device_get_match_data(&client->dev); > > > > + vc5->chip_info = i2c_get_match_data(client); > > > > + if (!vc5->chip_info) > > > > + return -ENODEV; > > > > > > Can this actually happen? All tables have data pointers. > > > > It is not needed. I just want to avoid people sending patches as this > > function can return NULL, so add a check. > > > > Please let me know, whether I should remove this? > > I am happy to send V2 taking out this check. > > I cannot foresee any sensible future use case for adding an entry > without a data pointer as the whole driver is now heavily based on this > data to handle so many variants. Also, the error checking did not exist > before and the i2c match table is not introducing anything new in terms > of .driver_data values. > > Thus I vote for not adding any error checking here. OK will remove error checking in next version. Cheers, Biju
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index 8bc54176f325..4197991803ba 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -956,7 +956,9 @@ static int vc5_probe(struct i2c_client *client) i2c_set_clientdata(client, vc5); vc5->client = client; - vc5->chip_info = device_get_match_data(&client->dev); + vc5->chip_info = i2c_get_match_data(client); + if (!vc5->chip_info) + return -ENODEV; vc5->pin_xin = devm_clk_get(&client->dev, "xin"); if (PTR_ERR(vc5->pin_xin) == -EPROBE_DEFER)
The device_get_match_data(), is to get match data for firmware interfaces such as just OF/ACPI. This driver has I2C matching table as well. Use i2c_get_match_data() to get match data for I2C, ACPI and DT-based matching. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- Note: This patch is based on the work done for rtc-isl1208 and is compile tested. --- drivers/clk/clk-versaclock5.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)