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