diff mbox series

[v3,03/16] mfd: mfd-core: match device tree node against reg property

Message ID 20200423174543.17161-4-michael@walle.cc (mailing list archive)
State Not Applicable
Headers show
Series Add support for Kontron sl28cpld | expand

Commit Message

Michael Walle April 23, 2020, 5:45 p.m. UTC
There might be multiple children with the device tree compatible, for
example if a MFD has multiple instances of the same function. In this
case only the first is matched and the other children get a wrong
of_node reference.
Add a new option to match also against the unit address of the child
node. Additonally, a new helper OF_MFD_CELL_REG is added.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
 include/linux/mfd/core.h | 26 ++++++++++++++++++++------
 2 files changed, 40 insertions(+), 15 deletions(-)

Comments

Michael Walle April 29, 2020, 10:18 p.m. UTC | #1
Hi Lee,

Am 2020-04-23 19:45, schrieb Michael Walle:
> There might be multiple children with the device tree compatible, for
> example if a MFD has multiple instances of the same function. In this
> case only the first is matched and the other children get a wrong
> of_node reference.
> Add a new option to match also against the unit address of the child
> node. Additonally, a new helper OF_MFD_CELL_REG is added.


Do you think this is feasible? I guess this is the biggest uncertainty
for me at the moment in this patch series.

