[PATCH RFC 0/2] Extend device_get_match_data() to struct bus_type

[PATCH RFC 0/2] Extend device_get_match_data() to struct bus_type

[Biju Das]

This patch series extend device_get_match_data() to struct bus_type,
so that buses like I2C can get matched data.

Biju Das (2):
  drivers: fwnode: Extend device_get_match_data() to struct bus_type
  i2c: Add i2c_device_get_match_data() callback

 drivers/base/property.c     |  8 +++++++-
 drivers/i2c/i2c-core-base.c | 38 +++++++++++++++++++++++++++++--------
 include/linux/device/bus.h  |  3 +++
 3 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

Extend device_get_match_data() to buses (for eg: I2C) by adding a
callback device_get_match_data() to struct bus_type() and call this method
as a fallback for generic fwnode based device_get_match_data().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/base/property.c    | 8 +++++++-
 include/linux/device/bus.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8c40abed7852..cc0bf7bb6f3a 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1277,7 +1277,13 @@ EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
 
 const void *device_get_match_data(const struct device *dev)
 {
-	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+	const void *data;
+
+	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+	if (!data && dev->bus && dev->bus->get_match_data)
+		data = dev->bus->get_match_data(dev);
+
+	return data;
 }
 EXPORT_SYMBOL_GPL(device_get_match_data);
 
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index ae10c4322754..2e15b0ae5384 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -60,6 +60,7 @@ struct fwnode_handle;
  *			this bus.
  * @dma_cleanup:	Called to cleanup DMA configuration on a device on
  *			this bus.
+ * @get_match_data:	Called to get match data on a device on this bus.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
  * @iommu_ops:  IOMMU specific operations for this bus, used to attach IOMMU
@@ -102,6 +103,8 @@ struct bus_type {
 	int (*dma_configure)(struct device *dev);
 	void (*dma_cleanup)(struct device *dev);
 
+	const void *(*get_match_data)(const struct device *dev);
+
 	const struct dev_pm_ops *pm;
 
 	const struct iommu_ops *iommu_ops;
-- 
2.25.1

Re: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:

Thank you for your contribution!
My comments below.

> Extend device_get_match_data() to buses (for eg: I2C) by adding a
> callback device_get_match_data() to struct bus_type() and call this method
> as a fallback for generic fwnode based device_get_match_data().

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

You can't just throw one's SoB tag without clear understanding what's going on
here (either wrong authorship or missing Co-developed-by or...?).

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

...

>  const void *device_get_match_data(const struct device *dev)
>  {
> -	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
> +	const void *data;
> +
> +	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
> +	if (!data && dev->bus && dev->bus->get_match_data)
> +		data = dev->bus->get_match_data(dev);
> +
> +	return data;

Much better looking is

	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
	if (data)
		return data;

	if (dev->bus && dev->bus->get_match_data)
		return dev->bus->get_match_data(dev);

	return NULL;

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 02:06:07PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:
> 
> Thank you for your contribution!
> My comments below.
> 
> > Extend device_get_match_data() to buses (for eg: I2C) by adding a
> > callback device_get_match_data() to struct bus_type() and call this method
> > as a fallback for generic fwnode based device_get_match_data().
> 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> You can't just throw one's SoB tag without clear understanding what's going on
> here (either wrong authorship or missing Co-developed-by or...?).
> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

...

> >  const void *device_get_match_data(const struct device *dev)

Btw, this needs a documentation update to explain how it works now.

> >  {
> > -	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
> > +	const void *data;
> > +
> > +	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
> > +	if (!data && dev->bus && dev->bus->get_match_data)
> > +		data = dev->bus->get_match_data(dev);
> > +
> > +	return data;
> 
> Much better looking is
> 
> 	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
> 	if (data)
> 		return data;
> 
> 	if (dev->bus && dev->bus->get_match_data)
> 		return dev->bus->get_match_data(dev);
> 
> 	return NULL;
> 
> >  }

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> device_get_match_data() to struct bus_type
> 
> On Mon, Jul 24, 2023 at 02:06:07PM +0300, Andy Shevchenko wrote:
> > On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:
> >
> > Thank you for your contribution!
> > My comments below.
> >
> > > Extend device_get_match_data() to buses (for eg: I2C) by adding a
> > > callback device_get_match_data() to struct bus_type() and call this
> > > method as a fallback for generic fwnode based
> device_get_match_data().
> >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > You can't just throw one's SoB tag without clear understanding what's
> > going on here (either wrong authorship or missing Co-developed-by
> or...?).
> >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> ...
> 
> > >  const void *device_get_match_data(const struct device *dev)
> 
> Btw, this needs a documentation update to explain how it works now.

Can you please point me to the location where I need to update?

Cheers,
Biju

> 
> > >  {
> > > -	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> > > +	const void *data;
> > > +
> > > +	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> > > +	if (!data && dev->bus && dev->bus->get_match_data)
> > > +		data = dev->bus->get_match_data(dev);
> > > +
> > > +	return data;
> >
> > Much better looking is
> >
> > 	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> > 	if (data)
> > 		return data;
> >
> > 	if (dev->bus && dev->bus->get_match_data)
> > 		return dev->bus->get_match_data(dev);
> >
> > 	return NULL;
> >
> > >  }
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 11:46:44AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > device_get_match_data() to struct bus_type
> > On Mon, Jul 24, 2023 at 02:06:07PM +0300, Andy Shevchenko wrote:
> > > On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:

...

> > > >  const void *device_get_match_data(const struct device *dev)

(1)

> > Btw, this needs a documentation update to explain how it works now.
> 
> Can you please point me to the location where I need to update?

Sure. It's just on top of the (1). It looks like no documentation
yet existed, so you need to create one.

Not sure if fwnode.h has to be updated. At least it doesn't contradict
with the added code, while not describing all details.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> device_get_match_data() to struct bus_type
> 
> On Mon, Jul 24, 2023 at 11:46:44AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > device_get_match_data() to struct bus_type On Mon, Jul 24, 2023 at
> > > 02:06:07PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:
> 
> ...
> 
> > > > >  const void *device_get_match_data(const struct device *dev)
> 
> (1)
> 
> > > Btw, this needs a documentation update to explain how it works now.
> >
> > Can you please point me to the location where I need to update?
> 
> Sure. It's just on top of the (1). It looks like no documentation yet
> existed, so you need to create one.
> 
> Not sure if fwnode.h has to be updated. At least it doesn't contradict
> with the added code, while not describing all details.

OK, will do.

Cheers,
Biju

RE: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> device_get_match_data() to struct bus_type
> 
> On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:
> 
> Thank you for your contribution!
> My comments below.
> 
> > Extend device_get_match_data() to buses (for eg: I2C) by adding a
> > callback device_get_match_data() to struct bus_type() and call this
> > method as a fallback for generic fwnode based device_get_match_data().
> 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> You can't just throw one's SoB tag without clear understanding what's
> going on here (either wrong authorship or missing Co-developed-by
> or...?).

Dmitry feels instead of having separate bus based match_data() like i2c_get_match_data[2] and spi_get_device_match_data[3], it is
better to have a generic approach like a single API device_get_match_data()
for getting match_data for OF/ACPI/I2C/SPI tables.

So, he came with a proposal and shared some code here[1].
Since,I have send this patch, I put my signed -off.

If this patch is accepted, then we can get rid of bus based match_data.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230717131756.240645-2-biju.das.jz@bp.renesas.com/#25436207

[2] https://elixir.bootlin.com/linux/v6.5-rc3/source/drivers/i2c/i2c-core-base.c#L117

[3] https://elixir.bootlin.com/linux/v6.5-rc3/source/drivers/spi/spi.c#L364

Cheers,
Biju


> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> ...
> 
> >  const void *device_get_match_data(const struct device *dev)  {
> > -	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> > +	const void *data;
> > +
> > +	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> > +	if (!data && dev->bus && dev->bus->get_match_data)
> > +		data = dev->bus->get_match_data(dev);
> > +
> > +	return data;
> 
> Much better looking is
> 
> 	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> 	if (data)
> 		return data;

OK.

Cheers,
Biju

> 
> 	if (dev->bus && dev->bus->get_match_data)
> 		return dev->bus->get_match_data(dev);
> 
> 	return NULL;
> 
> >  }
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 12:02:27PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > device_get_match_data() to struct bus_type
> > On Sun, Jul 23, 2023 at 09:37:20AM +0100, Biju Das wrote:

...

> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > You can't just throw one's SoB tag without clear understanding what's
> > going on here (either wrong authorship or missing Co-developed-by
> > or...?).
> 
> Dmitry feels instead of having separate bus based match_data() like
> i2c_get_match_data[2] and spi_get_device_match_data[3], it is better to have
> a generic approach like a single API device_get_match_data() for getting
> match_data for OF/ACPI/I2C/SPI tables.
> 
> So, he came with a proposal and shared some code here[1].

Yes, I'm pretty much following the discussion.

> Since,I have send this patch, I put my signed -off.

I'm not talking about this. There is no evidence that Dmitry gives you
any approval to use or clear SoB tag. Again, you may not do like this.

> If this patch is accepted, then we can get rid of bus based match_data.

This is unrelated to what I talking about.

> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230717131756.240645-2-biju.das.jz@bp.renesas.com/#25436207

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> device_get_match_data() to struct bus_type
> 
> On Mon, Jul 24, 2023 at 12:02:27PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > device_get_match_data() to struct bus_type On Sun, Jul 23, 2023 at
> > > 09:37:20AM +0100, Biju Das wrote:
> 
> ...
> 
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > >
> > > You can't just throw one's SoB tag without clear understanding
> > > what's going on here (either wrong authorship or missing
> > > Co-developed-by or...?).
> >
> > Dmitry feels instead of having separate bus based match_data() like
> > i2c_get_match_data[2] and spi_get_device_match_data[3], it is better
> > to have a generic approach like a single API device_get_match_data()
> > for getting match_data for OF/ACPI/I2C/SPI tables.
> >
> > So, he came with a proposal and shared some code here[1].
> 
> Yes, I'm pretty much following the discussion.
> 
> > Since,I have send this patch, I put my signed -off.
> 
> I'm not talking about this. There is no evidence that Dmitry gives you
> any approval to use or clear SoB tag. Again, you may not do like this.

Here Dmitry is acknowledging, he is ok with the patch I posted.

https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230717131756.240645-2-biju.das.jz@bp.renesas.com/#25437032

Cheers,
Biju

Re: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 01:19:02PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > device_get_match_data() to struct bus_type
> > On Mon, Jul 24, 2023 at 12:02:27PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > > device_get_match_data() to struct bus_type On Sun, Jul 23, 2023 at
> > > > 09:37:20AM +0100, Biju Das wrote:

...

> > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > >
> > > > You can't just throw one's SoB tag without clear understanding
> > > > what's going on here (either wrong authorship or missing
> > > > Co-developed-by or...?).
> > >
> > > Dmitry feels instead of having separate bus based match_data() like
> > > i2c_get_match_data[2] and spi_get_device_match_data[3], it is better
> > > to have a generic approach like a single API device_get_match_data()
> > > for getting match_data for OF/ACPI/I2C/SPI tables.
> > >
> > > So, he came with a proposal and shared some code here[1].
> > 
> > Yes, I'm pretty much following the discussion.
> > 
> > > Since,I have send this patch, I put my signed -off.
> > 
> > I'm not talking about this. There is no evidence that Dmitry gives you
> > any approval to use or clear SoB tag. Again, you may not do like this.
> 
> Here Dmitry is acknowledging, he is ok with the patch I posted.
> 
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230717131756.240645-2-biju.das.jz@bp.renesas.com/#25437032

