Date:	Mon, 1 Aug 2016 16:52:33 +0800
From:	Phil Reid <preid@...ctromag.com.au>
To:	Peter Rosin <peda@...ntia.se>, wsa@...-dreams.de,
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller
 support.

On 1/08/2016 16:22, Peter Rosin wrote:
> On 2016-08-01 08:25, Phil Reid wrote:
>> On 29/07/2016 13:48, Peter Rosin wrote:
>>> Ok, I think I get the problem, but I too am at a loss and see no elegant solution.
>>> One sad thing about your workaround is that it is not working at all unless there
>>> is an irq user on all mux child segments that unmasks the corresponding irq. Right?
>>>
>>> I think the default should be that the driver assumes sane HW, i.e. the irq inputs
>>> are not asserted unless some driver is able to clear them. You can then add an
>>> option to keep irqs disabled when the mask "is wrong". As a bonus, I think this
>>> "is wrong" test should be configurable so that you specify a bitmask of irqs that
>>> all has to be unmasked for the irq to be enabled (and use that instead of the 0x3
>>> in the pca954x_irq_mask/pca954x_irq_unmask functions). That makes the workaround
>>> more flexible.
>>
>> Correct, It works for my use case at the moment. There would need to be a way to define a mask.
>> Via device tree for example.
>
> Yes, some optional property in DT was my vision also.
>
>> I think sane devices that have irq masking don't really need the pca954x to be a irq source.
>> They could be configured with a shared IRQ as the pCA954x just ANDs then incoming lines together.
>
> Right. And I suppose we don't know if this is already happening... AFAIU,
> that approach would still work even if pca954x is made an interrupt source.
> Or?
>
> I mean, as you say, the pca954x functions as an electrical AND, and if
> interrupt clients register with whatever the parent interrupt controller
> is, they would be not be affected if there is also an interrupt controller
> for pca954x?
>
> Yes, such imaginary uses could have just wired all the irq lines together,
> but we don't know if they actually did that or if they possibly went via
> the irq routing in the pca954x for whatever reason...
>
>> So does my use case and workaround justify the complexity added to the driver?
>>
>> Thou there is possibly a performance benefit from reading the pca954x to dispatch the appropriate
>> irq, which would save alot of i2c transactions to probe each possible irq source device and assocaited
>> i2c mux switching.
>>
>> WDYR?
>
> It was the last bit that I also realized, and it would be nice to not have to
> dig through all irq devices on the child side of the mux (and other clients
> sharing the irq line with the pca954x for that matter). In theory there could
> be quite a few...
>
> So, yes, I think it might be worthwhile to make it an interrupt controller.
> And then the tweak with your mask is no longer the big part of it...
>
> BTW, you also need to change bindings docs in
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
Yeap, just wanted to nail down a direction before going thru that as well.
>
> I also noticed that you are using irq_domain_add_legacy which is marked as,
> *tada* legacy, and that most drivers should use a linear domain. Sounds like
> a linear domain fits the use case here, no?
I'll fix that.
>
> And another note, the workaround for your limited hw is rather non-generic.
> I mean, if the irq line from the pca954x to the parent interrupt controller
> is shared with something else, then there would still be a flood even with
> the workaround. Or am I misunderstanding that?
>

Yes there's still going to be a flood in that case.
My workaround is really specific to my hardware where I know that there is only
one irq source on each input the pca954x.

I'll start a patch with the irq support and then look at the how invasive my workaround is.

Thanks

-- 
Regards
Phil Reid

Powered by blists - more mailing lists