[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <3KAY8Q.NNI6X4F9QRIX1@crapouillou.net>
Date: Fri, 17 Apr 2020 23:04:51 +0200
From: Paul Cercueil <paul@...pouillou.net>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Artur Rojek <contact@...ur-rojek.eu>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Jonathan Cameron <jic23@...nel.org>,
Heiko Stuebner <heiko@...ech.de>,
linux-input <linux-input@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
Hi Andy,
Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
<andy.shevchenko@...il.com> a écrit :
> On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@...ur-rojek.eu>
> 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
Powered by blists - more mailing lists