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

Commit Message

Artur Rojek April 17, 2020, 8:28 p.m. UTC

The SADC component in JZ47xx SoCs provides support for touchscreen
operations (pen position and pen down pressure) in single-ended and
differential modes.

Of the known hardware to use this controller, GCW Zero and Anbernic RG-350
utilize the touchscreen mode by having their joystick(s) attached to the
X/Y positive/negative input pins.
GCW Zero comes with a single joystick and is sufficiently handled with the
currently implemented single-ended mode. Support for boards with two
joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp channels
will need to be provided in the future.

The touchscreen component of SADC takes a significant time to stabilize
after first receiving the clock and a delay of 50ms has been empirically
proven to be a safe value before data sampling can begin.

All the boards which probe this driver have the interrupt provided from
devicetree, with no need to handle a case where the irq was not provided.

Signed-off-by: Artur Rojek <[e-mail]>
Tested-by: Paul Cercueil <[e-mail]>
---

 Changes:

 v2: - improve description of the touchscreen mode,
     - get rid of the unneeded kfifo,
     - drop IIO_BUFFER_CB from Kconfig,
     - remove extended names from the touchscreen channels

 v3: remove unneeded `linux/iio/kfifo_buf.h` include

 v4: clarify irq provider source in the patch description

 v5: no change

 drivers/iio/adc/Kconfig       |   1 +
 drivers/iio/adc/ingenic-adc.c | 109 +++++++++++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko April 17, 2020, 8:59 p.m. UTC | #1

On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <[e-mail]> wrote:
>
> The SADC component in JZ47xx SoCs provides support for touchscreen
> operations (pen position and pen down pressure) in single-ended and
> differential modes.
>
> Of the known hardware to use this controller, GCW Zero and Anbernic RG-350
> utilize the touchscreen mode by having their joystick(s) attached to the
> X/Y positive/negative input pins.
> GCW Zero comes with a single joystick and is sufficiently handled with the
> currently implemented single-ended mode. Support for boards with two
> joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp channels
> will need to be provided in the future.
>
> The touchscreen component of SADC takes a significant time to stabilize
> after first receiving the clock and a delay of 50ms has been empirically
> proven to be a safe value before data sampling can begin.
>
> All the boards which probe this driver have the interrupt provided from
> devicetree, with no need to handle a case where the irq was not provided.

Device Tree
IRQ

...

> +               .scan_type = {
> +                       .sign = 'u',
> +                       .realbits = 12,

> +                       .storagebits = 16

It's slightly better to leave comma in such cases.

> +               },

> +               .scan_type = {
> +                       .sign = 'u',
> +                       .realbits = 12,

> +                       .storagebits = 16

Ditto.

> +               },

...

>                 .indexed = 1,
>                 .channel = INGENIC_ADC_AUX,
> +               .scan_index = -1

Ditto. You see above? Isn't it nice that you didn't touch that line?
So, perhaps next developer can leverage this subtle kind of things.

>                 .indexed = 1,
>                 .channel = INGENIC_ADC_BATTERY,
> +               .scan_index = -1

Ditto.

>                 .indexed = 1,
>                 .channel = INGENIC_ADC_AUX2,
> +               .scan_index = -1

Ditto.

...

> +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
> +{
> +       struct ingenic_adc *adc = iio_priv(iio_dev);
> +

> +       clk_enable(adc->clk);

Error check?

> +       /* It takes significant time for the touchscreen hw to stabilize. */
> +       msleep(50);
> +       ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
> +                              JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
> +                              JZ_ADC_REG_CFG_PULL_UP(4));
> +       writew(80, adc->base + JZ_ADC_REG_ADWAIT);
> +       writew(2, adc->base + JZ_ADC_REG_ADSAME);

> +       writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);

Why casting?

> +       writel(0, adc->base + JZ_ADC_REG_ADTCH);
> +       ingenic_adc_enable(adc, 2, true);
> +
> +       return 0;
> +}

> +       irq = platform_get_irq(pdev, 0);

Before it worked w/o IRQ, here is a regression you introduced.

