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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ezequiel,


Le dim. 19 avril 2020 à 9:54, Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx> a écrit :
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.

The vast majority of Ingenic devices running Linux use a 3.x kernel with a lot of patches on top. These kernels don't support devicetree. So there is no problem with legacy devicetree files: there are no legacy devicetree files.

Of the few Ingenic devices running mainline kernels, all of them with an ADC node in the devicetree have the 'interrupts' property specified, out-of-tree or in-tree.

As the informal Ingenic SoCs maintainer I'm pretty aware of these things, and I can assure that we're not breaking anything. The only thing broken is the documentation which doesn't specify that the 'interrupts' property is required.

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?

That would be great.

-Paul






[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux