lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75Vcwnu8tw92nMYc_5-x_iX+FY8_OhtaJkSYNehmNUDkHGQ@mail.gmail.com>
Date:   Fri, 17 Apr 2020 23:59:03 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Artur Rojek <contact@...ur-rojek.eu>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jonathan Cameron <jic23@...nel.org>,
        Paul Cercueil <paul@...pouillou.net>,
        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.

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.

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

Powered by Openwall GNU/*/Linux Powered by OpenVZ