Re: [PATCH 1/2] HISI LPC: Reference static MFD cells for ACPI support

From: John Garry
Date: Fri May 04 2018 - 06:22:17 EST


On 04/05/2018 11:03, Lee Jones wrote:
On Fri, 04 May 2018, John Garry wrote:

On 04/05/2018 10:02, Lee Jones wrote:
On Thu, 03 May 2018, Andy Shevchenko wrote:

On Thu, 2018-05-03 at 23:08 +0800, John Garry wrote:
Currently for ACPI support the driver models the host as
an MFD. For a device connected to the LPC bus, we dynamically
create an MFD cell for that device, configuring the cell
name and ACPI match parameters manually. This makes supporting
named devices and also special setup handling for certain devices
awkward, as we would need to introduce some special ACPI device
handling according to device HID.

To avoid this, create reference static MFD cells for known
child devices, so when adding an MFD cell we can fix the cell
platform data as required. For this, a setup callback function
is added.

For now, only the IPMI cell is added.

+static const struct mfd_cell *hisi_lpc_acpi_mfd_get_cell(const char
*hid)
+{
+ const struct hisi_lpc_acpi_mfd_cell *cell =
hisi_lpc_acpi_mfd_cells;
+
+ for (; cell && cell->mfd_cell.name; cell++) {
+ const struct mfd_cell *mfd_cell = &cell->mfd_cell;
+ const struct mfd_cell_acpi_match *acpi_match;
+
+ acpi_match = mfd_cell->acpi_match;
+ if (!strcmp(acpi_match->pnpid, hid))
+ return mfd_cell;
+ }
+
+ return NULL;
+}

I'm not sure I understand why MFD core can't do it (as seen in lines
drivers/mfd/core.c:105 and below).


Hi Lee,

You shouldn't be using the MFD API outside of MFD anyway. Either it
is an MFD driver, or it isn't. If it is, please move it. If it's not,
please don't use the API.

We're modelling as an MFD, but it's not an MFD in the classic sense. We're
just using the MFD API for convenience (and to avoid code duplication), as
the MFD API does what we require for this driver.

I know what you're doing, and it's wrong. ;)

My current suspicion is that the driver needs splitting and only part
of it ends up in MFD.

How would you propose splitting the driver? By adding a lib function
specific for this driver for the ACPI probe?

Look at:

drivers/platform/chrome/cros_ec_lpc.c

and

drivers/mfd/cros_ec.c


Right, I see, something similar to what I suggested.

I don't really see a point in splitting the driver across drivers/mfd and drivers/bus, and introducing dependencies. This is more especially considering this is a legacy host controller with no potential future developments, and not worth the effort.

If you feel strongly enough about not using the MFD API outside drivers/mfd, then I'll look at other solutions, like using platform device APIs directly.

Cheers,
John