[PATCH] iio: dac: ti-dac5571: Use i2c_get_match_data()

[PATCH] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Biju Das]

Replace device_get_match_data() and id lookup for retrieving match data
by i2c_get_match_data() by converting enum->pointer for data in the
match table.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/iio/dac/ti-dac5571.c | 48 ++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
index bab11b9adc25..2bb3f76569ee 100644
--- a/drivers/iio/dac/ti-dac5571.c
+++ b/drivers/iio/dac/ti-dac5571.c
@@ -313,7 +313,6 @@ static int dac5571_probe(struct i2c_client *client)
 	const struct dac5571_spec *spec;
 	struct dac5571_data *data;
 	struct iio_dev *indio_dev;
-	enum chip_id chip_id;
 	int ret, i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -329,12 +328,7 @@ static int dac5571_probe(struct i2c_client *client)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = dac5571_channels;
 
-	if (dev_fwnode(dev))
-		chip_id = (uintptr_t)device_get_match_data(dev);
-	else
-		chip_id = id->driver_data;
-
-	spec = &dac5571_spec[chip_id];
+	spec = i2c_get_match_data(client);
 
 	indio_dev->num_channels = spec->num_channels;
 	data->spec = spec;
@@ -392,31 +386,31 @@ static void dac5571_remove(struct i2c_client *i2c)
 }
 
 static const struct of_device_id dac5571_of_id[] = {
-	{.compatible = "ti,dac5571", .data = (void *)single_8bit},
-	{.compatible = "ti,dac6571", .data = (void *)single_10bit},
-	{.compatible = "ti,dac7571", .data = (void *)single_12bit},
-	{.compatible = "ti,dac5574", .data = (void *)quad_8bit},
-	{.compatible = "ti,dac6574", .data = (void *)quad_10bit},
-	{.compatible = "ti,dac7574", .data = (void *)quad_12bit},
-	{.compatible = "ti,dac5573", .data = (void *)quad_8bit},
-	{.compatible = "ti,dac6573", .data = (void *)quad_10bit},
-	{.compatible = "ti,dac7573", .data = (void *)quad_12bit},
-	{.compatible = "ti,dac121c081", .data = (void *)single_12bit},
+	{.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
+	{.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
+	{.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
+	{.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
+	{.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
+	{.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
+	{.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
+	{.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
+	{.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
+	{.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dac5571_of_id);
 
 static const struct i2c_device_id dac5571_id[] = {
-	{"dac5571", single_8bit},
-	{"dac6571", single_10bit},
-	{"dac7571", single_12bit},
-	{"dac5574", quad_8bit},
-	{"dac6574", quad_10bit},
-	{"dac7574", quad_12bit},
-	{"dac5573", quad_8bit},
-	{"dac6573", quad_10bit},
-	{"dac7573", quad_12bit},
-	{"dac121c081", single_12bit},
+	{"dac5571", (kernel_ulong_t)&dac5571_spec[single_8bit] },
+	{"dac6571", (kernel_ulong_t)&dac5571_spec[single_10bit] },
+	{"dac7571", (kernel_ulong_t)&dac5571_spec[single_12bit] },
+	{"dac5574", (kernel_ulong_t)&dac5571_spec[quad_8bit] },
+	{"dac6574", (kernel_ulong_t)&dac5571_spec[quad_10bit] },
+	{"dac7574", (kernel_ulong_t)&dac5571_spec[quad_12bit] },
+	{"dac5573", (kernel_ulong_t)&dac5571_spec[quad_8bit] },
+	{"dac6573", (kernel_ulong_t)&dac5571_spec[quad_10bit] },
+	{"dac7573", (kernel_ulong_t)&dac5571_spec[quad_12bit] },
+	{"dac121c081", (kernel_ulong_t)&dac5571_spec[single_12bit] },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, dac5571_id);
-- 
2.25.1

Re: [PATCH] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Andy Shevchenko]

On Sat, Aug 12, 2023 at 10:04:18AM +0100, Biju Das wrote:
> Replace device_get_match_data() and id lookup for retrieving match data
> by i2c_get_match_data() by converting enum->pointer for data in the
> match table.

...

> +	{.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
> +	{.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
> +	{.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
> +	{.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
> +	{.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
> +	{.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
> +	{.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
> +	{.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
> +	{.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
> +	{.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },

I would reorder them a bit.

	{.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
	{.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
	{.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
	{.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
	{.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
	{.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
	{.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
	{.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
	{.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
	{.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },

Same for I2C ID table.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Geert Uytterhoeven]

Hi Andy,

On Tue, Aug 15, 2023 at 9:19 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Sat, Aug 12, 2023 at 10:04:18AM +0100, Biju Das wrote:
> > Replace device_get_match_data() and id lookup for retrieving match data
> > by i2c_get_match_data() by converting enum->pointer for data in the
> > match table.
>
> ...
>
> > +     {.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
> > +     {.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
> > +     {.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
> > +     {.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
> > +     {.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
> > +     {.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
> > +     {.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
> > +     {.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
> > +     {.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
> > +     {.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
>
> I would reorder them a bit.

Which is safe in this particular case...
But not in general, as there might be fall-back compatible values.

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] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 09:29:06AM +0200, Geert Uytterhoeven wrote:
> On Tue, Aug 15, 2023 at 9:19 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, Aug 12, 2023 at 10:04:18AM +0100, Biju Das wrote:
> > > Replace device_get_match_data() and id lookup for retrieving match data
> > > by i2c_get_match_data() by converting enum->pointer for data in the
> > > match table.

...

> > > +     {.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
> > > +     {.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
> > > +     {.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
> > > +     {.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
> > > +     {.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
> > > +     {.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
> > > +     {.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
> > > +     {.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
> > > +     {.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
> > > +     {.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
> >
> > I would reorder them a bit.
> 
> Which is safe in this particular case...
> But not in general, as there might be fall-back compatible values.

You mean the OF ID list must be specifically ordered?! What a nice minefield!
This has to be fixed somewhere else, surely.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Geert Uytterhoeven]

Hi Andy,

On Tue, Aug 15, 2023 at 6:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Aug 15, 2023 at 09:29:06AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 15, 2023 at 9:19 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Sat, Aug 12, 2023 at 10:04:18AM +0100, Biju Das wrote:
> > > > Replace device_get_match_data() and id lookup for retrieving match data
> > > > by i2c_get_match_data() by converting enum->pointer for data in the
> > > > match table.
>
> ...
>
> > > > +     {.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
> > > > +     {.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
> > > > +     {.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
> > > > +     {.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
> > > > +     {.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
> > > > +     {.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
> > > > +     {.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
> > > > +     {.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
> > > > +     {.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
> > > > +     {.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
> > >
> > > I would reorder them a bit.
> >
> > Which is safe in this particular case...
> > But not in general, as there might be fall-back compatible values.
>
> You mean the OF ID list must be specifically ordered?! What a nice minefield!
> This has to be fixed somewhere else, surely.

Seems like it is, cfr. the scoring system in drivers/of/base.c
__of_device_is_compatible().  Sorry for the confusion.

I still tend to order them in match tables though, from most-specific
to least-specific.

Note that soc_device_match() (which is used less, fortunately) does
not have such a scoring system, so order does matter there.

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] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Andy Shevchenko]

On Wed, Aug 16, 2023 at 10:16:00AM +0200, Geert Uytterhoeven wrote:
> On Tue, Aug 15, 2023 at 6:50 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 15, 2023 at 09:29:06AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Aug 15, 2023 at 9:19 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Sat, Aug 12, 2023 at 10:04:18AM +0100, Biju Das wrote:

...

> > > > > +     {.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
> > > > > +     {.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
> > > > > +     {.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
> > > > > +     {.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
> > > > > +     {.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
> > > > > +     {.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
> > > > > +     {.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
> > > > > +     {.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
> > > > > +     {.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
> > > > > +     {.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
> > > >
> > > > I would reorder them a bit.
> > >
> > > Which is safe in this particular case...
> > > But not in general, as there might be fall-back compatible values.
> >
> > You mean the OF ID list must be specifically ordered?! What a nice minefield!
> > This has to be fixed somewhere else, surely.
> 
> Seems like it is, cfr. the scoring system in drivers/of/base.c
> __of_device_is_compatible().  Sorry for the confusion.
> 
> I still tend to order them in match tables though, from most-specific
> to least-specific.
> 
> Note that soc_device_match() (which is used less, fortunately) does
> not have such a scoring system, so order does matter there.

Should be fixed there, because it's a big downside of OF.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: ti-dac5571: Use i2c_get_match_data()

[Geert Uytterhoeven]

Hi Andy,

On Thu, Aug 17, 2023 at 12:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Aug 16, 2023 at 10:16:00AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 15, 2023 at 6:50 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Aug 15, 2023 at 09:29:06AM +0200, Geert Uytterhoeven wrote:
> > > > On Tue, Aug 15, 2023 at 9:19 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Sat, Aug 12, 2023 at 10:04:18AM +0100, Biju Das wrote:
>
> ...
>
> > > > > > +     {.compatible = "ti,dac5571", .data = &dac5571_spec[single_8bit] },
> > > > > > +     {.compatible = "ti,dac6571", .data = &dac5571_spec[single_10bit] },
> > > > > > +     {.compatible = "ti,dac7571", .data = &dac5571_spec[single_12bit] },
> > > > > > +     {.compatible = "ti,dac5574", .data = &dac5571_spec[quad_8bit] },
> > > > > > +     {.compatible = "ti,dac6574", .data = &dac5571_spec[quad_10bit] },
> > > > > > +     {.compatible = "ti,dac7574", .data = &dac5571_spec[quad_12bit] },
> > > > > > +     {.compatible = "ti,dac5573", .data = &dac5571_spec[quad_8bit] },
> > > > > > +     {.compatible = "ti,dac6573", .data = &dac5571_spec[quad_10bit] },
> > > > > > +     {.compatible = "ti,dac7573", .data = &dac5571_spec[quad_12bit] },
> > > > > > +     {.compatible = "ti,dac121c081", .data = &dac5571_spec[single_12bit] },
> > > > >
> > > > > I would reorder them a bit.
> > > >
> > > > Which is safe in this particular case...
> > > > But not in general, as there might be fall-back compatible values.
> > >
> > > You mean the OF ID list must be specifically ordered?! What a nice minefield!
> > > This has to be fixed somewhere else, surely.
> >
> > Seems like it is, cfr. the scoring system in drivers/of/base.c
> > __of_device_is_compatible().  Sorry for the confusion.
> >
> > I still tend to order them in match tables though, from most-specific
> > to least-specific.
> >
> > Note that soc_device_match() (which is used less, fortunately) does
> > not have such a scoring system, so order does matter there.
>
> Should be fixed there, because it's a big downside of OF.

Agreed. Note that it is unrelated to OF.

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