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

[Andy Shevchenko]

On Thu, Apr 04, 2019 at 08:03:57AM +0100, Lee Jones wrote:
> On Thu, 04 Apr 2019, Lee Jones wrote:
> > On Tue, 02 Apr 2019, Andy Shevchenko wrote:
> > > On Tue, Apr 02, 2019 at 06:12:11AM +0100, Lee Jones wrote:
> > > > On Mon, 18 Mar 2019, Andy Shevchenko wrote:

> > > > > +static const struct mfd_cell bcove_dev[] = {
> > > > > +	{
> > > > > +		.name = "mrfld_bcove_pwrbtn",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[0],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_tmu",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[1],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_thermal",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[2],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_bcu",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[3],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_adc",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[4],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_charger",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[5],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_extcon",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[5],
> > > > > +	}, {
> > > > > +		.name = "mrfld_bcove_gpio",
> > > > > +		.num_resources = 1,
> > > > > +		.resources = &irq_level2_resources[6],
> > > > > +	},
> > > > > +	{	.name = "mrfld_bcove_region", },
> > > > > +};

> > > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > > +		ret = platform_get_irq(pdev, i);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +
> > > > > +		irq_level2_resources[i].start = ret;
> > > > > +		irq_level2_resources[i].end = ret;
> > > > > +	}
> > > > 
> > > > Although succinct, dragging values from one platform device into
> > > > another doesn't sound that neat.
> > > 
> > > So, how to split resources given in one _physical_ multi-functional device to
> > > several of them?  Isn't it what MFD framework for?
> > > 
> > > Any other approach here? I'm all ears!
> > 
> > From the child:
> > 
> >   platform_get_irq(dev->parent, CLIENT_ID);

So, instead of keeping a fragile approach in one driver, we will spread this
to all of them.

> If you set the .id of the cell properly you could do:
> 
>   platform_get_irq(dev->parent, dev->id);

This will bring a confuse, ID is used to form an instance name, for now
we don't have several instances of any of the devices from PMIC.

On top of above, some of the resources (one already, others might be a case in
the future) is split between two drivers, which would bring even more confusion
to the entire picture.

> > > > Also, since the ordering of the
> > > > devices is critical in this implementation, it also comes across as
> > > > fragile.
> > > 
> > > How fragile? In ACPI we don't have IRQ labeling scheme. Index is used for that.
> > > 
> > > > Any reason why ACPI can't register all of the child devices, or for
> > > > the child devices to obtain their IRQ directly from the tables?
> > > 
> > > And how are we supposed to enumerated them taking into consideration single
> > > ACPI ID given?
> > 
> > This question was a little whimsical, since I have no idea how the
> > ACPI tables you're working with are laid out.

There is one device node with several IRQ and other resources.
In pseudo code:

	device node {
		device ID,
		IRQ 0,
		IRQ 1,
		...
		MMIO 0,
		...
	}

-- 
With Best Regards,
Andy Shevchenko