-michael

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
>  include/linux/mfd/core.h | 26 ++++++++++++++++++++------
>  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index e735565969b3..4ecb376338f7 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, 
> int id,
>  	struct device_node *np = NULL;
>  	int ret = -ENOMEM;
>  	int platform_id;
> +	u32 of_reg;
>  	int r;
> 
>  	if (id == PLATFORM_DEVID_AUTO)
> @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent, 
> int id,
> 
>  	if (parent->of_node && cell->of_compatible) {
>  		for_each_child_of_node(parent->of_node, np) {
> -			if (of_device_is_compatible(np, cell->of_compatible)) {
> -				if (!of_device_is_available(np)) {
> -					/* Ignore disabled devices error free */
> -					ret = 0;
> -					goto fail_alias;
> -				}
> -				pdev->dev.of_node = np;
> -				pdev->dev.fwnode = &np->fwnode;
> -				break;
> +			if (!of_device_is_compatible(np, cell->of_compatible))
> +				continue;
> +
> +			/* also match the unit address if set */
> +			if (cell->of_reg & MFD_OF_REG_VALID) {
> +				if (of_property_read_u32(np, "reg", &of_reg))
> +					continue;
> +				if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg)
> +					continue;
>  			}
> +
> +			if (!of_device_is_available(np)) {
> +				/* Ignore disabled devices error free */
> +				ret = 0;
> +				goto fail_alias;
> +			}
> +
> +			pdev->dev.of_node = np;
> +			pdev->dev.fwnode = &np->fwnode;
> +			break;
>  		}
>  	}
> 
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index d01d1299e49d..c2c0ad6b14f3 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -13,8 +13,11 @@
>  #include <linux/platform_device.h>
> 
>  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
> +#define MFD_OF_REG_VALID	BIT(31)
> +#define MFD_OF_REG_MASK		GENMASK(30, 0)
> 
> -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, 
> _match)\
> +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
> +		     _of_reg, _match)					\
>  	{								\
>  		.name = (_name),					\
>  		.resources = (_res),					\
> @@ -22,24 +25,32 @@
>  		.platform_data = (_pdata),				\
>  		.pdata_size = (_pdsize),				\
>  		.of_compatible = (_compat),				\
> +		.of_reg = (_of_reg),					\
>  		.acpi_match = (_match),					\
>  		.id = (_id),						\
>  	}
> 
> +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat,	\
> +			_of_reg)					\
> +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
> +		     ((_of_reg) | MFD_OF_REG_VALID), NULL)		\
> +
>  #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat)		\
> -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL)	\
> +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
> +		     0, NULL)						\
> 
>  #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match)	\
> -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match)	\
> +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0,	\
> +		     _match)						\
> 
>  #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id)		\
> -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL)	\
> +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \
> 
>  #define MFD_CELL_RES(_name, _res)					\
> -	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL)		\
> +	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL)		\
> 
>  #define MFD_CELL_NAME(_name)						\
> -	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL)		\
> +	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL)		\
> 
>  struct irq_domain;
>  struct property_entry;
> @@ -78,6 +89,9 @@ struct mfd_cell {
>  	 */
>  	const char		*of_compatible;
> 
> +	/* matching the reg property if set */
> +	unsigned int		of_reg;
> +
>  	/* Matches ACPI */
>  	const struct mfd_cell_acpi_match	*acpi_match;
Lee Jones May 15, 2020, 10:28 a.m. UTC | #2
On Thu, 30 Apr 2020, Michael Walle wrote:

> Hi Lee,
> 
> Am 2020-04-23 19:45, schrieb Michael Walle:
> > There might be multiple children with the device tree compatible, for
> > example if a MFD has multiple instances of the same function. In this
> > case only the first is matched and the other children get a wrong
> > of_node reference.
> > Add a new option to match also against the unit address of the child
> > node. Additonally, a new helper OF_MFD_CELL_REG is added.
> 
> 
> Do you think this is feasible? I guess this is the biggest uncertainty
> for me at the moment in this patch series.

I think it sounds fine in principle.  So long as it doesn't change the
existing behaviour when of_reg isn't set.

> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
> >  include/linux/mfd/core.h | 26 ++++++++++++++++++++------
> >  2 files changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index e735565969b3..4ecb376338f7 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int
> > id,
> >  	struct device_node *np = NULL;
> >  	int ret = -ENOMEM;
> >  	int platform_id;
> > +	u32 of_reg;
> >  	int r;
> > 
> >  	if (id == PLATFORM_DEVID_AUTO)
> > @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent,
> > int id,
> > 
> >  	if (parent->of_node && cell->of_compatible) {
> >  		for_each_child_of_node(parent->of_node, np) {
> > -			if (of_device_is_compatible(np, cell->of_compatible)) {
> > -				if (!of_device_is_available(np)) {
> > -					/* Ignore disabled devices error free */
> > -					ret = 0;
> > -					goto fail_alias;
> > -				}
> > -				pdev->dev.of_node = np;
> > -				pdev->dev.fwnode = &np->fwnode;
> > -				break;
> > +			if (!of_device_is_compatible(np, cell->of_compatible))
> > +				continue;
> > +
> > +			/* also match the unit address if set */

Please use correct grammar in comments (leaving off the full-stop).

> > +			if (cell->of_reg & MFD_OF_REG_VALID) {
> > +				if (of_property_read_u32(np, "reg", &of_reg))
> > +					continue;
> > +				if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg)
> > +					continue;
> >  			}
> > +
> > +			if (!of_device_is_available(np)) {
> > +				/* Ignore disabled devices error free */
> > +				ret = 0;
> > +				goto fail_alias;
> > +			}
> > +
> > +			pdev->dev.of_node = np;
> > +			pdev->dev.fwnode = &np->fwnode;
> > +			break;
> >  		}
> >  	}
> > 
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index d01d1299e49d..c2c0ad6b14f3 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -13,8 +13,11 @@
> >  #include <linux/platform_device.h>
> > 
> >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
> > +#define MFD_OF_REG_VALID	BIT(31)

What about 64bit platforms?

> > +#define MFD_OF_REG_MASK		GENMASK(30, 0)
> > 
> > -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,
> > _match)\
> > +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
> > +		     _of_reg, _match)					\
> >  	{								\
> >  		.name = (_name),					\
> >  		.resources = (_res),					\
> > @@ -22,24 +25,32 @@
> >  		.platform_data = (_pdata),				\
> >  		.pdata_size = (_pdsize),				\
> >  		.of_compatible = (_compat),				\
> > +		.of_reg = (_of_reg),					\
> >  		.acpi_match = (_match),					\
> >  		.id = (_id),						\
> >  	}
> > 
> > +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat,	\
> > +			_of_reg)					\
> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
> > +		     ((_of_reg) | MFD_OF_REG_VALID), NULL)		\
> > +
> >  #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat)		\
> > -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL)	\
> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
> > +		     0, NULL)						\
> > 
> >  #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match)	\
> > -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match)	\
> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0,	\
> > +		     _match)						\
> > 
> >  #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id)		\
> > -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL)	\
> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \
> > 
> >  #define MFD_CELL_RES(_name, _res)					\
> > -	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL)		\
> > +	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL)		\
> > 
> >  #define MFD_CELL_NAME(_name)						\
> > -	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL)		\
> > +	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL)		\
> > 
> >  struct irq_domain;
> >  struct property_entry;
> > @@ -78,6 +89,9 @@ struct mfd_cell {
> >  	 */
> >  	const char		*of_compatible;
> > 
> > +	/* matching the reg property if set */

Proper grammar please.

"OF unit address for device matching"

> > +	unsigned int		of_reg;
> > +
> >  	/* Matches ACPI */
> >  	const struct mfd_cell_acpi_match	*acpi_match;
Michael Walle May 25, 2020, 5:36 p.m. UTC | #3
Am 2020-05-15 12:28, schrieb Lee Jones:
> On Thu, 30 Apr 2020, Michael Walle wrote:
> 
>> Hi Lee,
>> 
>> Am 2020-04-23 19:45, schrieb Michael Walle:
>> > There might be multiple children with the device tree compatible, for
>> > example if a MFD has multiple instances of the same function. In this
>> > case only the first is matched and the other children get a wrong
>> > of_node reference.
>> > Add a new option to match also against the unit address of the child
>> > node. Additonally, a new helper OF_MFD_CELL_REG is added.
>> 
>> 
>> Do you think this is feasible? I guess this is the biggest uncertainty
>> for me at the moment in this patch series.
> 
> I think it sounds fine in principle.  So long as it doesn't change the
> existing behaviour when of_reg isn't set.
> 
>> > Signed-off-by: Michael Walle <michael@walle.cc>
>> > ---
>> >  drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
>> >  include/linux/mfd/core.h | 26 ++++++++++++++++++++------
>> >  2 files changed, 40 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
>> > index e735565969b3..4ecb376338f7 100644
>> > --- a/drivers/mfd/mfd-core.c
>> > +++ b/drivers/mfd/mfd-core.c
>> > @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int
>> > id,
>> >  	struct device_node *np = NULL;
>> >  	int ret = -ENOMEM;
>> >  	int platform_id;
>> > +	u32 of_reg;
>> >  	int r;
>> >
>> >  	if (id == PLATFORM_DEVID_AUTO)
>> > @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent,
>> > int id,
>> >
>> >  	if (parent->of_node && cell->of_compatible) {
>> >  		for_each_child_of_node(parent->of_node, np) {
>> > -			if (of_device_is_compatible(np, cell->of_compatible)) {
>> > -				if (!of_device_is_available(np)) {
>> > -					/* Ignore disabled devices error free */
>> > -					ret = 0;
>> > -					goto fail_alias;
>> > -				}
>> > -				pdev->dev.of_node = np;
>> > -				pdev->dev.fwnode = &np->fwnode;
>> > -				break;
>> > +			if (!of_device_is_compatible(np, cell->of_compatible))
>> > +				continue;
>> > +
>> > +			/* also match the unit address if set */
> 
> Please use correct grammar in comments (leaving off the full-stop).
> 
>> > +			if (cell->of_reg & MFD_OF_REG_VALID) {
>> > +				if (of_property_read_u32(np, "reg", &of_reg))
>> > +					continue;
>> > +				if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg)
>> > +					continue;
>> >  			}
>> > +
>> > +			if (!of_device_is_available(np)) {
>> > +				/* Ignore disabled devices error free */
>> > +				ret = 0;
>> > +				goto fail_alias;
>> > +			}
>> > +
>> > +			pdev->dev.of_node = np;
>> > +			pdev->dev.fwnode = &np->fwnode;
>> > +			break;
>> >  		}
>> >  	}
>> >
>> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>> > index d01d1299e49d..c2c0ad6b14f3 100644
>> > --- a/include/linux/mfd/core.h
>> > +++ b/include/linux/mfd/core.h
>> > @@ -13,8 +13,11 @@
>> >  #include <linux/platform_device.h>
>> >
>> >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
>> > +#define MFD_OF_REG_VALID	BIT(31)
> 
> What about 64bit platforms?

The idea was to have this as a logical number. I.e. for now you may only
have one subdevice per unique compatible string. In fact, if you have a
look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
subdevices. But there is only one DT node for all three of it. I guess
this works as long as you don't use phandles to reference the pwm node
in the device tree. Or you don't want to use device tree properties
per subdevice (for example the "timeout-sec" of a watchdog device).

So to circumvent this, I thought of having the unit-address (and thus
the "reg" property) to differentiate between multiple subdevices. Now
there is one special case for me: this board management controller
might be upgradable and it might change internally. Thus I came up
with that logical numbering of subdevices. Rob doesn't seem to be a
fan of that, though. Therefore, having bit 31 as a valid indicator
leaves you with 2^31 logical devices, which should be enough ;)

Rob proposed to have the internal offset as the unit-address. But
in that case I can also use devm_of_platform_populate() and don't
need the OF_MFD_CELL_REG; I'd just parse the reg offset in each
individual subdevice driver. But like I said, I wanted to keep the
internal offsets out of the device tree.

-michael

> 
>> > +#define MFD_OF_REG_MASK		GENMASK(30, 0)
>> >
>> > -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,
>> > _match)\
>> > +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
>> > +		     _of_reg, _match)					\
>> >  	{								\
>> >  		.name = (_name),					\
>> >  		.resources = (_res),					\
>> > @@ -22,24 +25,32 @@
>> >  		.platform_data = (_pdata),				\
>> >  		.pdata_size = (_pdsize),				\
>> >  		.of_compatible = (_compat),				\
>> > +		.of_reg = (_of_reg),					\
>> >  		.acpi_match = (_match),					\
>> >  		.id = (_id),						\
>> >  	}
>> >
>> > +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat,	\
>> > +			_of_reg)					\
>> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
>> > +		     ((_of_reg) | MFD_OF_REG_VALID), NULL)		\
>> > +
>> >  #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat)		\
>> > -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL)	\
>> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
>> > +		     0, NULL)						\
>> >
>> >  #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match)	\
>> > -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match)	\
>> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0,	\
>> > +		     _match)						\
>> >
>> >  #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id)		\
>> > -	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL)	\
>> > +	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \
>> >
>> >  #define MFD_CELL_RES(_name, _res)					\
>> > -	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL)		\
>> > +	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL)		\
>> >
>> >  #define MFD_CELL_NAME(_name)						\
>> > -	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL)		\
>> > +	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL)		\
>> >
>> >  struct irq_domain;
>> >  struct property_entry;
>> > @@ -78,6 +89,9 @@ struct mfd_cell {
>> >  	 */
>> >  	const char		*of_compatible;
>> >
>> > +	/* matching the reg property if set */
> 
> Proper grammar please.
> 
> "OF unit address for device matching"
> 
>> > +	unsigned int		of_reg;
>> > +
>> >  	/* Matches ACPI */
>> >  	const struct mfd_cell_acpi_match	*acpi_match;
Lee Jones May 26, 2020, 7:24 a.m. UTC | #4
On Mon, 25 May 2020, Michael Walle wrote:

> Am 2020-05-15 12:28, schrieb Lee Jones:
> > On Thu, 30 Apr 2020, Michael Walle wrote:
> > 
> > > Hi Lee,
> > > 
> > > Am 2020-04-23 19:45, schrieb Michael Walle:
> > > > There might be multiple children with the device tree compatible, for
> > > > example if a MFD has multiple instances of the same function. In this
> > > > case only the first is matched and the other children get a wrong
> > > > of_node reference.
> > > > Add a new option to match also against the unit address of the child
> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added.
> > > 
> > > 
> > > Do you think this is feasible? I guess this is the biggest uncertainty
> > > for me at the moment in this patch series.
> > 
> > I think it sounds fine in principle.  So long as it doesn't change the
> > existing behaviour when of_reg isn't set.
> > 
> > > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > > ---
> > > >  drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
> > > >  include/linux/mfd/core.h | 26 ++++++++++++++++++++------
> > > >  2 files changed, 40 insertions(+), 15 deletions(-)

[...]

> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > > > index d01d1299e49d..c2c0ad6b14f3 100644
> > > > --- a/include/linux/mfd/core.h
> > > > +++ b/include/linux/mfd/core.h
> > > > @@ -13,8 +13,11 @@
> > > >  #include <linux/platform_device.h>
> > > >
> > > >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
> > > > +#define MFD_OF_REG_VALID	BIT(31)
> > 
> > What about 64bit platforms?
> 
> The idea was to have this as a logical number. I.e. for now you may only
> have one subdevice per unique compatible string. In fact, if you have a
> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
> subdevices. But there is only one DT node for all three of it. I guess
> this works as long as you don't use phandles to reference the pwm node
> in the device tree. Or you don't want to use device tree properties
> per subdevice (for example the "timeout-sec" of a watchdog device).
> 
> So to circumvent this, I thought of having the unit-address (and thus
> the "reg" property) to differentiate between multiple subdevices. Now
> there is one special case for me: this board management controller
> might be upgradable and it might change internally. Thus I came up
> with that logical numbering of subdevices. Rob doesn't seem to be a
> fan of that, though. Therefore, having bit 31 as a valid indicator
> leaves you with 2^31 logical devices, which should be enough ;)
> 
> Rob proposed to have the internal offset as the unit-address. But
> in that case I can also use devm_of_platform_populate() and don't
> need the OF_MFD_CELL_REG; I'd just parse the reg offset in each
> individual subdevice driver. But like I said, I wanted to keep the
> internal offsets out of the device tree.

