Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.

[Artur Rojek]

Hi Ezequiel,

On 2020-04-19 14:54, Ezequiel Garcia wrote:
> On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko 
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx> 
> > wrote:
> > > Le sam. 18 avril 2020 Ã 0:42, Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> a Ãcrit :
> > > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > > > wrote:
> > > >>  Le sam. 18 avril 2020 Ã 0:13, Andy Shevchenko
> > > >>  <andy.shevchenko@xxxxxxxxx> a Ãcrit :
> > > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
> > > >> <paul@xxxxxxxxxxxxxxx>
> > > >>  > wrote:
> > > >>  >>  Le ven. 17 avril 2020 Ã 23:59, Andy Shevchenko
> > > >>  >>  <andy.shevchenko@xxxxxxxxx> a Ãcrit :
> > > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> > > >>  >> <contact@xxxxxxxxxxxxxx>
> > > >>  >>  > wrote:
> > > >>  >
> > > >>  > ...
> > > >>  >
> > > >>  >>  >>  +       irq = platform_get_irq(pdev, 0);
> > > >>  >>  >
> > > >>  >>  > Before it worked w/o IRQ, here is a regression you introduced.
> > > >>  >>
> > > >>  >>  Before it simply did not need the IRQ, which is provided by the
> > > >>  >>  devicetree anyway. No regression here.
> > > >>  >
> > > >>  > Does it work without IRQ? Or it was a dead code till now?
> > > >>  > For me it's clear regression. Otherwise something is really wrong
> > > >> in a
> > > >>  > process of development of this driver.
> > > >>
> > > >>  Nothing wrong here. The IRQ was not used by the driver for the
> > > >>  functionality it provided before. It is required now to support the
> > > >>  touchscreen channels.
> > > >
> > > > This is exactly what's wrong.
> > > > Previous DTS for my (hypothetical) case has no IRQ defined. Everything
> > > > works, right?
> > > > Now, due to this change it breaks my setup. Don't you see the problem?
> > >
> > > The IRQ has been provided by every concerned DTS file since the
> > > introduction of this driver and the related bindings, even though it
> > > was not used by the driver.
> >
> > Can you speak for all possible DTSs/DTBs in the wild?
> > Okay, in any case it will be problem of maintainers and yours if
> > somebody complains.
> > I'm not going to push this anyway -- your choice.
> >
> > But I see a (potential) regression.
>
> So, there are a few things to keep in mind here.
>
> Let's abstract ourselves from this specific driver
> for a minute.
>
> First, and just as Andy pointed out, we can never be fully
> sure about DTBs out there. These could be out of tree,
> so out of our control. By introducing a new requirement
> we break them, which may be seen as a regression.
>
> Second, the interrupt is not required as per
> current mainline bindings/iio/adc/ingenic,adc.txt,
> so it is perfectly legal for users to not have an interrupt
> specified.
>
> Now, back to this case, I think we can get away with this
> change, provided this hardware is not that widespread
> among developers/users that follow upstream closely.
>
> I suspect anyone developing a serious platform
> with this SoC is most likely using some vendor kernel.
>
> If that is not the case, i.e. if you have users _actually_
> using this upstream driver, then we should consider
> making the interrupt optional instead of required.
>
> Or we can also just break it and hope nobody
> complaints.
>
> BTW, this series looks great and I'm happy
> to see JZ47xx activity :-)
>
> Arthur: perhaps you can consider converting the txt dt binding
> to yaml?
Sure, it will come with v6 of this patchset.
And this time I'll make the `interrupts` property required :-)

- Artur
> Cheers,
> Ezequiel