Re: [PATCH v5 2/3] mfd: add support for Allwinner SoCs ADC

[Lee Jones <lee.jones@linaro.org>]

On Mon, 12 Sep 2016, Maxime Ripard wrote:

> On Mon, Sep 12, 2016 at 02:56:55PM +0100, Lee Jones wrote:
> > > >>> Then use .data = <defined model ID> and set up a switch() in .probe().
> > > >>
> > > >> Uh? Why? It just adds a non-standard indirection, while using
> > > >> of_match_device is very standard, and used extensively in Linux.
> > > > 
> > > > You still use of_match_device() to obtain the ID.
> > > > 
> > > > The "don't mix DT with the MFD API" is there to prevent some of the
> > > > nasty hacks I've seen previously.  This particular example doesn't
> > > > seem so bad, but it's a gateway to ridiculous hackery!
> > > 
> > > How am I supposed to get the .data without of_match_node then?
> > > What's more hackish in using .data field for specific data for each
> > > compatible than in using a random ID in .data and switching on it? The
> > > result is exactly the same, the switching case being more verbose and
> > > adding complexity to something that can be done in a straightforward manner.
> > 
> > I've already agreed that your implementation isn't terrible, but I'd
> > still like to remain strict on the rules.
> > 
> > Better still, can you can dynamically test which platform you're on,
> > via a version register or similar?
> > 
> > Failing that, see how everyone else does it:
> > 
> >  `git grep "\.data" -- drivers/mfd/`
> 
> Just to make sure, you prefer something like
> 
> static struct my_struct data = {
> };
> 
> static struct my_struct data2 = {
> };
> 
> struct of_device_id matches[] = {
>        { compatible = "...", data = <ID> },
>        { compatible = "...", data = <ID2> },
> };
> 
> of_id = of_match_device (dev, matches);
> switch (of_id->data) {
> case <ID>:
>      function(data);
> case <ID2>:
>      function(data2);
> };
> 
> over
> 
> static struct my_struct data = {
> };
> 
> static struct my_struct data2 = {
> };
> 
> struct of_device_id matches[] = {
>        { compatible = "...", data = data },
>        { compatible = "...", data = data2 },
> };
> 
> of_id = of_match_device (dev, matches);
> function(of_id->data);
> 
> ?
> 
> This is the *only* time this is going to be used in that driver. I can
> understand the need for a version if you need to apply quirks in
> several functions, but here it clearly looks suboptimal.
> 
> And we are indeed using this construct in the AXP MFD, and it just
> doesn't scale either and become quite difficult to maintain when you
> have a significant number of variants, and then you have to patch
> *all* the switch instances to get something done.

static struct my_struct data = {
};

static struct my_struct data2 = {
};

struct of_device_id matches[] = {
       { compatible = "...", data = <ID> },
       { compatible = "...", data = <ID2> },
};

int probe()
{
	struct mfd_cell *cell;

	of_id = of_match_device (dev, matches);
	switch (of_id->data) {
	case <ID>:
     	     cell = data;
	case <ID2>:
     	     cell = data2;
	};

	mfd_add_devices(..., cell, ...)
};

It's an extra few lines, but worth it to unbind MFD from DT.

Is there really no way to obtain this information dynamically?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog