Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC

[Andy Shevchenko]

On Thu, Jun 27, 2019 at 02:44:46PM +0100, Lee Jones wrote:
> On Wed, 26 Jun 2019, Andy Shevchenko wrote:
> 
> > On Wed, Jun 26, 2019 at 11:17:27AM +0100, Lee Jones wrote:
> > > On Wed, 26 Jun 2019, Andy Shevchenko wrote:
> > > > On Mon, Jun 24, 2019 at 05:13:48PM +0100, Lee Jones wrote:
> > > > > On Wed, 12 Jun 2019, Andy Shevchenko wrote:
> > 
> > > > > > Add an MFD driver for Intel Merrifield Basin Cove PMIC.
> > 
> > > > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > > > +		ret = platform_get_irq(pdev, i);
> > > > > 
> > > > > If you already know the order, define the children's device IDs in the
> > > > > parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
> > > > > like:
> > > > > 
> > > > >   platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);
> > > > > 
> > > > > Then you can skip all of this platform device -> platform device hoop
> > > > > jumping.
> > > > 
> > > > The idea of MFD is to get children to be parent agnostic
> > > > (at least to some extent). What you are proposing here
> > > > seems like disadvantage from MFD philosophy. No?
> > > 
> > > Not at all.  The idea of MFD is to split up support for monolithic h/w
> > > such that they can be handled properly by their appropriate
> > > subsystems, and by extension, maintained by the associated subject
> > > matter experts.
> > > 
> > > Children are often aware of their parents (some siblings are even
> > > aware of each other!), and many expect and depend on the data-sets
> > > provided by their parents.
> > 
> > Yes, that's true and that's why I put wording "to some extent" above.
> > 
> > > For instance (this example may come to bite me in the behind, but),
> > > taken from this very patch, where is this consumed?
> > > 
> > >  platform_set_drvdata(pdev, pmic);
> > 
> > Yes. It's used in children. BUT. This structure covers several PMIC chips and
> > the children driver doesn't know which generation / version of PMIC is serving.
> > 
> > What you are proposing with the change is to strictly link the children driver
> > to PMIC gen X ver Y, while above example doesn't do that.
> 
> Well that is a different argument. :)

> I still don't really like the idea of sucking one set of platform data
> just to place in another.  The implementation even looks hacky.

To me it looks straight forward. We get a description of MFD and split it
accordingly. Look at the rest of Intel PMIC drivers, they are built on the
same principles.

"Thanks" to firmware here, that does more, than on other PMICs, that's why we
have several IRQ resources, but from hardware point of view it's still an MFD.

> What if you were to provide the child driver with its IRQ index?
> Perhaps via platform data?

It seems some people would like to bring up Shady Cove PMIC support in the
future, which seems to behave in a similar way, while the (most of the) drivers
can be re-used.

So, for me it will look like a hack in each of PMIC driver to have additional
platform data for whatever can be passed using well-established facilities,
like device resources.

> > So, I'm not convinced it's a good change to have.

-- 
With Best Regards,
Andy Shevchenko