Oh, I see what you're doing.

So you're adding an arbitrary ID to the device's reg property in DT?

How is this not a hack?

Why don't you use the full address for identification?
Michael Walle May 26, 2020, 3:54 p.m. UTC | #5
Am 2020-05-26 09:24, schrieb Lee Jones:
> On Mon, 25 May 2020, Michael Walle wrote:
> 
>> Am 2020-05-15 12:28, schrieb Lee Jones:
>> > On Thu, 30 Apr 2020, Michael Walle wrote:
>> >
>> > > Hi Lee,
>> > >
>> > > Am 2020-04-23 19:45, schrieb Michael Walle:
>> > > > There might be multiple children with the device tree compatible, for
>> > > > example if a MFD has multiple instances of the same function. In this
>> > > > case only the first is matched and the other children get a wrong
>> > > > of_node reference.
>> > > > Add a new option to match also against the unit address of the child
>> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added.
>> > >
>> > >
>> > > Do you think this is feasible? I guess this is the biggest uncertainty
>> > > for me at the moment in this patch series.
>> >
>> > I think it sounds fine in principle.  So long as it doesn't change the
>> > existing behaviour when of_reg isn't set.
>> >
>> > > > Signed-off-by: Michael Walle <michael@walle.cc>
>> > > > ---
>> > > >  drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
>> > > >  include/linux/mfd/core.h | 26 ++++++++++++++++++++------
>> > > >  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> [...]
> 
>> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>> > > > index d01d1299e49d..c2c0ad6b14f3 100644
>> > > > --- a/include/linux/mfd/core.h
>> > > > +++ b/include/linux/mfd/core.h
>> > > > @@ -13,8 +13,11 @@
>> > > >  #include <linux/platform_device.h>
>> > > >
>> > > >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
>> > > > +#define MFD_OF_REG_VALID	BIT(31)
>> >
>> > What about 64bit platforms?
>> 
>> The idea was to have this as a logical number. I.e. for now you may 
>> only
>> have one subdevice per unique compatible string. In fact, if you have 
>> a
>> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
>> subdevices. But there is only one DT node for all three of it. I guess
>> this works as long as you don't use phandles to reference the pwm node
>> in the device tree. Or you don't want to use device tree properties
>> per subdevice (for example the "timeout-sec" of a watchdog device).
>> 
>> So to circumvent this, I thought of having the unit-address (and thus
>> the "reg" property) to differentiate between multiple subdevices. Now
>> there is one special case for me: this board management controller
>> might be upgradable and it might change internally. Thus I came up
>> with that logical numbering of subdevices. Rob doesn't seem to be a
>> fan of that, though. Therefore, having bit 31 as a valid indicator
>> leaves you with 2^31 logical devices, which should be enough ;)
>> 
>> Rob proposed to have the internal offset as the unit-address. But
>> in that case I can also use devm_of_platform_populate() and don't
>> need the OF_MFD_CELL_REG; I'd just parse the reg offset in each
>> individual subdevice driver. But like I said, I wanted to keep the
>> internal offsets out of the device tree.
> 
> Oh, I see what you're doing.
> 
> So you're adding an arbitrary ID to the device's reg property in DT?

Yes.

> How is this not a hack?

Well IMHO this is not more or less a hack as the current of_node
handling of MFD devices, which happens to work only because there
is only one device per compatible string (or it doesn't really work,
like in the stericsson,ab8500-pwm case). The of_node is assigned
according to the compatible string, just like in my case, only that
I have two subdevices with the same compatible string.

> Why don't you use the full address for identification?

Like I said, in the long term I would like to have support for
different versions of the board management controller without having
to change the device tree and have device tree bindings for the
subdevices at the same time. But it seems, that this is not possible
and I guess I have to bite the bullet and may need to provide another
device tree if the controller might be updated.

-michael
Andy Shevchenko May 26, 2020, 4:03 p.m. UTC | #6
On Tue, May 26, 2020 at 05:54:38PM +0200, Michael Walle wrote:
> Am 2020-05-26 09:24, schrieb Lee Jones:

...

> Like I said, in the long term I would like to have support for
> different versions of the board management controller

> without having to change the device tree and have device tree bindings for the
> subdevices at the same time.

But isn't device tree to describe *very specific platform* rather than *class
of platforms*?

> But it seems, that this is not possible
> and I guess I have to bite the bullet and may need to provide another
> device tree if the controller might be updated.
Lee Jones May 27, 2020, 6:53 a.m. UTC | #7
On Tue, 26 May 2020, Andy Shevchenko wrote:

