From: Lee Jones <lee@kernel.org>
To: Liu Ying <victor.liu@nxp.com>
Cc: Rob Herring <robh@kernel.org>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de,
festevam@gmail.com, linux-imx@nxp.com
Subject: Re: [PATCH RESEND] mfd: Add Freescale i.MX8qxp Control and Status Registers (CSR) module driver
Date: Wed, 16 Nov 2022 13:57:04 +0000 [thread overview]
Message-ID: <Y3TsMDpphLYAIrf1@google.com> (raw)
In-Reply-To: <de9c49d33e4a90863e309190a27cd46bef47df07.camel@nxp.com>
On Wed, 16 Nov 2022, Liu Ying wrote:
> On Tue, 2022-11-15 at 07:33 -0600, Rob Herring wrote:
> > On Mon, Nov 14, 2022 at 11:22 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > On Mon, 2022-11-14 at 14:54 -0600, Rob Herring wrote:
> > > > On Mon, Nov 7, 2022 at 9:58 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > > > Hi Lee,
> > > > >
> > > > > On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > > > > > 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:
> > > > > > > > >
> >
> > [...]
> >
> > > > > > > > > > + 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:
> > > > >
> > > > > simple-pm-bus.c may populate child devices of simple-pm-bus devices
> > > > > because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> > > > >
> > > > > simple-pm-bus.c would _not_ populate child devices of simple-mfd
> > > > > devices because "ONLY_BUS" is set for simple-mfd devices.
> > > > >
> > > > > > /*
> > > > > > * 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.
> > > > >
> > > > > If simple-mfd device is a child device of one device listed in
> > > > > of_default_bus_match_table[], then it may be populated by
> > > > > drivers/of/platform.c. But, in my case, simple-mfd device is a child
> > > > > device of simple-pm-bus device(not listed in that table), so it will
> > > > > not be populated by drivers/of/platform.c. Instead,
> > > > > drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> > > > >
> > > > > > > > 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?
> > > > >
> > > > > If I change the compatible string of the CSR module from
> > > > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> > > > > to
> > > > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> > > > > all devices I'm interested in are populated and all relevant drivers
> > > > > can probe. But, this change makes 'make dt_binding_check' for the
> > > > > existing fsl,imx8qxp-csr.yaml fail:
> > > >
> > > > As 'simple-bus' is for MMIO devices, so is 'simple-pm-bus' with the
> > > > addition of PM capabilities. That means you have registers defined
> > > > (reg), but you don't.
> > > >
> > > > Either you have a block with mixed functions or you have separate
> > > > blocks. If the register space is all mixed together, then it is the
> > > > former and an MFD. Don't be designing your binding based on Linux
> > > > behavior.
> > >
> > > Thanks for clarifying how to differetiate MFD and 'simple-bus'/'simple-
> > > pm-bus'. I would say the register space of the CSR module is mixed
> > > together, e.g., LVDS PHY child device has a register offset 0x00,
> > > PXL2DPI child device has a register offset 0x40 and LDB child device
> > > has register offsets 0x20 and 0xe0 in i.MX8qxp MIPI DSI/LVDS combo
> > > subsystem CSR module register space. So, it appears to be a MFD.
> > >
> > > Lee, what do you think? If it is indeed an MFD, a new MFD driver for
> > > the CSR module is needed then.
> >
> > There already exists a driver which does what you need, so why create
> > a 2nd one? Just add the "fsl,imx8qxp-mipi-lvds-csr" compatible to the
> > simple-pm-bus match table and don't set ONLY_BUS. Isn't that enough?
>
> Adding "fsl,imx8qxp-mipi-lvds-csr" and "fsl,imx8qm-lvds-csr" compatible
> strings to the simple-pm-bus match table does work, but it tends to
> make people think the CSR module is a bus instead of a MFD.
MFDs don't exist. They're a figment of our imagination.
If you want PM enabled and wish for your child devices to be
auto-magically registered for you, simple-pm-bus is what you need.
> It's kinka
> straightforward if a MFD device driver lives in drivers/mfd directory.
> However, since "simple-mfd" compatible is also in that match table, I
> would add the CSR module's compatible strings to that match table if no
> objections.
Fine by me if it saves authoring a pointless driver.
--
Lee Jones [李琼斯]
prev parent reply other threads:[~2022-11-16 14:02 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-17 7:57 [PATCH RESEND] mfd: Add Freescale i.MX8qxp Control and Status Registers (CSR) module driver Liu Ying
2022-10-31 8:44 ` Liu Ying
2022-10-31 9:30 ` Lee Jones
2022-10-31 15:40 ` Lee Jones
2022-11-01 5:53 ` Liu Ying
2022-11-02 3:49 ` Liu Ying
2022-11-07 9:05 ` Lee Jones
2022-11-08 3:56 ` Liu Ying
2022-11-09 12:50 ` Ying Liu
2022-11-14 10:03 ` Lee Jones
2022-11-14 10:01 ` Lee Jones
2022-11-14 20:54 ` Rob Herring
2022-11-15 5:21 ` Liu Ying
2022-11-15 13:33 ` Rob Herring
2022-11-16 3:12 ` Liu Ying
2022-11-16 13:57 ` Lee Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y3TsMDpphLYAIrf1@google.com \
--to=lee@kernel.org \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=victor.liu@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).