> +       if (irq < 0) {

> +               dev_err(dev, "Failed to get irq: %d\n", irq);

Redundant message.

> +               return irq;
> +       }

Paul Cercueil April 17, 2020, 9:04 p.m. UTC | #2

Hi Andy,

Le ven. 17 avril 2020 à 23:59, Andy Shevchenko 
<[e-mail]> a écrit :
> On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <[e-mail]> 
> wrote:
>> 
>>  The SADC component in JZ47xx SoCs provides support for touchscreen
>>  operations (pen position and pen down pressure) in single-ended and
>>  differential modes.
>> 
>>  Of the known hardware to use this controller, GCW Zero and Anbernic 
>> RG-350
>>  utilize the touchscreen mode by having their joystick(s) attached 
>> to the
>>  X/Y positive/negative input pins.
>>  GCW Zero comes with a single joystick and is sufficiently handled 
>> with the
>>  currently implemented single-ended mode. Support for boards with two
>>  joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp 
>> channels
>>  will need to be provided in the future.
>> 
>>  The touchscreen component of SADC takes a significant time to 
>> stabilize
>>  after first receiving the clock and a delay of 50ms has been 
>> empirically
>>  proven to be a safe value before data sampling can begin.
>> 
>>  All the boards which probe this driver have the interrupt provided 
>> from
>>  devicetree, with no need to handle a case where the irq was not 
>> provided.
> 
> Device Tree
> IRQ
> 
> ...
> 
>>  +               .scan_type = {
>>  +                       .sign = 'u',
>>  +                       .realbits = 12,
> 
>>  +                       .storagebits = 16
> 
> It's slightly better to leave comma in such cases.
> 
>>  +               },
> 
>>  +               .scan_type = {
>>  +                       .sign = 'u',
>>  +                       .realbits = 12,
> 
>>  +                       .storagebits = 16
> 
> Ditto.
> 
>>  +               },
> 
> ...
> 
>>                  .indexed = 1,
>>                  .channel = INGENIC_ADC_AUX,
>>  +               .scan_index = -1
> 
> Ditto. You see above? Isn't it nice that you didn't touch that line?
> So, perhaps next developer can leverage this subtle kind of things.
> 
>>                  .indexed = 1,
>>                  .channel = INGENIC_ADC_BATTERY,
>>  +               .scan_index = -1
> 
> Ditto.
> 
>>                  .indexed = 1,
>>                  .channel = INGENIC_ADC_AUX2,
>>  +               .scan_index = -1
> 
> Ditto.
> 
> ...
> 
>>  +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
>>  +{
>>  +       struct ingenic_adc *adc = iio_priv(iio_dev);
>>  +
> 
>>  +       clk_enable(adc->clk);
> 
> Error check?
> 
>>  +       /* It takes significant time for the touchscreen hw to 
>> stabilize. */
>>  +       msleep(50);
>>  +       ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
>>  +                              JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
>>  +                              JZ_ADC_REG_CFG_PULL_UP(4));
>>  +       writew(80, adc->base + JZ_ADC_REG_ADWAIT);
>>  +       writew(2, adc->base + JZ_ADC_REG_ADSAME);
> 
>>  +       writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
> 
> Why casting?
> 
>>  +       writel(0, adc->base + JZ_ADC_REG_ADTCH);
>>  +       ingenic_adc_enable(adc, 2, true);
>>  +
>>  +       return 0;
>>  +}
> 
>>  +       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.

-Paul