No, you just misinterpreted his message.

See https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
for the explanation. The SoB has to be explicitly given. Dmitry had _not_
put it like this.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> device_get_match_data() to struct bus_type
> 
> On Mon, Jul 24, 2023 at 01:19:02PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > device_get_match_data() to struct bus_type On Mon, Jul 24, 2023 at
> > > 12:02:27PM +0000, Biju Das wrote:
> > > > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > > > device_get_match_data() to struct bus_type On Sun, Jul 23, 2023
> > > > > at 09:37:20AM +0100, Biju Das wrote:
> 
> ...
> 
> > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > >
> > > > > You can't just throw one's SoB tag without clear understanding
> > > > > what's going on here (either wrong authorship or missing
> > > > > Co-developed-by or...?).
> > > >
> > > > Dmitry feels instead of having separate bus based match_data()
> > > > like i2c_get_match_data[2] and spi_get_device_match_data[3], it is
> > > > better to have a generic approach like a single API
> > > > device_get_match_data() for getting match_data for OF/ACPI/I2C/SPI
> tables.
> > > >
> > > > So, he came with a proposal and shared some code here[1].
> > >
> > > Yes, I'm pretty much following the discussion.
> > >
> > > > Since,I have send this patch, I put my signed -off.
> > >
> > > I'm not talking about this. There is no evidence that Dmitry gives
> > > you any approval to use or clear SoB tag. Again, you may not do like
> this.
> >
> > Here Dmitry is acknowledging, he is ok with the patch I posted.
> >
> 
> No, you just misinterpreted his message.
> 