> On Tue, May 26, 2020 at 05:54:38PM +0200, Michael Walle wrote:
> > Am 2020-05-26 09:24, schrieb Lee Jones:
> 
> ...
> 
> > Like I said, in the long term I would like to have support for
> > different versions of the board management controller
> 
> > without having to change the device tree and have device tree bindings for the
> > subdevices at the same time.
> 
> But isn't device tree to describe *very specific platform* rather than *class
> of platforms*?

Yes.  Device Tree describes the hardware.

If the hardware changes, so must the Device Tree.
Lee Jones June 8, 2020, 2:24 p.m. UTC | #8
On Mon, 25 May 2020, Michael Walle wrote:
> Am 2020-05-15 12:28, schrieb Lee Jones:
> > On Thu, 30 Apr 2020, Michael Walle wrote:
> > 
> > > Hi Lee,
> > > 
> > > Am 2020-04-23 19:45, schrieb Michael Walle:
> > > > There might be multiple children with the device tree compatible, for
> > > > example if a MFD has multiple instances of the same function. In this
> > > > case only the first is matched and the other children get a wrong
> > > > of_node reference.
> > > > Add a new option to match also against the unit address of the child
> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added.

[...]

> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > > > index d01d1299e49d..c2c0ad6b14f3 100644
> > > > --- a/include/linux/mfd/core.h
> > > > +++ b/include/linux/mfd/core.h
> > > > @@ -13,8 +13,11 @@
> > > >  #include <linux/platform_device.h>
> > > >
> > > >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
> > > > +#define MFD_OF_REG_VALID	BIT(31)
> > 
> > What about 64bit platforms?
> 
> The idea was to have this as a logical number. I.e. for now you may only
> have one subdevice per unique compatible string. In fact, if you have a
> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
> subdevices. But there is only one DT node for all three of it. I guess
> this works as long as you don't use phandles to reference the pwm node
> in the device tree. Or you don't want to use device tree properties
> per subdevice (for example the "timeout-sec" of a watchdog device).

