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

Michael Walle <michael@xxxxxxxx> · Mon, 25 May 2020 19:36:27 +0200

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@xxxxxxxx>
> ---
>  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;