Dmitry,

As you are the author of code, either you post a patch or provide your SoB as per the guideline mentioned here to avoid confusion.

 https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Cheers,
Biju

Re: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Dmitry Torokhov]

On Mon, Jul 24, 2023 at 01:58:55PM +0000, Biju Das wrote:
> Hi Andy,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > device_get_match_data() to struct bus_type
> > 
> > On Mon, Jul 24, 2023 at 01:19:02PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > > device_get_match_data() to struct bus_type On Mon, Jul 24, 2023 at
> > > > 12:02:27PM +0000, Biju Das wrote:
> > > > > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > > > > device_get_match_data() to struct bus_type On Sun, Jul 23, 2023
> > > > > > at 09:37:20AM +0100, Biju Das wrote:
> > 
> > ...
> > 
> > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > >
> > > > > > You can't just throw one's SoB tag without clear understanding
> > > > > > what's going on here (either wrong authorship or missing
> > > > > > Co-developed-by or...?).
> > > > >
> > > > > Dmitry feels instead of having separate bus based match_data()
> > > > > like i2c_get_match_data[2] and spi_get_device_match_data[3], it is
> > > > > better to have a generic approach like a single API
> > > > > device_get_match_data() for getting match_data for OF/ACPI/I2C/SPI
> > tables.
> > > > >
> > > > > So, he came with a proposal and shared some code here[1].
> > > >
> > > > Yes, I'm pretty much following the discussion.
> > > >
> > > > > Since,I have send this patch, I put my signed -off.
> > > >
> > > > I'm not talking about this. There is no evidence that Dmitry gives
> > > > you any approval to use or clear SoB tag. Again, you may not do like
> > this.
> > >
> > > Here Dmitry is acknowledging, he is ok with the patch I posted.
> > >
> > 
> > No, you just misinterpreted his message.
> > 
> 
> Dmitry,
> 
> As you are the author of code, either you post a patch or provide your SoB as per the guideline mentioned here to avoid confusion.
> 
>  https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

It was not really proper patch, consider it as an email with parts
written in unified diff, as sometimes it is easier than to explain in
words, and I do not want to take much credit for it.

If you wish you can put "Suggested-by" for me, or just drop my name off
the patch description altogether.

Thanks.

-- 
Dmitry

RE: [PATCH RFC 1/2] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

> Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> device_get_match_data() to struct bus_type
> 
> On Mon, Jul 24, 2023 at 01:58:55PM +0000, Biju Das wrote:
> > Hi Andy,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > device_get_match_data() to struct bus_type
> > >
> > > On Mon, Jul 24, 2023 at 01:19:02PM +0000, Biju Das wrote:
> > > > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > > > device_get_match_data() to struct bus_type On Mon, Jul 24, 2023
> > > > > at 12:02:27PM +0000, Biju Das wrote:
> > > > > > > Subject: Re: [PATCH RFC 1/2] drivers: fwnode: Extend
> > > > > > > device_get_match_data() to struct bus_type On Sun, Jul 23,
> > > > > > > 2023 at 09:37:20AM +0100, Biju Das wrote:
> > >
> > > ...
> > >
> > > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > > >
> > > > > > > You can't just throw one's SoB tag without clear
> > > > > > > understanding what's going on here (either wrong authorship
> > > > > > > or missing Co-developed-by or...?).
> > > > > >
> > > > > > Dmitry feels instead of having separate bus based match_data()
> > > > > > like i2c_get_match_data[2] and spi_get_device_match_data[3],
> > > > > > it is better to have a generic approach like a single API
> > > > > > device_get_match_data() for getting match_data for
> > > > > > OF/ACPI/I2C/SPI
> > > tables.
> > > > > >
> > > > > > So, he came with a proposal and shared some code here[1].
> > > > >
> > > > > Yes, I'm pretty much following the discussion.
> > > > >
> > > > > > Since,I have send this patch, I put my signed -off.
> > > > >
> > > > > I'm not talking about this. There is no evidence that Dmitry
> > > > > gives you any approval to use or clear SoB tag. Again, you may
> > > > > not do like
> > > this.
> > > >
> > > > Here Dmitry is acknowledging, he is ok with the patch I posted.
> > > >
> > >
> > > No, you just misinterpreted his message.
> > >
> >
> > Dmitry,
> >
> > As you are the author of code, either you post a patch or provide your
> SoB as per the guideline mentioned here to avoid confusion.
> >
> >
> It was not really proper patch, consider it as an email with parts
> written in unified diff, as sometimes it is easier than to explain in
> words, and I do not want to take much credit for it.
> 
> If you wish you can put "Suggested-by" for me, or just drop my name off
> the patch description altogether.

Sure, will add Suggested-by tag.

Cheers,
Biju

[PATCH RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Biju Das]

Add i2c_device_get_match_data() callback to struct bus_type().

While at it, introduced i2c_get_match_data_helper() to avoid code
duplication with i2c_get_match_data().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/i2c-core-base.c | 38 +++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..80259111355b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 }
 EXPORT_SYMBOL_GPL(i2c_match_id);
 
+static void *i2c_get_match_data_helper(struct i2c_driver *driver,
+				       const struct i2c_client *client)
+{
+	const struct i2c_device_id *match;
+
+	match = i2c_match_id(driver->id_table, client);
+	if (!match)
+		return NULL;
+
+	return (const void *)match->driver_data;
+}
+
+static const void *i2c_device_get_match_data(const struct device *dev)
+{
+	const struct i2c_client *client = to_i2c_client(dev);
+	const struct i2c_driver *driver;
+
+	if (!dev->driver)
+		return NULL;
+
+	driver = to_i2c_driver(dev->driver);
+	if (!driver)
+		return NULL;
+
+	return i2c_get_match_data_helper(driver, client);
+}
+
 const void *i2c_get_match_data(const struct i2c_client *client)
 {
 	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
-	const struct i2c_device_id *match;
 	const void *data;
 
 	data = device_get_match_data(&client->dev);
-	if (!data) {
-		match = i2c_match_id(driver->id_table, client);
-		if (!match)
-			return NULL;
-
-		data = (const void *)match->driver_data;
-	}
+	if (!data)
+		data = i2c_get_match_data_helper(driver, client);
 
 	return data;
 }
