Messages in this thread | | | Date | Mon, 7 Nov 2022 09:05:09 +0000 | From | Lee Jones <> | Subject | Re: [PATCH RESEND] mfd: Add Freescale i.MX8qxp Control and Status Registers (CSR) module driver |
| |
On Wed, 02 Nov 2022, Liu Ying wrote:
> Hi Lee, > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote: > > Hi Lee, > > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote: > > > On Mon, 17 Oct 2022, Liu Ying wrote: > > > > > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a > > > > system > > > > controller. It represents a set of miscellaneous registers of a > > > > specific > > > > subsystem. It may provide control and/or status report interfaces > > > > to a > > > > mix of standalone hardware devices within that subsystem. > > > > > > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple > > > > power-managed > > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management > > > > operations > > > > of the CSR module's child devices to that simple power-managed > > > > bus, add a > > > > dedicated driver for the CSR module. Also, the driver would > > > > populate the CSR > > > > module's child devices. > > > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > --- > > > > The Freescale i.MX8qxp CSR DT bindings is at > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml. > > > > > > > > Resend the patch based on v6.1-rc1. > > > > > > > > drivers/mfd/Kconfig | 10 +++++++ > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/fsl-imx8qxp-csr.c | 53 > > > > +++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 64 insertions(+) > > > > create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c > > > > [...] > > > > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl- > > > > imx8qxp-csr.c > > > > new file mode 100644 > > > > index 000000000000..3915d3d6ca65 > > > > --- /dev/null > > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c > > > > @@ -0,0 +1,53 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > + > > > > +/* > > > > + * Copyright 2022 NXP > > > > + */ > > > > + > > > > +#include <linux/module.h> > > > > +#include <linux/of_platform.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/pm_runtime.h> > > > > + > > > > +static int imx8qxp_csr_probe(struct platform_device *pdev) > > > > +{ > > > > + int ret; > > > > + > > > > + pm_runtime_enable(&pdev->dev); > > > > + > > > > + ret = devm_of_platform_populate(&pdev->dev); > > > > > > The use of this API does not constitute a MFD. > > > > > > Please use "simple-mfd" instead. > > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the > > simple-pm-bus driver would not populate child devices of simple-mfd > > devices.
Right, simple-pm-bus will not populate child devices, because:
/* * These are transparent bus devices (not simple-pm-bus matches) that * have their child nodes populated automatically. So, don't need to * do anything more. We only match with the device if this driver is * the most specific match because we don't want to incorrectly bind to * a device that has a more specific driver. */
So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> > Also, the simple-pm-bus driver would not enable runtime > > power management for simple-mfd devices due to "ONLY_BUS", which > > means it would not propagate power management operations from child > > devices of simple-mfd devices to parent devices of simple-mfd > > devices. That's why a dedicated fsl-imx8qxp-csr driver is needed.
This is more of an issue.
Why can't this device use "simple-pm-bus" to have support for both auto-registration AND Runtime PM?
> One more point which might be overlooked - as mentioned in commit > message, the CSR module is a child node of a simple power-managed > bus(i.MX8qxp pixel link MSI bus), which means the child devices of the > CSR module(as a simple-mfd device) won't be populated by > of_platform_default_populate() from of_platform_default_populate_init() > because "simple-pm-bus" is not listed in of_default_bus_match_table[] > and hence recursion of of_platform_bus_create() will stop at the > simple-pm-bus. This is also a reason why a dedicated fsl-imx8qxp-csr > driver is needed to populated those child devices of the CSR module.
Not sure I know the semantics well enough (anymore) to get a meaningful picture of what you're trying to explain, and I do not have any suitable H/W here to mock-up a real-world test-bed / concept demonstrator to debug this for you.
The long and the short of it is; we have a bunch of automatic child-device-registering helpers in the kernel. One of the mechanisms is bound to work for you if you structure your code appropriately.
Introducing an empty, meaningless driver, simply to call a single function it not acceptable. Please make the infrastructure already offered specifically to solve this category of issue work for your use-case.
-- Lee Jones [李琼斯]
| |