[7/7] OF/ACPI/I2C: Add generic match function for the aforementioned systems

Currently this is a helper function for the I2C subsystem to aid the
matching of non-standard compatible strings and devices which use DT
and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
it has been made more generic as it can be used to only make one call
for drivers which support any mixture of OF, ACPI and/or I2C matching.

The initial aim is for of_match_device() to be replaced by this call
in all I2C device drivers.

[1] Documentation/i2c/instantiating-devices

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 include/linux/match.h

On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> Currently this is a helper function for the I2C subsystem to aid the
> matching of non-standard compatible strings and devices which use DT
> and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> it has been made more generic as it can be used to only make one call
> for drivers which support any mixture of OF, ACPI and/or I2C matching.
> 
> The initial aim is for of_match_device() to be replaced by this call
> in all I2C device drivers.
> 
> [1] Documentation/i2c/instantiating-devices
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Mika, can you please have a look at this, please?


> ---
>  include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 include/linux/match.h
> 
> diff --git a/include/linux/match.h b/include/linux/match.h
> new file mode 100644
> index 0000000..20a08e2
> --- /dev/null
> +++ b/include/linux/match.h
> @@ -0,0 +1,40 @@
> +#include <linux/of.h>
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +
> +static void *device_match(struct device *dev)
> +{
> +	struct device_driver *driver = dev->driver;
> +
> +	if (!driver)
> +		return NULL;
> +
> +	/* Attempt an OF style match */
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		const struct of_device_id *of_match =
> +			i2c_of_match_device(driver->of_match_table, dev);
> +		if (of_match)
> +			return (void *)of_match;
> +	}
> +
> +	/* Then ACPI style match */
> +	if (IS_ENABLED(CONFIG_ACPI)) {
> +		const struct acpi_device_id *acpi_match	=
> +			acpi_match_device(driver->acpi_match_table, dev);
> +		if (acpi_match)
> +			return (void *)acpi_match;
> +	}
> +
> +	/* Finally an I2C match */
> +	if (IS_ENABLED(CONFIG_I2C)) {
> +		struct i2c_client *client = i2c_verify_client(dev);
> +		struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> +		struct i2c_device_id *i2c_match;
> +
> +		i2c_match = i2c_match_id(i2c_drv->id_table, client);
> +		if (i2c_match)
> +			return (void *)i2c_match;
> +	}
> +
> +	return NULL;
> +}
>

On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > Currently this is a helper function for the I2C subsystem to aid the
> > matching of non-standard compatible strings and devices which use DT
> > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> > it has been made more generic as it can be used to only make one call
> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > 
> > The initial aim is for of_match_device() to be replaced by this call
> > in all I2C device drivers.
> > 
> > [1] Documentation/i2c/instantiating-devices
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Mika, can you please have a look at this, please?

I don't see any fundamental problems with this wrt. ACPI.

That said, I find it kind of weird to have generic function that then
has knowledge of how different buses do their matching.

I would rather see something like firmware_device_match(dev) that goes
and matches from DT/ACPI and leave bus specific match to happen internal
to that bus.