@@ -695,6 +716,7 @@ struct bus_type i2c_bus_type = {
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
 	.shutdown	= i2c_device_shutdown,
+	.get_match_data	= i2c_device_get_match_data,
 };
 EXPORT_SYMBOL_GPL(i2c_bus_type);
 
-- 
2.25.1

Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Dmitry Torokhov]

On Sun, Jul 23, 2023 at 09:37:21AM +0100, Biju Das wrote:
> Add i2c_device_get_match_data() callback to struct bus_type().
> 
> While at it, introduced i2c_get_match_data_helper() to avoid code
> duplication with i2c_get_match_data().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/i2c/i2c-core-base.c | 38 +++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 60746652fd52..80259111355b 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
>  }
>  EXPORT_SYMBOL_GPL(i2c_match_id);
>  
> +static void *i2c_get_match_data_helper(struct i2c_driver *driver,
> +				       const struct i2c_client *client)
> +{
> +	const struct i2c_device_id *match;
> +
> +	match = i2c_match_id(driver->id_table, client);
> +	if (!match)
> +		return NULL;
> +
> +	return (const void *)match->driver_data;

Not sure if this helper is needed given that in the end (hopefully) we
can simply remove i2c_get_match_data() and leave only
i2c_device_get_match_data().

i2c_get_match_data() and leave only i2c_device_get_match_data().

Thanks.

-- 
Dmitry

Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Geert Uytterhoeven]

Hi Biju,

On Sun, Jul 23, 2023 at 10:37 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add i2c_device_get_match_data() callback to struct bus_type().
>
> While at it, introduced i2c_get_match_data_helper() to avoid code
> duplication with i2c_get_match_data().
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
>  }
>  EXPORT_SYMBOL_GPL(i2c_match_id);
>
> +static void *i2c_get_match_data_helper(struct i2c_driver *driver,

static const void *

> +                                      const struct i2c_client *client)
> +{
> +       const struct i2c_device_id *match;
> +
> +       match = i2c_match_id(driver->id_table, client);
> +       if (!match)
> +               return NULL;
> +
> +       return (const void *)match->driver_data;

I guess your compiler didn't complain about the const/non-const
conversion when returning because it inlined the function?

> +}
> +
> +static const void *i2c_device_get_match_data(const struct device *dev)
> +{
> +       const struct i2c_client *client = to_i2c_client(dev);
> +       const struct i2c_driver *driver;
> +
> +       if (!dev->driver)
> +               return NULL;
> +
> +       driver = to_i2c_driver(dev->driver);
> +       if (!driver)
> +               return NULL;

"driver" can never be NULL here.

> +
> +       return i2c_get_match_data_helper(driver, client);
> +}
> +

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 RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data()
> callback
> 
> Hi Biju,
> 
> On Sun, Jul 23, 2023 at 10:37 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add i2c_device_get_match_data() callback to struct bus_type().
> >
> > While at it, introduced i2c_get_match_data_helper() to avoid code
> > duplication with i2c_get_match_data().
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const
> > struct i2c_device_id *id,  }  EXPORT_SYMBOL_GPL(i2c_match_id);
> >
> > +static void *i2c_get_match_data_helper(struct i2c_driver *driver,
> 
> static const void *

I missed this.

> 
> > +                                      const struct i2c_client
> > +*client) {
> > +       const struct i2c_device_id *match;
> > +
> > +       match = i2c_match_id(driver->id_table, client);
> > +       if (!match)
> > +               return NULL;
> > +
> > +       return (const void *)match->driver_data;
> 
> I guess your compiler didn't complain about the const/non-const
> conversion when returning because it inlined the function?

It complained. Somehow, I didn't notice that warning before sending the patch.

> 
> > +}
> > +
> > +static const void *i2c_device_get_match_data(const struct device
> > +*dev) {
> > +       const struct i2c_client *client = to_i2c_client(dev);

Not sure, non-const i2c_verify_client(dev)to be used here??

> > +       const struct i2c_driver *driver;
> > +
> > +       if (!dev->driver)
> > +               return NULL;
> > +
> > +       driver = to_i2c_driver(dev->driver);
> > +       if (!driver)
> > +               return NULL;
> 
> "driver" can never be NULL here.

OK, will remove this.

Cheers,
Biju

> 
> > +
> > +       return i2c_get_match_data_helper(driver, client); }
> > +
> 

Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Dmitry Torokhov]

On Mon, Jul 24, 2023 at 03:06:50PM +0000, Biju Das wrote:
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data()
> > callback
> > 
> > Hi Biju,
> > 
> > On Sun, Jul 23, 2023 at 10:37 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > Add i2c_device_get_match_data() callback to struct bus_type().
> > >
> > > While at it, introduced i2c_get_match_data_helper() to avoid code
> > > duplication with i2c_get_match_data().
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > 
> > Thanks for your patch!
> > 
> > > --- a/drivers/i2c/i2c-core-base.c
> > > +++ b/drivers/i2c/i2c-core-base.c
> > > @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const
> > > struct i2c_device_id *id,  }  EXPORT_SYMBOL_GPL(i2c_match_id);
> > >
> > > +static void *i2c_get_match_data_helper(struct i2c_driver *driver,
> > 
> > static const void *
> 
> I missed this.
> 
> > 
> > > +                                      const struct i2c_client
> > > +*client) {
> > > +       const struct i2c_device_id *match;
> > > +
> > > +       match = i2c_match_id(driver->id_table, client);
> > > +       if (!match)
> > > +               return NULL;
> > > +
> > > +       return (const void *)match->driver_data;
> > 
> > I guess your compiler didn't complain about the const/non-const
> > conversion when returning because it inlined the function?
> 
> It complained. Somehow, I didn't notice that warning before sending the patch.
> 
> > 
> > > +}
> > > +
> > > +static const void *i2c_device_get_match_data(const struct device
> > > +*dev) {
> > > +       const struct i2c_client *client = to_i2c_client(dev);
> 
> Not sure, non-const i2c_verify_client(dev)to be used here??

Good call, it actually should, as i2c bus contains instances of both
i2c_client and i2c_adapter.

Unfortunately i2c_verify_client() right now is a function, we might need
to turn it into a macro to allow transparently handle const/non-const
device argument... If this is too hard at the moment we could open-code
i2c_verify_client() in i2c_device_get_match_data() and first check on
the device type before doing to_i2c_client() conversion.

Also I see i2c_device_remove() uses to_i2c_client(). I guess it only
works because i2c adapters never have drivers assigned to them, but it
would be nice to use i2c_verify_client() and maybe put a big fat warning
if we get wrong type of device there.

Thanks.

-- 
Dmitry

Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Geert Uytterhoeven]

Hi Dmitry,

