Re: [PATCH 4/6] mfd: Add support for the MediaTek MT6358 PMIC

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]

On Thu, Feb 07, 2019 at 10:04:50AM +0000, Marc Zyngier wrote:
> On 07/02/2019 09:34, Lee Jones wrote:

> >> +static struct irq_top_t mt6358_ints[] = {
> >> +	MT6358_TOP_GEN(BUCK),
> >> +	MT6358_TOP_GEN(LDO),
> >> +	MT6358_TOP_GEN(PSC),
> >> +	MT6358_TOP_GEN(SCK),
> >> +	MT6358_TOP_GEN(BM),
> >> +	MT6358_TOP_GEN(HK),
> >> +	MT6358_TOP_GEN(AUD),
> >> +	MT6358_TOP_GEN(MISC),
> >> +};

> > What is a 'top' IRQ?

It looks like it's an intermediate parent IRQ controller; that's quite a
common design for MFDs on slow buses to cut down on the number of status
registers to poll.  IIRC there at least used to be some framework reason
for not using normal chained interrupts for these but I can't remember
it any more - possibly something to do with threaded handlers.

This is all looking very famililar, I suspect it's based on other
drivers dating back years rather than doing anything original.  There's
certainly a bunch of other drivers with very similar patterns in tree,
this doesn't look like it's got any problems over what most similar
drivers are doing.  The patterns all predate drivers/irqchip and a lot
of them will come from me.

> >> +static void pmic_irq_enable(struct irq_data *data)
> >> +{
> >> +	unsigned int hwirq = irqd_to_hwirq(data);
> >> +	struct mt6397_chip *chip = irq_data_get_irq_chip_data(data);
> >> +	struct pmic_irq_data *irq_data = chip->irq_data;
> >> +
> >> +	irq_data->enable_hwirq[hwirq] = 1;
> >> +}

> > I see that you're doing your own caching operations.  Is that
> > required?  I think I'm going to stop here and as for some IRQ guy's
> > input on this.

> Dunno either. I thought that's what regmap was for?

IIRC from when I wrote drivers for chips like this these operations are
called with interrupts disabled so you can't write to interrupt driven
buses so you need to cache the write and flush in sync_unlock().  You
can't do this with regmap as it stands since on a device that can't be
accessed with interrupts disabled you'd need to disable the writes
before going into the interrupts off section and there's a risk that
having the device cache disabled would disrupt some other function on
the chip that was expecting writes to get posted immediately.  

It would be possible to add per-register cache only behaviour but
someone would need to do that and it's not clear that it's worth the
effort, especially since for slow buses we currently lock the entire
regmap including the cache with mutexes so you can't actually access the
cache with interrupts off at the minute.

> >> +	for (i = 0; i < irq_data->num_pmic_irqs; i++) {
> >> +		if (irq_data->enable_hwirq[i] ==
> >> +				irq_data->cache_hwirq[i])
> >> +			continue;

> Please explain what you are trying to do here. The unlock operation is
> supposed to affect exactly one interrupt. Instead, you seem to deal with
> a bunch of them at once. Operations are supposed to happen on the "leaf"
> IRQs, not on the multiplexing interrupt.

IIRC it was done this way because it wasn't altogether clear if
operations on multiple interrupts could ever be batched or not and given
that we're dealing with slow buses the cost of the loop is negligable.

> Also, the whole cache thing seems pretty pointless. Why isn't regmap
> doing that for you?

See above.

> >> +	for (i = 0; i < mt6358_irq_data->num_top; i++) {
> >> +		if (top_int_status & BIT(mt6358_ints[i].top_offset))
> >> +			mt6358_irq_sp_handler(chip, i);
> >> +	}
> >> +
> >> +	return IRQ_HANDLED;
> >> +}

> Why isn't this a normal chained irq flow, instead of a homegrown irq
> handler? Is that because this is a threaded handler?

I think that's it but like I say I can't remember clearly any more, it's
been a decade.

> >> +	ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL,
> >> +					mt6358_irq_handler, IRQF_ONESHOT,
> >> +					mt6358_irq_chip.name, chip);
> >> +	if (ret) {
> >> +		dev_err(chip->dev, "failed to register irq=%d; err: %d\n",
> >> +			chip->irq, ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	enable_irq_wake(chip->irq);

> Why is that decided at probe time, from kernel space?

IIRC it's due to it being the main interrupt for the device and there
being at some point issues with this getting propagated to parent
interrupts so wake just got turned on all the time for the parent and we
relied on controlling the children.  Making things be proper chained
interrupt controllers would solve that I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]