This is not a good example, as the "stericsson,ab8500-pwm" is
legitimate.  Here we are registering 3 potential devices, but only
instantiating 1 of them.
Michael Walle June 8, 2020, 3:21 p.m. UTC | #9
Am 2020-06-08 16:24, schrieb Lee Jones:
> On Mon, 25 May 2020, Michael Walle wrote:
>> Am 2020-05-15 12:28, schrieb Lee Jones:
>> > On Thu, 30 Apr 2020, Michael Walle wrote:
>> >
>> > > Hi Lee,
>> > >
>> > > Am 2020-04-23 19:45, schrieb Michael Walle:
>> > > > There might be multiple children with the device tree compatible, for
>> > > > example if a MFD has multiple instances of the same function. In this
>> > > > case only the first is matched and the other children get a wrong
>> > > > of_node reference.
>> > > > Add a new option to match also against the unit address of the child
>> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added.
> 
> [...]
> 
>> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>> > > > index d01d1299e49d..c2c0ad6b14f3 100644
>> > > > --- a/include/linux/mfd/core.h
>> > > > +++ b/include/linux/mfd/core.h
>> > > > @@ -13,8 +13,11 @@
>> > > >  #include <linux/platform_device.h>
>> > > >
>> > > >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
>> > > > +#define MFD_OF_REG_VALID	BIT(31)
>> >
>> > What about 64bit platforms?
>> 
>> The idea was to have this as a logical number. I.e. for now you may 
>> only
>> have one subdevice per unique compatible string. In fact, if you have 
>> a
>> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
>> subdevices. But there is only one DT node for all three of it. I guess
>> this works as long as you don't use phandles to reference the pwm node
>> in the device tree. Or you don't want to use device tree properties
>> per subdevice (for example the "timeout-sec" of a watchdog device).
> 
> This is not a good example, as the "stericsson,ab8500-pwm" is
> legitimate.  Here we are registering 3 potential devices, but only
> instantiating 1 of them.

Mh?