> 
> 
> > ---
> >  include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >  create mode 100644 include/linux/match.h
> > 
> > diff --git a/include/linux/match.h b/include/linux/match.h
> > new file mode 100644
> > index 0000000..20a08e2
> > --- /dev/null
> > +++ b/include/linux/match.h
> > @@ -0,0 +1,40 @@
> > +#include <linux/of.h>
> > +#include <linux/acpi.h>
> > +#include <linux/i2c.h>
> > +
> > +static void *device_match(struct device *dev)
> > +{
> > +	struct device_driver *driver = dev->driver;
> > +
> > +	if (!driver)
> > +		return NULL;
> > +
> > +	/* Attempt an OF style match */
> > +	if (IS_ENABLED(CONFIG_OF)) {
> > +		const struct of_device_id *of_match =
> > +			i2c_of_match_device(driver->of_match_table, dev);
> > +		if (of_match)
> > +			return (void *)of_match;
> > +	}
> > +
> > +	/* Then ACPI style match */
> > +	if (IS_ENABLED(CONFIG_ACPI)) {
> > +		const struct acpi_device_id *acpi_match	=
> > +			acpi_match_device(driver->acpi_match_table, dev);
> > +		if (acpi_match)
> > +			return (void *)acpi_match;
> > +	}
> > +
> > +	/* Finally an I2C match */
> > +	if (IS_ENABLED(CONFIG_I2C)) {
> > +		struct i2c_client *client = i2c_verify_client(dev);
> > +		struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> > +		struct i2c_device_id *i2c_match;
> > +
> > +		i2c_match = i2c_match_id(i2c_drv->id_table, client);
> > +		if (i2c_match)
> > +			return (void *)i2c_match;
> > +	}
> > +
> > +	return NULL;
> > +}
> > 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 04 Jun 2014, Mika Westerberg wrote:
> On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > Currently this is a helper function for the I2C subsystem to aid the
> > > matching of non-standard compatible strings and devices which use DT
> > > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> > > it has been made more generic as it can be used to only make one call
> > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > 
> > > The initial aim is for of_match_device() to be replaced by this call
> > > in all I2C device drivers.
> > > 
> > > [1] Documentation/i2c/instantiating-devices
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Mika, can you please have a look at this, please?
> 
> I don't see any fundamental problems with this wrt. ACPI.
> 
> That said, I find it kind of weird to have generic function that then
> has knowledge of how different buses do their matching.
> 
> I would rather see something like firmware_device_match(dev) that goes
> and matches from DT/ACPI and leave bus specific match to happen internal
> to that bus.

Unfortunately that completely defeats the object of the patch.  When a
of_match_device() is invoked it solely looks up devices based on OF
matching, but I2C is special in that devices can be registered via
sysfs, thus will no have device node.  If of_match_device() is called
in one of these instances it will fail.  The idea of this patch is to
generify the matching into something does has the knowledge to firstly
attempt a traditional match, and if that fails will fall back to a
special i2c_{of,acpi}_match_device() which knows how to deal with
node-less registration.

We don't support that for ACPI yet, as I don't have a system to test
it on, but when we do acpi_match_device() in the patch will too be
swapped out for an equivalent i2c_acpi_match_device().

Actually, I've just spotted that this patch is wrong I need to change
it in the following way:

> > > ---
> > >  include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 40 insertions(+)
> > >  create mode 100644 include/linux/match.h
> > > 
> > > diff --git a/include/linux/match.h b/include/linux/match.h
> > > new file mode 100644
> > > index 0000000..20a08e2
> > > --- /dev/null
> > > +++ b/include/linux/match.h
> > > @@ -0,0 +1,40 @@
> > > +#include <linux/of.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/i2c.h>
> > > +
> > > +static void *device_match(struct device *dev)
> > > +{
> > > +	struct device_driver *driver = dev->driver;
> > > +
> > > +	if (!driver)
> > > +		return NULL;
> > > +
> > > +	/* Attempt an OF style match */
> > > +	if (IS_ENABLED(CONFIG_OF)) {
> > > +		const struct of_device_id *of_match =
> > > +			i2c_of_match_device(driver->of_match_table, dev);

This should be of_match_device()

> > > +		if (of_match)
> > > +			return (void *)of_match;
> > > +	}
> > > +
> > > +	/* Then ACPI style match */
> > > +	if (IS_ENABLED(CONFIG_ACPI)) {
> > > +		const struct acpi_device_id *acpi_match	=
> > > +			acpi_match_device(driver->acpi_match_table, dev);
> > > +		if (acpi_match)
> > > +			return (void *)acpi_match;
> > > +	}
> > > +
> > > +	/* Finally an I2C match */
> > > +	if (IS_ENABLED(CONFIG_I2C)) {
> > > +		struct i2c_client *client = i2c_verify_client(dev);
> > > +		struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> > > +		struct i2c_device_id *i2c_match;

i2c_of_match_device() and later i2c_acpi_match_device() should be here.

> > > +		i2c_match = i2c_match_id(i2c_drv->id_table, client);
> > > +		if (i2c_match)
> > > +			return (void *)i2c_match;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > 
> >

On Wed, Jun 04, 2014 at 02:28:20PM +0100, Lee Jones wrote:
> On Wed, 04 Jun 2014, Mika Westerberg wrote:
> > On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > > Currently this is a helper function for the I2C subsystem to aid the
> > > > matching of non-standard compatible strings and devices which use DT
> > > > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> > > > it has been made more generic as it can be used to only make one call
> > > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > > 
> > > > The initial aim is for of_match_device() to be replaced by this call
> > > > in all I2C device drivers.
> > > > 
> > > > [1] Documentation/i2c/instantiating-devices
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > 
> > > Mika, can you please have a look at this, please?
> > 
> > I don't see any fundamental problems with this wrt. ACPI.
> > 
> > That said, I find it kind of weird to have generic function that then
> > has knowledge of how different buses do their matching.
> > 
> > I would rather see something like firmware_device_match(dev) that goes
> > and matches from DT/ACPI and leave bus specific match to happen internal
> > to that bus.
> 
> Unfortunately that completely defeats the object of the patch.  When a
> of_match_device() is invoked it solely looks up devices based on OF
> matching, but I2C is special in that devices can be registered via
> sysfs, thus will no have device node.  If of_match_device() is called
> in one of these instances it will fail.  The idea of this patch is to
> generify the matching into something does has the knowledge to firstly
> attempt a traditional match, and if that fails will fall back to a
> special i2c_{of,acpi}_match_device() which knows how to deal with
> node-less registration.

OK, then but since this is now I2C specific, why call it device_match()
instead of something like i2c_device_match()? Or do you have plans to
add there more knowledge about other buses like SPI and PCI to name few?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 05 Jun 2014, Mika Westerberg wrote:
> On Wed, Jun 04, 2014 at 02:28:20PM +0100, Lee Jones wrote:
> > On Wed, 04 Jun 2014, Mika Westerberg wrote:
> > > On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > > > Currently this is a helper function for the I2C subsystem to aid the
> > > > > matching of non-standard compatible strings and devices which use DT
> > > > > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> > > > > it has been made more generic as it can be used to only make one call
> > > > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > > > 
> > > > > The initial aim is for of_match_device() to be replaced by this call
> > > > > in all I2C device drivers.
> > > > > 
> > > > > [1] Documentation/i2c/instantiating-devices
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > 
> > > > Mika, can you please have a look at this, please?
> > > 
> > > I don't see any fundamental problems with this wrt. ACPI.
> > > 
> > > That said, I find it kind of weird to have generic function that then
> > > has knowledge of how different buses do their matching.
> > > 
> > > I would rather see something like firmware_device_match(dev) that goes
> > > and matches from DT/ACPI and leave bus specific match to happen internal
> > > to that bus.
> > 
> > Unfortunately that completely defeats the object of the patch.  When a
> > of_match_device() is invoked it solely looks up devices based on OF
> > matching, but I2C is special in that devices can be registered via
> > sysfs, thus will no have device node.  If of_match_device() is called
> > in one of these instances it will fail.  The idea of this patch is to
> > generify the matching into something does has the knowledge to firstly
> > attempt a traditional match, and if that fails will fall back to a
> > special i2c_{of,acpi}_match_device() which knows how to deal with
> > node-less registration.
> 
> OK, then but since this is now I2C specific, why call it device_match()
> instead of something like i2c_device_match()? Or do you have plans to

So in an early incarnation of the patch I did just that, and it might
not actually be such a bad idea still - I'm open to other people's
opinions on this.

> add there more knowledge about other buses like SPI and PCI to name few?

... but yes, this is the new idea - that it can be expanded as required.

On Wed,  4 Jun 2014 13:09:56 +0100, Lee Jones <lee.jones@linaro.org> wrote:
> Currently this is a helper function for the I2C subsystem to aid the
> matching of non-standard compatible strings and devices which use DT
> and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> it has been made more generic as it can be used to only make one call
> for drivers which support any mixture of OF, ACPI and/or I2C matching.
> 
> The initial aim is for of_match_device() to be replaced by this call
> in all I2C device drivers.
> 
> [1] Documentation/i2c/instantiating-devices
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

I don't like this. It drops all type safety on the match entry
and the caller has no idea what it got back.

g.

> ---
>  include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 include/linux/match.h
> 
> diff --git a/include/linux/match.h b/include/linux/match.h
> new file mode 100644
> index 0000000..20a08e2
> --- /dev/null
> +++ b/include/linux/match.h
> @@ -0,0 +1,40 @@
> +#include <linux/of.h>
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +
> +static void *device_match(struct device *dev)
> +{
> +	struct device_driver *driver = dev->driver;
> +
> +	if (!driver)
> +		return NULL;
> +
> +	/* Attempt an OF style match */
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		const struct of_device_id *of_match =
> +			i2c_of_match_device(driver->of_match_table, dev);
> +		if (of_match)
> +			return (void *)of_match;
> +	}
> +
> +	/* Then ACPI style match */
> +	if (IS_ENABLED(CONFIG_ACPI)) {
> +		const struct acpi_device_id *acpi_match	=
> +			acpi_match_device(driver->acpi_match_table, dev);
> +		if (acpi_match)
> +			return (void *)acpi_match;
> +	}
> +
> +	/* Finally an I2C match */
> +	if (IS_ENABLED(CONFIG_I2C)) {
> +		struct i2c_client *client = i2c_verify_client(dev);
> +		struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> +		struct i2c_device_id *i2c_match;
> +
> +		i2c_match = i2c_match_id(i2c_drv->id_table, client);
> +		if (i2c_match)
> +			return (void *)i2c_match;
> +	}
> +
> +	return NULL;
> +}
> -- 
> 1.8.3.2
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 5 Jun 2014 09:20:08 +0100, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 05 Jun 2014, Mika Westerberg wrote:
> > On Wed, Jun 04, 2014 at 02:28:20PM +0100, Lee Jones wrote:
> > > On Wed, 04 Jun 2014, Mika Westerberg wrote:
> > > > On Wed, Jun 04, 2014 at 02:37:42PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wednesday, June 04, 2014 01:09:56 PM Lee Jones wrote:
> > > > > > Currently this is a helper function for the I2C subsystem to aid the
> > > > > > matching of non-standard compatible strings and devices which use DT
> > > > > > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> > > > > > it has been made more generic as it can be used to only make one call
> > > > > > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > > > > > 
> > > > > > The initial aim is for of_match_device() to be replaced by this call
> > > > > > in all I2C device drivers.
> > > > > > 
> > > > > > [1] Documentation/i2c/instantiating-devices
> > > > > > 
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > 
> > > > > Mika, can you please have a look at this, please?
> > > > 
> > > > I don't see any fundamental problems with this wrt. ACPI.
> > > > 
> > > > That said, I find it kind of weird to have generic function that then
> > > > has knowledge of how different buses do their matching.
> > > > 
> > > > I would rather see something like firmware_device_match(dev) that goes
> > > > and matches from DT/ACPI and leave bus specific match to happen internal
> > > > to that bus.
> > > 
> > > Unfortunately that completely defeats the object of the patch.  When a
> > > of_match_device() is invoked it solely looks up devices based on OF
> > > matching, but I2C is special in that devices can be registered via
> > > sysfs, thus will no have device node.  If of_match_device() is called
> > > in one of these instances it will fail.  The idea of this patch is to
> > > generify the matching into something does has the knowledge to firstly
> > > attempt a traditional match, and if that fails will fall back to a
> > > special i2c_{of,acpi}_match_device() which knows how to deal with
> > > node-less registration.
> > 
> > OK, then but since this is now I2C specific, why call it device_match()
> > instead of something like i2c_device_match()? Or do you have plans to
> 
> So in an early incarnation of the patch I did just that, and it might
> not actually be such a bad idea still - I'm open to other people's
> opinions on this.
> 
> > add there more knowledge about other buses like SPI and PCI to name few?
> 
> ... but yes, this is the new idea - that it can be expanded as required.

The whole point of this series is to deal with a special use case of i2c
that we don't need to support for the other bus types. We're having to
just through special hoops to make it work and I don't want to expand it
to other bus types if at all possible.

g.

> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 05 Jun 2014, Grant Likely wrote:

> On Wed,  4 Jun 2014 13:09:56 +0100, Lee Jones <lee.jones@linaro.org> wrote:
> > Currently this is a helper function for the I2C subsystem to aid the
> > matching of non-standard compatible strings and devices which use DT
> > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> > it has been made more generic as it can be used to only make one call
> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> > 
> > The initial aim is for of_match_device() to be replaced by this call
> > in all I2C device drivers.
> > 
> > [1] Documentation/i2c/instantiating-devices
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> I don't like this. It drops all type safety on the match entry
> and the caller has no idea what it got back.

Okay, so what's the best way forward?

Introduce a i2c_of_match_device() call instead?

> > ---
> >  include/linux/match.h | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >  create mode 100644 include/linux/match.h
> > 
> > diff --git a/include/linux/match.h b/include/linux/match.h
> > new file mode 100644
> > index 0000000..20a08e2
> > --- /dev/null
> > +++ b/include/linux/match.h
> > @@ -0,0 +1,40 @@
> > +#include <linux/of.h>
> > +#include <linux/acpi.h>
> > +#include <linux/i2c.h>
> > +
> > +static void *device_match(struct device *dev)
> > +{
> > +	struct device_driver *driver = dev->driver;
> > +
> > +	if (!driver)
> > +		return NULL;
> > +
> > +	/* Attempt an OF style match */
> > +	if (IS_ENABLED(CONFIG_OF)) {
> > +		const struct of_device_id *of_match =
> > +			i2c_of_match_device(driver->of_match_table, dev);
> > +		if (of_match)
> > +			return (void *)of_match;
> > +	}
> > +
> > +	/* Then ACPI style match */
> > +	if (IS_ENABLED(CONFIG_ACPI)) {
> > +		const struct acpi_device_id *acpi_match	=
> > +			acpi_match_device(driver->acpi_match_table, dev);
> > +		if (acpi_match)
> > +			return (void *)acpi_match;
> > +	}
> > +
> > +	/* Finally an I2C match */
> > +	if (IS_ENABLED(CONFIG_I2C)) {
> > +		struct i2c_client *client = i2c_verify_client(dev);
> > +		struct i2c_driver *i2c_drv = to_i2c_driver(driver);
> > +		struct i2c_device_id *i2c_match;
> > +
> > +		i2c_match = i2c_match_id(i2c_drv->id_table, client);
> > +		if (i2c_match)
> > +			return (void *)i2c_match;
> > +	}
> > +
> > +	return NULL;
> > +}
>

On Thu, Jun 5, 2014 at 11:37 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:
>
>> On Wed,  4 Jun 2014 13:09:56 +0100, Lee Jones <lee.jones@linaro.org> wrote:
>> > Currently this is a helper function for the I2C subsystem to aid the
>> > matching of non-standard compatible strings and devices which use DT
>> > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
>> > it has been made more generic as it can be used to only make one call
>> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
>> >
>> > The initial aim is for of_match_device() to be replaced by this call
>> > in all I2C device drivers.
>> >
>> > [1] Documentation/i2c/instantiating-devices
>> >
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>
>> I don't like this. It drops all type safety on the match entry
>> and the caller has no idea what it got back.
>
> Okay, so what's the best way forward?
>
> Introduce a i2c_of_match_device() call instead?

I still think the way to do it is to emulate the missing i2c_device_id
when calling the drivers .probe() hook by having a temporary copy on
the stack and filling it with data from the OF or ACPI table....
Actually I would completely skip trying to get the data from ACPI.
Since all of this is to continue supporting instantiating devices from
sysfs, having a fallback to the OF table completely solves that use
case.

Anticipating an objection of: "what do we do with drivers that are
ACPI only?"... That will be an incredibly rare case. If that ever does
happen then we'll solve it by simply adding an of_device_id table.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 05 Jun 2014, Grant Likely wrote:
> On Thu, Jun 5, 2014 at 11:37 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 05 Jun 2014, Grant Likely wrote:
> >
> >> On Wed,  4 Jun 2014 13:09:56 +0100, Lee Jones <lee.jones@linaro.org> wrote:
> >> > Currently this is a helper function for the I2C subsystem to aid the
> >> > matching of non-standard compatible strings and devices which use DT
> >> > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
> >> > it has been made more generic as it can be used to only make one call
> >> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
> >> >
> >> > The initial aim is for of_match_device() to be replaced by this call
> >> > in all I2C device drivers.
> >> >
> >> > [1] Documentation/i2c/instantiating-devices
> >> >
> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>
> >> I don't like this. It drops all type safety on the match entry
> >> and the caller has no idea what it got back.
> >
> > Okay, so what's the best way forward?
> >
> > Introduce a i2c_of_match_device() call instead?
> 
> I still think the way to do it is to emulate the missing i2c_device_id
> when calling the drivers .probe() hook by having a temporary copy on
> the stack and filling it with data from the OF or ACPI table....

That's the opposite of what I'm trying to achieve.  I'm trying to get
rid of unused i2c_device_id tables, rather than reinforce their
mandatory existence.  I think an i2c_of_match_device() with knowledge
of how to match via pure DT principles (of_node/compatible) and a
fall-back, which is able to match on a provided of_device_id table
alone i.e. without the requirement of an existing of_node.

I've also been mulling over the idea of removing the second probe()
parameter, as suggested by Wolfram.  However, this has quite deep
ramifications which would require a great deal of driver adaptions.

> Actually I would completely skip trying to get the data from ACPI.
> Since all of this is to continue supporting instantiating devices from
> sysfs, having a fallback to the OF table completely solves that use
> case.

> Anticipating an objection of: "what do we do with drivers that are
> ACPI only?"... That will be an incredibly rare case. If that ever does
> happen then we'll solve it by simply adding an of_device_id table.

I'm fine with leaving out ACPI support for now.

On Thu, Jun 5, 2014 at 4:55 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:
>> On Thu, Jun 5, 2014 at 11:37 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Thu, 05 Jun 2014, Grant Likely wrote:
>> >
>> >> On Wed,  4 Jun 2014 13:09:56 +0100, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > Currently this is a helper function for the I2C subsystem to aid the
>> >> > matching of non-standard compatible strings and devices which use DT
>> >> > and/or ACPI, but do not supply any nodes (see: [1] Method 4).  However,
>> >> > it has been made more generic as it can be used to only make one call
>> >> > for drivers which support any mixture of OF, ACPI and/or I2C matching.
>> >> >
>> >> > The initial aim is for of_match_device() to be replaced by this call
>> >> > in all I2C device drivers.
>> >> >
>> >> > [1] Documentation/i2c/instantiating-devices
>> >> >
>> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> >>
>> >> I don't like this. It drops all type safety on the match entry
>> >> and the caller has no idea what it got back.
>> >
>> > Okay, so what's the best way forward?
>> >
>> > Introduce a i2c_of_match_device() call instead?
>>
>> I still think the way to do it is to emulate the missing i2c_device_id
>> when calling the drivers .probe() hook by having a temporary copy on
>> the stack and filling it with data from the OF or ACPI table....
>
> That's the opposite of what I'm trying to achieve.  I'm trying to get
> rid of unused i2c_device_id tables, rather than reinforce their
> mandatory existence.  I think an i2c_of_match_device() with knowledge
> of how to match via pure DT principles (of_node/compatible) and a
> fall-back, which is able to match on a provided of_device_id table
> alone i.e. without the requirement of an existing of_node.

What I'm suggesting does allow all those tables to be removed, but
without a painful API breakage... having said that, it is only the
drivers that drop their i2c_device_id tables that need to support the
fallback behaviour, so it would be fine to pass null into the
i2c_device_id argument and make the driver call the new function that
returns an of_device_id regardless of whether a node is attached.

> I've also been mulling over the idea of removing the second probe()
> parameter, as suggested by Wolfram.  However, this has quite deep
> ramifications which would require a great deal of driver adaptions.

That is of course the ideal. It would be a lot of work (I count 633
users), but it would get i2c exactly where you want it to be. I did
that kind of work when I merge platform_bus_type with
of_platform_bus_type.

You can mitigate it though by creating a .probe2 hook that doesn't
have the parameter and then change over all the users in separate
commits, finally removing the old hook when safe to do so.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote:
> On Thu, 05 Jun 2014, Grant Likely wrote:

> > I still think the way to do it is to emulate the missing i2c_device_id
> > when calling the drivers .probe() hook by having a temporary copy on
> > the stack and filling it with data from the OF or ACPI table....

> That's the opposite of what I'm trying to achieve.  I'm trying to get
> rid of unused i2c_device_id tables, rather than reinforce their
> mandatory existence.  I think an i2c_of_match_device() with knowledge
> of how to match via pure DT principles (of_node/compatible) and a
> fall-back, which is able to match on a provided of_device_id table
> alone i.e. without the requirement of an existing of_node.

> I've also been mulling over the idea of removing the second probe()
> parameter, as suggested by Wolfram.  However, this has quite deep
> ramifications which would require a great deal of driver adaptions.

If you're going to do that another option is to refactor the probe()
function to take the driver_data as an argument and then have the core
pass that from whatever table it matched from rather than the entire
i2c_device_id structure.  That way the driver just needs to supply all
the ID tables mapping binding information to whatever it needs and the
core can pass in the driver data from whatever table it matched against.

On Fri, 06 Jun 2014, Mark Brown wrote:

> On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote:
> > On Thu, 05 Jun 2014, Grant Likely wrote:
> 
> > > I still think the way to do it is to emulate the missing i2c_device_id
> > > when calling the drivers .probe() hook by having a temporary copy on
> > > the stack and filling it with data from the OF or ACPI table....
> 
> > That's the opposite of what I'm trying to achieve.  I'm trying to get
> > rid of unused i2c_device_id tables, rather than reinforce their
> > mandatory existence.  I think an i2c_of_match_device() with knowledge
> > of how to match via pure DT principles (of_node/compatible) and a
> > fall-back, which is able to match on a provided of_device_id table
> > alone i.e. without the requirement of an existing of_node.
> 
> > I've also been mulling over the idea of removing the second probe()
> > parameter, as suggested by Wolfram.  However, this has quite deep
> > ramifications which would require a great deal of driver adaptions.
> 
> If you're going to do that another option is to refactor the probe()
> function to take the driver_data as an argument and then have the core
> pass that from whatever table it matched from rather than the entire
> i2c_device_id structure.  That way the driver just needs to supply all
> the ID tables mapping binding information to whatever it needs and the
> core can pass in the driver data from whatever table it matched against.

Unfortunately this means we're back to the aforementioned typing
issue.  For struct {platform,i2c,spi,acpi,etc}_device_id the driver
data is a kernel ulong but the of_device_id's driver data attribute is
a void*.

I've just started work on a migration over to a new probe().  I don't
think it's all that much work, but if there are any objections I'd
prefer to hear them now rather than waste any time.

I propose to convert a couple of drivers, one which doesn't use the
driver_data and one that does, but is DT only and send them for
review.  See if Wolfram et. al like the method.

On Fri, 6 Jun 2014 13:36:53 +0100, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 06 Jun 2014, Mark Brown wrote:
> 
> > On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote:
> > > On Thu, 05 Jun 2014, Grant Likely wrote:
> > 
> > > > I still think the way to do it is to emulate the missing i2c_device_id
> > > > when calling the drivers .probe() hook by having a temporary copy on
> > > > the stack and filling it with data from the OF or ACPI table....
> > 
> > > That's the opposite of what I'm trying to achieve.  I'm trying to get
> > > rid of unused i2c_device_id tables, rather than reinforce their
> > > mandatory existence.  I think an i2c_of_match_device() with knowledge
> > > of how to match via pure DT principles (of_node/compatible) and a
> > > fall-back, which is able to match on a provided of_device_id table
> > > alone i.e. without the requirement of an existing of_node.
> > 
> > > I've also been mulling over the idea of removing the second probe()
> > > parameter, as suggested by Wolfram.  However, this has quite deep
> > > ramifications which would require a great deal of driver adaptions.
> > 
> > If you're going to do that another option is to refactor the probe()
> > function to take the driver_data as an argument and then have the core
> > pass that from whatever table it matched from rather than the entire
> > i2c_device_id structure.  That way the driver just needs to supply all
> > the ID tables mapping binding information to whatever it needs and the
> > core can pass in the driver data from whatever table it matched against.
> 
> Unfortunately this means we're back to the aforementioned typing
> issue.  For struct {platform,i2c,spi,acpi,etc}_device_id the driver
> data is a kernel ulong but the of_device_id's driver data attribute is
> a void*.

We're actually okay there. Each subsystem defines it's own convention
about what those values mean. ulong and void* are the same size and
every user I've seen stuffs the same data into the data field of both
tables.

> I've just started work on a migration over to a new probe().  I don't
> think it's all that much work, but if there are any objections I'd
> prefer to hear them now rather than waste any time.

I have no problem with that approach.

> I propose to convert a couple of drivers, one which doesn't use the
> driver_data and one that does, but is DT only and send them for
> review.  See if Wolfram et. al like the method.
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Jun 07, 2014 at 12:42:53AM +0100, Grant Likely wrote:
> On Fri, 6 Jun 2014 13:36:53 +0100, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 06 Jun 2014, Mark Brown wrote:

> > > > I've also been mulling over the idea of removing the second probe()
> > > > parameter, as suggested by Wolfram.  However, this has quite deep
> > > > ramifications which would require a great deal of driver adaptions.

> > > If you're going to do that another option is to refactor the probe()
> > > function to take the driver_data as an argument and then have the core
> > > pass that from whatever table it matched from rather than the entire
> > > i2c_device_id structure.  That way the driver just needs to supply all
> > > the ID tables mapping binding information to whatever it needs and the
> > > core can pass in the driver data from whatever table it matched against.

> > Unfortunately this means we're back to the aforementioned typing
> > issue.  For struct {platform,i2c,spi,acpi,etc}_device_id the driver
> > data is a kernel ulong but the of_device_id's driver data attribute is
> > a void*.

> We're actually okay there. Each subsystem defines it's own convention
> about what those values mean. ulong and void* are the same size and
> every user I've seen stuffs the same data into the data field of both
> tables.

Indeed - if we're going to go with that approach it seems to me like we
should just pick one and put any casting in the core.

> > I've just started work on a migration over to a new probe().  I don't
> > think it's all that much work, but if there are any objections I'd
> > prefer to hear them now rather than waste any time.

> I have no problem with that approach.

Converting probe() makes sense to me.  I think I would prefer to see the
ID lookup handled as an argument to probe() rather than with functions
in probe() but it's not the end of the world if it isn't.