On Mon, Jul 24, 2023 at 6:35 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jul 24, 2023 at 03:06:50PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data()
> > > callback
> > > On Sun, Jul 23, 2023 at 10:37 AM Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > Add i2c_device_get_match_data() callback to struct bus_type().
> > > >
> > > > While at it, introduced i2c_get_match_data_helper() to avoid code
> > > > duplication with i2c_get_match_data().
> > > >
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/i2c/i2c-core-base.c
> > > > +++ b/drivers/i2c/i2c-core-base.c
> > > > @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const
> > > > struct i2c_device_id *id,  }  EXPORT_SYMBOL_GPL(i2c_match_id);
> > > >
> > > > +static void *i2c_get_match_data_helper(struct i2c_driver *driver,
> > >
> > > static const void *
> >
> > I missed this.
> >
> > >
> > > > +                                      const struct i2c_client
> > > > +*client) {
> > > > +       const struct i2c_device_id *match;
> > > > +
> > > > +       match = i2c_match_id(driver->id_table, client);
> > > > +       if (!match)
> > > > +               return NULL;
> > > > +
> > > > +       return (const void *)match->driver_data;
> > >
> > > I guess your compiler didn't complain about the const/non-const
> > > conversion when returning because it inlined the function?
> >
> > It complained. Somehow, I didn't notice that warning before sending the patch.
> >
> > >
> > > > +}
> > > > +
> > > > +static const void *i2c_device_get_match_data(const struct device
> > > > +*dev) {
> > > > +       const struct i2c_client *client = to_i2c_client(dev);
> >
> > Not sure, non-const i2c_verify_client(dev)to be used here??
>
> Good call, it actually should, as i2c bus contains instances of both
> i2c_client and i2c_adapter.
>
> Unfortunately i2c_verify_client() right now is a function, we might need
> to turn it into a macro to allow transparently handle const/non-const
> device argument... If this is too hard at the moment we could open-code
> i2c_verify_client() in i2c_device_get_match_data() and first check on
> the device type before doing to_i2c_client() conversion.

Tadah, we have _Generic()! See container_of_const():
https://elixir.bootlin.com/linux/latest/source/include/linux/container_of.h#L25

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 RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Dmitry Torokhov]

On Mon, Jul 24, 2023 at 06:43:30PM +0200, Geert Uytterhoeven wrote:
> Hi Dmitry,
> 
> On Mon, Jul 24, 2023 at 6:35 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Jul 24, 2023 at 03:06:50PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data()
> > > > callback
> > > > On Sun, Jul 23, 2023 at 10:37 AM Biju Das <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > Add i2c_device_get_match_data() callback to struct bus_type().
> > > > >
> > > > > While at it, introduced i2c_get_match_data_helper() to avoid code
> > > > > duplication with i2c_get_match_data().
> > > > >
> > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/i2c/i2c-core-base.c
> > > > > +++ b/drivers/i2c/i2c-core-base.c
> > > > > @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const
> > > > > struct i2c_device_id *id,  }  EXPORT_SYMBOL_GPL(i2c_match_id);
> > > > >
> > > > > +static void *i2c_get_match_data_helper(struct i2c_driver *driver,
> > > >
> > > > static const void *
> > >
> > > I missed this.
> > >
> > > >
> > > > > +                                      const struct i2c_client
> > > > > +*client) {
> > > > > +       const struct i2c_device_id *match;
> > > > > +
> > > > > +       match = i2c_match_id(driver->id_table, client);
> > > > > +       if (!match)
> > > > > +               return NULL;
> > > > > +
> > > > > +       return (const void *)match->driver_data;
> > > >
> > > > I guess your compiler didn't complain about the const/non-const
> > > > conversion when returning because it inlined the function?
> > >
> > > It complained. Somehow, I didn't notice that warning before sending the patch.
> > >
> > > >
> > > > > +}
> > > > > +
> > > > > +static const void *i2c_device_get_match_data(const struct device
> > > > > +*dev) {
> > > > > +       const struct i2c_client *client = to_i2c_client(dev);
> > >
> > > Not sure, non-const i2c_verify_client(dev)to be used here??
> >
> > Good call, it actually should, as i2c bus contains instances of both
> > i2c_client and i2c_adapter.
> >
> > Unfortunately i2c_verify_client() right now is a function, we might need
> > to turn it into a macro to allow transparently handle const/non-const
> > device argument... If this is too hard at the moment we could open-code
> > i2c_verify_client() in i2c_device_get_match_data() and first check on
> > the device type before doing to_i2c_client() conversion.
> 
> Tadah, we have _Generic()! See container_of_const():
> https://elixir.bootlin.com/linux/latest/source/include/linux/container_of.h#L25

I think we might want to go through all buses and switch to_XXX()
implementations there to use container_of_const() instead of old
container_of() there.

This still does not help with i2c_verify_client() being currently a
function, so it still needs to be converted to a macro to be able to
return const and nont-const pointers, depending on the argument type.

Thanks.

-- 
Dmitry