static const struct mfd_cell ab8500_devs[] = {
..
        OF_MFD_CELL("ab8500-pwm",
                     NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
         OF_MFD_CELL("ab8500-pwm",
                     NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
         OF_MFD_CELL("ab8500-pwm",
                     NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
..
}

And in pwm-ab8500.c there are three offsets based on the pdev->id.

Am I missing something here?
Lee Jones June 8, 2020, 6:45 p.m. UTC | #10
On Mon, 08 Jun 2020, Michael Walle wrote:

> Am 2020-06-08 16:24, schrieb Lee Jones:
> > On Mon, 25 May 2020, Michael Walle wrote:
> > > Am 2020-05-15 12:28, schrieb Lee Jones:
> > > > On Thu, 30 Apr 2020, Michael Walle wrote:
> > > >
> > > > > Hi Lee,
> > > > >
> > > > > Am 2020-04-23 19:45, schrieb Michael Walle:
> > > > > > There might be multiple children with the device tree compatible, for
> > > > > > example if a MFD has multiple instances of the same function. In this
> > > > > > case only the first is matched and the other children get a wrong
> > > > > > of_node reference.
> > > > > > Add a new option to match also against the unit address of the child
> > > > > > node. Additonally, a new helper OF_MFD_CELL_REG is added.
> > 
> > [...]
> > 
> > > > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > > > > > index d01d1299e49d..c2c0ad6b14f3 100644
> > > > > > --- a/include/linux/mfd/core.h
> > > > > > +++ b/include/linux/mfd/core.h
> > > > > > @@ -13,8 +13,11 @@
> > > > > >  #include <linux/plataorm_device.h>
> > > > > >
> > > > > >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
> > > > > > +#define MFD_OF_REG_VALID	BIT(31)
> > > >
> > > > What about 64bit platforms?
> > > 
> > > The idea was to have this as a logical number. I.e. for now you may
> > > only
> > > have one subdevice per unique compatible string. In fact, if you
> > > have a
> > > look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
> > > subdevices. But there is only one DT node for all three of it. I guess
> > > this works as long as you don't use phandles to reference the pwm node
> > > in the device tree. Or you don't want to use device tree properties
> > > per subdevice (for example the "timeout-sec" of a watchdog device).
> > 
> > This is not a good example, as the "stericsson,ab8500-pwm" is
> > legitimate.  Here we are registering 3 potential devices, but only
> > instantiating 1 of them.
> 
> Mh?
> 
> static const struct mfd_cell ab8500_devs[] = {
> ..
>        OF_MFD_CELL("ab8500-pwm",
>                     NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
>         OF_MFD_CELL("ab8500-pwm",
>                     NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
>         OF_MFD_CELL("ab8500-pwm",
>                     NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
> ..
> }
> 
> And in pwm-ab8500.c there are three offsets based on the pdev->id.
> 
> Am I missing something here?

Scrap what I said above.

For some reason I had of_platform_populate() in my head.

This will register and enumerate 3 devices.
diff mbox series

Patch

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index e735565969b3..4ecb376338f7 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -117,6 +117,7 @@  static int mfd_add_device(struct device *parent, int id,
 	struct device_node *np = NULL;
 	int ret = -ENOMEM;
 	int platform_id;
+	u32 of_reg;
 	int r;
 
 	if (id == PLATFORM_DEVID_AUTO)
@@ -151,16 +152,26 @@  static int mfd_add_device(struct device *parent, int id,
 
 	if (parent->of_node && cell->of_compatible) {
 		for_each_child_of_node(parent->of_node, np) {
-			if (of_device_is_compatible(np, cell->of_compatible)) {
-				if (!of_device_is_available(np)) {
-					/* Ignore disabled devices error free */
-					ret = 0;
-					goto fail_alias;
-				}
-				pdev->dev.of_node = np;
-				pdev->dev.fwnode = &np->fwnode;
-				break;
+			if (!of_device_is_compatible(np, cell->of_compatible))
+				continue;
+
+			/* also match the unit address if set */
+			if (cell->of_reg & MFD_OF_REG_VALID) {
+				if (of_property_read_u32(np, "reg", &of_reg))
+					continue;
+				if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg)
+					continue;
 			}
+
+			if (!of_device_is_available(np)) {
+				/* Ignore disabled devices error free */
+				ret = 0;
+				goto fail_alias;
+			}
+
+			pdev->dev.of_node = np;
+			pdev->dev.fwnode = &np->fwnode;
+			break;
 		}
 	}
 
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index d01d1299e49d..c2c0ad6b14f3 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -13,8 +13,11 @@ 
 #include <linux/platform_device.h>
 
 #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
+#define MFD_OF_REG_VALID	BIT(31)
+#define MFD_OF_REG_MASK		GENMASK(30, 0)
 
-#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, _match)\
+#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
+		     _of_reg, _match)					\
 	{								\
 		.name = (_name),					\
 		.resources = (_res),					\
@@ -22,24 +25,32 @@ 
 		.platform_data = (_pdata),				\
 		.pdata_size = (_pdsize),				\
 		.of_compatible = (_compat),				\
+		.of_reg = (_of_reg),					\
 		.acpi_match = (_match),					\
 		.id = (_id),						\
 	}
 
+#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat,	\
+			_of_reg)					\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
+		     ((_of_reg) | MFD_OF_REG_VALID), NULL)		\
+
 #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat)		\
-	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL)	\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
+		     0, NULL)						\
 
 #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match)	\
-	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match)	\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0,	\
+		     _match)						\
 
 #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id)		\
-	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL)	\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \
 
 #define MFD_CELL_RES(_name, _res)					\
-	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL)		\
+	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL)		\
 
 #define MFD_CELL_NAME(_name)						\
-	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL)		\
+	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL)		\
 
 struct irq_domain;
 struct property_entry;
@@ -78,6 +89,9 @@  struct mfd_cell {
 	 */
 	const char		*of_compatible;
 
+	/* matching the reg property if set */
+	unsigned int		of_reg;
+
 	/* Matches ACPI */
 	const struct mfd_cell_acpi_match	*acpi_match;