> 
>>  +       if (irq < 0) {
> 
>>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
> 
> Redundant message.
> 
>>  +               return irq;
>>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

Andy Shevchenko April 17, 2020, 9:13 p.m. UTC | #3

On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil <[e-mail]> wrote:
> Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> <[e-mail]> a écrit :
> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <[e-mail]>
> > 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.

> >>  +       if (irq < 0) {
> >
> >>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
> >
> > Redundant message.
> >
> >>  +               return irq;
> >>  +       }

Paul Cercueil April 17, 2020, 9:18 p.m. UTC | #4

Le sam. 18 avril 2020 à 0:13, Andy Shevchenko 
<[e-mail]> a écrit :
> On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil <[e-mail]> 
> wrote:
>>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  <[e-mail]> a écrit :
>>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek 
>> <[e-mail]>
>>  > 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.

-Paul


>>  >>  +       if (irq < 0) {
>>  >
>>  >>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
>>  >
>>  > Redundant message.
>>  >
>>  >>  +               return irq;
>>  >>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

Andy Shevchenko April 17, 2020, 9:42 p.m. UTC | #5

On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <[e-mail]> wrote:
> Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
> <[e-mail]> a écrit :
> > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil <[e-mail]>
> > wrote:
> >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> >>  <[e-mail]> a écrit :
> >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> >> <[e-mail]>
> >>  > 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?

> >>  >>  +       if (irq < 0) {
> >>  >
> >>  >>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
> >>  >
> >>  > Redundant message.
> >>  >
> >>  >>  +               return irq;
> >>  >>  +       }

Paul Cercueil April 17, 2020, 9:45 p.m. UTC | #6

Le sam. 18 avril 2020 à 0:42, Andy Shevchenko 
<[e-mail]> a écrit :
> On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <[e-mail]> 
> wrote:
>>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>>  <[e-mail]> a écrit :
>>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil 
>> <[e-mail]>
>>  > wrote:
>>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  >>  <[e-mail]> a écrit :
>>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >> <[e-mail]>
>>  >>  > 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.

-Paul

>>  >>  >>  +       if (irq < 0) {
>>  >>  >
>>  >>  >>  +               dev_err(dev, "Failed to get irq: %d\n", 
>> irq);
>>  >>  >
>>  >>  > Redundant message.
>>  >>  >
>>  >>  >>  +               return irq;
>>  >>  >>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

Andy Shevchenko April 17, 2020, 9:52 p.m. UTC | #7

On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <[e-mail]> wrote:
> Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
> <[e-mail]> a écrit :
> > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <[e-mail]>
> > wrote:
> >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
> >>  <[e-mail]> a écrit :
> >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
> >> <[e-mail]>
> >>  > wrote:
> >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> >>  >>  <[e-mail]> a écrit :
> >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> >>  >> <[e-mail]>
> >>  >>  > 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.

> >>  >>  >>  +       if (irq < 0) {
> >>  >>  >
> >>  >>  >>  +               dev_err(dev, "Failed to get irq: %d\n",
> >> irq);
> >>  >>  >
> >>  >>  > Redundant message.
> >>  >>  >
> >>  >>  >>  +               return irq;
> >>  >>  >>  +       }

Paul Cercueil April 17, 2020, 9:56 p.m. UTC | #8

Le sam. 18 avril 2020 à 0:52, Andy Shevchenko 
<[e-mail]> a écrit :
> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <[e-mail]> 
> wrote:
>>  Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>>  <[e-mail]> a écrit :
>>  > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil 
>> <[e-mail]>
>>  > wrote:
>>  >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>>  >>  <[e-mail]> a écrit :
>>  >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>>  >> <[e-mail]>
>>  >>  > wrote:
>>  >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  >>  >>  <[e-mail]> a écrit :
>>  >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >>  >> <[e-mail]>
>>  >>  >>  > 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 can, since I wrote all of them.

-Paul

> I'm not going to push this anyway -- your choice.
> 
> But I see a (potential) regression.
> 
>>  >>  >>  >>  +       if (irq < 0) {
>>  >>  >>  >
>>  >>  >>  >>  +               dev_err(dev, "Failed to get irq: %d\n",
>>  >> irq);
>>  >>  >>  >
>>  >>  >>  > Redundant message.
>>  >>  >>  >
>>  >>  >>  >>  +               return irq;
>>  >>  >>  >>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

Artur Rojek April 19, 2020, 12:19 p.m. UTC | #9

Hi Andy,
thanks for the review. Comments inline.

On 2020-04-17 22:59, Andy Shevchenko wrote:
> On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <[e-mail]> 
> wrote:
>> 
>> The SADC component in JZ47xx SoCs provides support for touchscreen
>> operations (pen position and pen down pressure) in single-ended and
>> differential modes.
>> 
>> Of the known hardware to use this controller, GCW Zero and Anbernic 
>> RG-350
>> utilize the touchscreen mode by having their joystick(s) attached to 
>> the
>> X/Y positive/negative input pins.
>> GCW Zero comes with a single joystick and is sufficiently handled with 
>> the
>> currently implemented single-ended mode. Support for boards with two
>> joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp 
>> channels
>> will need to be provided in the future.
>> 
>> The touchscreen component of SADC takes a significant time to 
>> stabilize
>> after first receiving the clock and a delay of 50ms has been 
>> empirically
>> proven to be a safe value before data sampling can begin.
>> 
>> All the boards which probe this driver have the interrupt provided 
>> from
>> devicetree, with no need to handle a case where the irq was not 
>> provided.
> 
> Device Tree
> IRQ
> 
> ...
> 
>> +               .scan_type = {
>> +                       .sign = 'u',
>> +                       .realbits = 12,
> 
>> +                       .storagebits = 16
> 
> It's slightly better to leave comma in such cases.
> 
>> +               },
> 
>> +               .scan_type = {
>> +                       .sign = 'u',
>> +                       .realbits = 12,
> 
>> +                       .storagebits = 16
> 
> Ditto.
> 
>> +               },
> 
> ...
> 
>>                 .indexed = 1,
>>                 .channel = INGENIC_ADC_AUX,
>> +               .scan_index = -1
> 
> Ditto. You see above? Isn't it nice that you didn't touch that line?
> So, perhaps next developer can leverage this subtle kind of things.
> 
>>                 .indexed = 1,
>>                 .channel = INGENIC_ADC_BATTERY,
>> +               .scan_index = -1
> 
> Ditto.
> 
>>                 .indexed = 1,
>>                 .channel = INGENIC_ADC_AUX2,
>> +               .scan_index = -1
> 
> Ditto.
> 
> ...
> 
>> +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
>> +{
>> +       struct ingenic_adc *adc = iio_priv(iio_dev);
>> +
> 
>> +       clk_enable(adc->clk);
> 
> Error check?
> 
>> +       /* It takes significant time for the touchscreen hw to 
>> stabilize. */
>> +       msleep(50);
>> +       ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
>> +                              JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
>> +                              JZ_ADC_REG_CFG_PULL_UP(4));
>> +       writew(80, adc->base + JZ_ADC_REG_ADWAIT);
>> +       writew(2, adc->base + JZ_ADC_REG_ADSAME);
> 
>> +       writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
> 
> Why casting?
After flipping the bits, the resulting value can't be represented by u8. 
Since we care only about the first 8 bits anyway, explicit cast here is 
to silence a compiler warning.
> 
>> +       writel(0, adc->base + JZ_ADC_REG_ADTCH);
>> +       ingenic_adc_enable(adc, 2, true);
>> +
>> +       return 0;
>> +}
> 
>> +       irq = platform_get_irq(pdev, 0);
> 
> Before it worked w/o IRQ, here is a regression you introduced.
> 
>> +       if (irq < 0) {
> 
>> +               dev_err(dev, "Failed to get irq: %d\n", irq);
> 
> Redundant message.
> 
>> +               return irq;
>> +       }

- Artur

Ezequiel Garcia April 19, 2020, 12:54 p.m. UTC | #10

On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko <[e-mail]> wrote:
>
> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <[e-mail]> wrote:
> > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
> > <[e-mail]> a écrit :
> > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <[e-mail]>
> > > wrote:
> > >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
> > >>  <[e-mail]> a écrit :
> > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
> > >> <[e-mail]>
> > >>  > wrote:
> > >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> > >>  >>  <[e-mail]> a écrit :
> > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> > >>  >> <[e-mail]>
> > >>  >>  > 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?

Cheers,
Ezequiel

Paul Cercueil April 19, 2020, 1:23 p.m. UTC | #11

Hi Ezequiel,


Le dim. 19 avril 2020 à 9:54, Ezequiel Garcia 
<[e-mail]> a écrit :
> On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko 
> <[e-mail]> wrote:
>> 
>>  On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil 
>> <[e-mail]> wrote:
>>  > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>>  > <[e-mail]> a écrit :
>>  > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil 
>> <[e-mail]>
>>  > > wrote:
>>  > >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>>  > >>  <[e-mail]> a écrit :
>>  > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>>  > >> <[e-mail]>
>>  > >>  > wrote:
>>  > >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  > >>  >>  <[e-mail]> a écrit :
>>  > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  > >>  >> <[e-mail]>
>>  > >>  >>  > 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

Artur Rojek April 19, 2020, 1:31 p.m. UTC | #12

Hi Ezequiel,

On 2020-04-19 14:54, Ezequiel Garcia wrote:
> On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko 
> <[e-mail]> wrote:
>> 
>> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <[e-mail]> 
>> wrote:
>> > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>> > <[e-mail]> a écrit :
>> > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <[e-mail]>
>> > > wrote:
>> > >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>> > >>  <[e-mail]> a écrit :
>> > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>> > >> <[e-mail]>
>> > >>  > wrote:
>> > >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>> > >>  >>  <[e-mail]> a écrit :
>> > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>> > >>  >> <[e-mail]>
>> > >>  >>  > 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

Patch

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 12bb8b7ca1ff..c3314135fa2a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -465,6 +465,7 @@  config INA2XX_ADC
 config INGENIC_ADC
 	tristate "Ingenic JZ47xx SoCs ADC driver"
 	depends on MIPS || COMPILE_TEST
+	select IIO_BUFFER
 	help
 	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC unit.
 
diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index 7a24bc1dabe1..0dafc8d5d0d8 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -8,7 +8,9 @@ 
 
 #include <dt-bindings/iio/adc/ingenic,adc.h>
 #include <linux/clk.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -20,6 +22,8 @@ 
 #define JZ_ADC_REG_CFG			0x04
 #define JZ_ADC_REG_CTRL			0x08
 #define JZ_ADC_REG_STATUS		0x0c
+#define JZ_ADC_REG_ADSAME		0x10
+#define JZ_ADC_REG_ADWAIT		0x14
 #define JZ_ADC_REG_ADTCH		0x18
 #define JZ_ADC_REG_ADBDAT		0x1c
 #define JZ_ADC_REG_ADSDAT		0x20
@@ -28,6 +32,9 @@ 
 #define JZ_ADC_REG_ENABLE_PD		BIT(7)
 #define JZ_ADC_REG_CFG_AUX_MD		(BIT(0) | BIT(1))
 #define JZ_ADC_REG_CFG_BAT_MD		BIT(4)
+#define JZ_ADC_REG_CFG_PULL_UP(n)	((n) << 16)
+#define JZ_ADC_REG_CFG_SAMPLE_NUM(n)	((n) << 10)
+#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK	(BIT(31) | GENMASK(23, 10))
 #define JZ_ADC_REG_ADCLK_CLKDIV_LSB	0
 #define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB	16
 #define JZ4770_ADC_REG_ADCLK_CLKDIV10US_LSB	8
@@ -44,6 +51,14 @@ 
 #define JZ4770_ADC_BATTERY_VREF			6600
 #define JZ4770_ADC_BATTERY_VREF_BITS		12
 
+#define JZ_ADC_IRQ_AUX			BIT(0)
+#define JZ_ADC_IRQ_BATTERY		BIT(1)
+#define JZ_ADC_IRQ_TOUCH		BIT(2)
+#define JZ_ADC_IRQ_PEN_DOWN		BIT(3)
+#define JZ_ADC_IRQ_PEN_UP		BIT(4)
+#define JZ_ADC_IRQ_PEN_DOWN_SLEEP	BIT(5)
+#define JZ_ADC_IRQ_SLEEP		BIT(7)
+
 struct ingenic_adc;
 
 struct ingenic_adc_soc_data {
@@ -411,6 +426,28 @@  static const struct iio_info ingenic_adc_info = {
 };
 
 static const struct iio_chan_spec ingenic_channels[] = {
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_XP,
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16
+		},
+	},
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_YP,
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16
+		},
+	},
 	{
 		.extend_name = "aux",
 		.type = IIO_VOLTAGE,
@@ -418,6 +455,7 @@  static const struct iio_chan_spec ingenic_channels[] = {
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
 		.channel = INGENIC_ADC_AUX,
+		.scan_index = -1
 	},
 	{
 		.extend_name = "battery",
@@ -428,6 +466,7 @@  static const struct iio_chan_spec ingenic_channels[] = {
 						BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
 		.channel = INGENIC_ADC_BATTERY,
+		.scan_index = -1
 	},
 	{ /* Must always be last in the array. */
 		.extend_name = "aux2",
@@ -436,16 +475,69 @@  static const struct iio_chan_spec ingenic_channels[] = {
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
 		.channel = INGENIC_ADC_AUX2,
+		.scan_index = -1
 	},
 };
 
+static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+
+	clk_enable(adc->clk);
+	/* It takes significant time for the touchscreen hw to stabilize. */
+	msleep(50);
+	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
+			       JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
+			       JZ_ADC_REG_CFG_PULL_UP(4));
+	writew(80, adc->base + JZ_ADC_REG_ADWAIT);
+	writew(2, adc->base + JZ_ADC_REG_ADSAME);
+	writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
+	writel(0, adc->base + JZ_ADC_REG_ADTCH);
+	ingenic_adc_enable(adc, 2, true);
+
+	return 0;
+}
+
+static int ingenic_adc_buffer_disable(struct iio_dev *iio_dev)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+
+	ingenic_adc_enable(adc, 2, false);
+	writeb(0xff, adc->base + JZ_ADC_REG_CTRL);
+	writeb(0xff, adc->base + JZ_ADC_REG_STATUS);
+	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK, 0);
+	writew(0, adc->base + JZ_ADC_REG_ADSAME);
+	writew(0, adc->base + JZ_ADC_REG_ADWAIT);
+	clk_disable(adc->clk);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ingenic_buffer_setup_ops = {
+	.postenable = &ingenic_adc_buffer_enable,
+	.predisable = &ingenic_adc_buffer_disable
+};
+
+static irqreturn_t ingenic_adc_irq(int irq, void *data)
+{
+	struct iio_dev *iio_dev = data;
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+	u32 tdat;
+
+	tdat = readl(adc->base + JZ_ADC_REG_ADTCH);
+	iio_push_to_buffers(iio_dev, &tdat);
+	writeb(JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_STATUS);
+
+	return IRQ_HANDLED;
+}
+
 static int ingenic_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct iio_dev *iio_dev;
 	struct ingenic_adc *adc;
 	const struct ingenic_adc_soc_data *soc_data;
-	int ret;
+	int irq, ret;
 
 	soc_data = device_get_match_data(dev);
 	if (!soc_data)
@@ -460,6 +552,18 @@  static int ingenic_adc_probe(struct platform_device *pdev)
 	mutex_init(&adc->aux_lock);
 	adc->soc_data = soc_data;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "Failed to get irq: %d\n", irq);
+		return irq;
+	}
+	ret = devm_request_irq(dev, irq, ingenic_adc_irq, 0,
+			       dev_name(dev), iio_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to request irq: %d\n", ret);
+		return ret;
+	}
+
 	adc->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(adc->base))
 		return PTR_ERR(adc->base);
@@ -499,7 +603,8 @@  static int ingenic_adc_probe(struct platform_device *pdev)
 
 	iio_dev->dev.parent = dev;
 	iio_dev->name = "jz-adc";
-	iio_dev->modes = INDIO_DIRECT_MODE;
+	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	iio_dev->setup_ops = &ingenic_buffer_setup_ops;
 	iio_dev->channels = ingenic_channels;
 	iio_dev->num_channels = ARRAY_SIZE(ingenic_channels);
 	/* Remove AUX2 from the list of supported channels. */