Re: [PATCH v2] mfd: core: Support auxiliary device

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Apr 07, 2025 at 01:16:14PM +0530, Raag Jadav wrote:
> Extend MFD subsystem to support auxiliary child device. This is useful
> for MFD usecases where parent device is on a discoverable bus and doesn't
> fit into the platform device criteria. Purpose of this implementation is
> to provide discoverable MFDs just enough infrastructure to register
> independent child devices with their own memory and interrupt resources
> without abusing the platform device.
> 
> Current support is limited to just PCI type MFDs, but this can be further
> extended to support other types like USB in the future.

> PS: I'm leaning towards not doing any of the ioremap or regmap on MFD
> side and think that we should enforce child devices to not overlap.

Yes, but we will have the cases in the future, whatever,
for the first step it's okay.

> If there's a need to handle common register access by parent device,
> then I think it warrants its own driver which adds auxiliary devices
> along with a custom interface to communicate with them, and MFD on
> AUX is not the right solution for it.

...

> -static const struct device_type mfd_dev_type = {
> -	.name	= "mfd_device",
> +enum mfd_dev {
> +	MFD_AUX_DEV,
> +	MFD_PLAT_DEV,
> +	MFD_MAX_DEV
> +};
> +
> +static const struct device_type mfd_dev_type[MFD_MAX_DEV] = {
> +	[MFD_AUX_DEV]	= { .name = "mfd_auxiliary_device" },
> +	[MFD_PLAT_DEV]	= { .name = "mfd_platform_device" },
>  };

This is likely an ABI breakage if anything looks in sysfs for mfd_device.

...

> +static int mfd_remove_devices_fn(struct device *dev, void *data)
> +{
> +	if (dev->type == &mfd_dev_type[MFD_AUX_DEV])
> +		return mfd_remove_auxiliary_device(dev);

> +	else if (dev->type == &mfd_dev_type[MFD_PLAT_DEV])

Redundant 'else'

> +		return mfd_remove_platform_device(dev, data);
> +
> +	return 0;
> +}

...

> +#ifndef MFD_AUX_H
> +#define MFD_AUX_H
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/ioport.h>

> +#include <linux/types.h>

How is this one being used?

> +#define auxiliary_dev_to_mfd_aux_dev(auxiliary_dev) \
> +	container_of(auxiliary_dev, struct mfd_aux_device, auxdev)

Missing container_of.h and better to define after the data type as it can be
converted to static inline, if required.

> +/*
> + * Common structure between MFD parent and auxiliary child device.
> + * To be used by leaf drivers to access child device resources.
> + */
> +struct mfd_aux_device {
> +	struct auxiliary_device auxdev;

> +	struct resource	mem;
> +	struct resource	irq;
> +	/* Place holder for other types */
> +	struct resource	ext;

Why this can't be simply a VLA?

> +};
> +
> +#endif

...

> +/* TODO: Convert the platform device abusers and remove this flag */
> +#define MFD_AUX_TYPE	INT_MIN

INT_MIN?! This is a bit unintuitive. BIT(31) sounds better to me.
Or even a plain (decimal) number as PLATFORM_DEVID_* done, for example.

-- 
With Best Regards,
Andy Shevchenko