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

[Lee Jones]

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.

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

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

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog