Hello Stephen,

On Mon, 2019-11-04 at 16:55 -0800, Stephen Boyd wrote:

Quoting Vaittinen, Matti (2019-10-28 23:28:51)

quoted

On Mon, 2019-10-28 at 16:32 -0700, Stephen Boyd wrote:

quoted

Quoting Matti Vaittinen (2019-10-24 04:44:40)

quoted

diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-

bd718x7.c
index ae6e5baee330..d17a19e04592 100644

--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c

@@ -8,6 +8,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/mfd/rohm-bd718x7.h>
+#include <linux/mfd/rohm-bd71828.h>
 #include <linux/mfd/rohm-bd70528.h>

It would be really great to not need to include these random
header
files in this driver and just use raw numbers somehow. Looks like
maybe
it can be done by populating a different device name from the mfd
driver
depending on the version of the clk controller desired? Then that
can
be
matched in this clk driver and we can just put the register info
in
this
file?

I still like keeping the chip type information on one header - no
matter what-ever format the clk-controller type/version information
is.
Rationale is that MFD and also few other sub-devices (not only the
clk)
need to use it. Currently at least the RTC.

But if we define clk register information for all PMICs in this c-
file, 
then (I think) we can only include the <linux/mfd/rohm-generic.h> -
which contains the PMIC type defines and the generic MFD data
structure. That would:

-#include <linux/mfd/rohm-bd718x7.h>
-#include <linux/mfd/rohm-bd71828.h>
-#include <linux/mfd/rohm-bd70528.h>
+#include <linux/mfd/rohm-generic.h>

That way the chip-type information could still be same for MFD and
all
sub-devices but clk driver would not need to include all the
details
for all the PMICs. I understand your point well as clk registers
for
these PMICs are really *limited*.

It's not even just about clk registers. It's also about how we have
device compatible strings and device names but this driver isn't
using
them to differentiate. Instead, it's looking at the parent device. I
don't get it. Why can't the MFD populate different clk devices for
the
different PMICs and make this driver completely oblivious to the
parent
device name/structure and these headers?

Probably because I didn't know how MFD/child device 'matching' works.

Do you mean the clk driver could do something like:

static const struct platform_device_id bd718x7_clk_id[] = {
        { "bd71837-clk", FOO},
        { "bd71847-clk", BAR},
        { "bd70528-clk", BAZ},
        { "bd71828-clk", BAF},
        { },
};
MODULE_DEVICE_TABLE(platform, bd718x7_clk_id);

static struct platform_driver bd71837_clk = {
        .driver = {
                .name = "bd718xx-clk",
        },
        .probe = bd71837_clk_probe,
	.id_table = bd718x7_clk_id
};

and then in MFD we just use correct name string for the mfd cell
representing the clk? (Eg. one of the bd71837-clk, bd71847-clk,
bd70528-clk, bd71828-clk) and in clk probe just differentiate based on
FOO, BAR, BAZ and BAF?

I guess we could do that (didn't try it out yet so I only guess for
now) - but I think this don't really mitigate the need for common
header. If we change the sub-device match mechanism to this then the
same mechanism should probably be applied to all sub-devices. And that
would be a case where I would like to see the very same FOO, BAR, BAZ
and BAF being used for all sub-devices - meaning it should still be a
MFD header. I think the drivers/clk/clk-s2mps11.c, drivers/mfd/sec-
core.c and include/linux/mfd/samsung/core.h are examples of this.

But I do like this platform_device_id based PMIC differentiation
better. It looks like the "de facto" way of doing this. Still, as I
said, I don't see we're getting rid of common header this way. Anyways,
I think I can cook-up patches to change this if I get buy-in from Lee,
Alexandre and Mark for changing the existing mechanism.

Thanks for teaching me something new once again! :)

Br,
	Matti Vaittinen