[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z_OQgqt0Wg17N05j@smile.fi.intel.com>
Date: Mon, 7 Apr 2025 11:44:50 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Raag Jadav <raag.jadav@...el.com>
Cc: gregkh@...uxfoundation.org, david.m.ertman@...el.com,
ira.weiny@...el.com, lee@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mfd: core: Support auxiliary device
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
Powered by blists - more mailing lists