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

[Paul Cercueil]

Hi Andy,

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:
> >  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