From: Artur Rojek <contact@artur-rojek.eu>
To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
Paul Cercueil <paul@crapouillou.net>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Jonathan Cameron <jic23@kernel.org>,
Heiko Stuebner <heiko@sntech.de>,
linux-input <linux-input@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
linux-iio <linux-iio@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
Date: Sun, 19 Apr 2020 15:31:54 +0200 [thread overview]
Message-ID: <2ac495eef8143f2339b3e2a3eef24b27@artur-rojek.eu> (raw)
In-Reply-To: <CAAEAJfDLQnpsMPs8n4S1O6dA9155=mUPE7FqNQo_yNmp--UFgg@mail.gmail.com>
Hi Ezequiel,
On 2020-04-19 14:54, Ezequiel Garcia wrote:
> On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <paul@crapouillou.net>
>> wrote:
>> > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>> > <andy.shevchenko@gmail.com> a écrit :
>> > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@crapouillou.net>
>> > > wrote:
>> > >> Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>> > >> <andy.shevchenko@gmail.com> a écrit :
>> > >> > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>> > >> <paul@crapouillou.net>
>> > >> > wrote:
>> > >> >> Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>> > >> >> <andy.shevchenko@gmail.com> a écrit :
>> > >> >> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>> > >> >> <contact@artur-rojek.eu>
>> > >> >> > 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
next prev parent reply other threads:[~2020-04-19 13:32 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-17 20:28 [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 2/5] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode Artur Rojek
2020-04-17 20:59 ` Andy Shevchenko
2020-04-17 21:04 ` Paul Cercueil
2020-04-17 21:13 ` Andy Shevchenko
2020-04-17 21:18 ` Paul Cercueil
2020-04-17 21:42 ` Andy Shevchenko
2020-04-17 21:45 ` Paul Cercueil
2020-04-17 21:52 ` Andy Shevchenko
2020-04-17 21:56 ` Paul Cercueil
2020-04-19 12:54 ` Ezequiel Garcia
2020-04-19 13:23 ` Paul Cercueil
2020-04-19 13:31 ` Artur Rojek [this message]
2020-04-19 12:19 ` Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 4/5] dt-bindings: input: Add docs for ADC driven joystick Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver Artur Rojek
2020-04-17 21:10 ` Andy Shevchenko
2020-04-17 21:23 ` Paul Cercueil
2020-04-17 21:49 ` Andy Shevchenko
2020-04-17 22:48 ` Paul Cercueil
2020-04-18 11:57 ` Andy Shevchenko
2020-04-18 12:10 ` Paul Cercueil
2020-04-18 12:42 ` Andy Shevchenko
2020-04-18 13:24 ` Paul Cercueil
2020-04-18 14:22 ` Jonathan Cameron
2020-04-18 17:25 ` Paul Cercueil
2020-04-18 18:20 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ac495eef8143f2339b3e2a3eef24b27@artur-rojek.eu \
--to=contact@artur-rojek.eu \
--cc=andy.shevchenko@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=ezequiel@vanguardiasur.com.ar \
--cc=heiko@sntech.de \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=paul@crapouillou.net \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).