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






[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