Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Phil Reid]

On 29/07/2016 13:48, Peter Rosin wrote:
> On 2016-07-28 04:44, Phil Reid wrote:
> > G'day Peter,
> >
> > Thanks for the feedback.
> > +linux-kernel@xxxxxxxxxxxxxxx
> >
> > On 27/07/2016 13:32, Peter Rosin wrote:
> > > On 2016-07-27 05:05, Phil Reid wrote:
> > > > +static void pca954x_irq_mask(struct irq_data *idata)
> > > > +{
> > > > +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
> > > > +	struct pca954x *data = i2c_mux_priv(muxc);
> > > > +	unsigned int pos = idata->hwirq;
> > > > +
> > > > +	data->irq_mask &= ~BIT(pos);
> > > > +	if ((data->irq_mask & 0x3) != 0)
> > >
> > > The 0x3 mask should probably also go into struct chip_desc, then a non-
> > > empty value in this field could be used as trigger for initing the irq
> > > stuff.
>
> Ok, this comment just shows that I was not getting it. Please ignore it.
> Some other (new) thing in chip_desc is needed to trigger irq init for the
> chips that have irq support.
>
> > > > +		disable_irq(data->client->irq);
> > > > +}
> > > > +
> > > > +static void pca954x_irq_unmask(struct irq_data *idata)
> > > > +{
> > > > +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
> > > > +	struct pca954x *data = i2c_mux_priv(muxc);
> > > > +	unsigned int pos = idata->hwirq;
> > > > +
> > > > +	data->irq_mask |= ~BIT(pos);
> > > > +	if ((data->irq_mask & 0x3) == 0x3)
> > >
> > > This is what you mentioned in the commit message, but I don't get it.
> > > Please explain further, and also think about how the same problem
> > > could be handled should there be 4 incoming irq lines as in pca9544
> > > and pca9545.
> >
> > Yes this is the tricky point.
> >
> > Consider this:
> >
> > Host - pca954x + Dev1
> >                 \ Dev2
> >
> > In my case devs are ltc1760 that generate SMBALERT's (active low irq) which
> > are not maskable in the device. There is nothing that can be configured in the
> > device to prevent them assert an irq.
> >
> > If just considering them as shared interrupts, ie pca954x is not an irq ctlr, just a wired and.
> > On boot Dev1 is registered and enable shared irq. If Dev2 is asserting SMBALERT
> > this results in irq being continuously triggered, trigger Dev1 irq handler which
> > can't clear the irq because it's being asserted by Dev2.
> >
> > My system eventually boots but spends alot of time looping in the irq for dev1
> > until dev2 gets registered.
> >
> >
> >
> > The same problem occurs with the pca954x driver present, unless I disable the irq
> > until both slaves have unmasked the irq. This is not a good generic solution as the
> > system may be used with only one irq active.
> >
> > It's really a deficiency in the hardware design but I'm not sure I'll get that fixed.
> > Or I may be missing something obvious to deal with this situation
>
> 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.
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.
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?


--
Regards
Phil Reid