Messages in this thread |  | | Date | Sun, 12 Jul 2020 22:16:04 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH v8 6/6] IIO: Ingenic JZ47xx: Add touchscreen mode. |
| |
Hi Jonathan,
Le dim. 12 juil. 2020 à 14:19, Jonathan Cameron <jic23@kernel.org> a écrit : > On Thu, 9 Jul 2020 17:22:00 +0200 > Artur Rojek <contact@artur-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. >> >> 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. >> >> 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. >> >> JZ4770 and later SoCs introduce a low-level command feature. With >> it, up >> to 32 commands can be programmed, each one corresponding to a >> sampling >> job. It allows to change the low-voltage reference, the high-voltage >> reference, have them connected to VCC, GND, or one of the X-/X+ or >> Y-/Y+ >> pins. >> >> This patch introduces support for 6 stream-capable channels: >> - channel #0 samples X+/GND >> - channel #1 samples Y+/GND >> - channel #2 samples X-/GND >> - channel #3 samples Y-/GND >> - channel #4 samples X+/X- >> - channel #5 samples Y+/Y- > > The one thing I noticed on this read was that we are slightly > stretching > the normal IIO channel definitions. The claim is that each of these > channels > is a POSITIONREALTIVE channel, whereas that isn't really true. They > are > related to the position, but as I understand it not directly > measuring it > (particularly X+ - X-) which is pretty much a reference voltage (I > think!) > > We might be better off just describing these as voltage channels, with > 4 and 5 described as differential voltage channels. The problem there > being that it doesn't describe the fact that the measurements are with > particular voltages also being applied to the touch screen. > > Perhaps we are best off just leaving it as you have it and being a bit > 'odd'. What do you think? Having written this down I think perhaps > leaving > it alone is the best plan :(
That's up to you really. These ADC channels will measure voltage, but they are supposed to be used with a touchscreen, that's why we went with POSITIONRELATIVE. However, with the kind of devices we're working with, they are never used as such, so using VOLTAGE would work too.
> Otherwise this all looks fine to me. > > Thanks, > > Jonathan > >> >> Being able to sample X-/GND and Y-/GND is useful on some devices, >> where >> one joystick is connected to the X+/Y+ pins, and a second joystick >> is >> connected to the X-/Y- pins. >> >> All the boards which probe this driver have the interrupt provided >> from >> Device Tree, with no need to handle a case where the IRQ was not >> provided. >> >> Co-developed-by: Paul Cercueil <paul@crapouillou.net> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> Signed-off-by: Artur Rojek <contact@artur-rojek.eu> >> --- >> >> Changes: >> >> v2: - improve description of the touchscreen mode, >> - get rid of the unneeded kfifo, >> - drop IIO_BUFFER_CB from Kconfig, >> - remove extended names from the touchscreen channels >> >> v3: remove unneeded `linux/iio/kfifo_buf.h` include >> >> v4: clarify irq provider source in the patch description >> >> v5: no change >> >> v6: - correct the spelling of Device Tree and IRQ in commit message >> - don't omit trailing commas from initializer lists >> - error check `clk_enable` >> - remove redundant `dev_err` from `platform_get_irq` error >> check >> >> v7: no change >> >> v8: add support for ADCMD low-level command feature >> >> drivers/iio/adc/Kconfig | 1 + >> drivers/iio/adc/ingenic-adc.c | 250 >> +++++++++++++++++++++++++++++++++- >> 2 files changed, 249 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index ff3569635ce0..5b57437cef75 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -500,6 +500,7 @@ config INA2XX_ADC >> config INGENIC_ADC >> tristate "Ingenic JZ47xx SoCs ADC driver" >> depends on MIPS || COMPILE_TEST >> + select IIO_BUFFER >> help >> Say yes here to build support for the Ingenic JZ47xx SoCs ADC >> unit. >> >> diff --git a/drivers/iio/adc/ingenic-adc.c >> b/drivers/iio/adc/ingenic-adc.c >> index 0233a9055c86..976aea46fede 100644 >> --- a/drivers/iio/adc/ingenic-adc.c >> +++ b/drivers/iio/adc/ingenic-adc.c >> @@ -8,7 +8,9 @@ >> >> #include <dt-bindings/iio/adc/ingenic,adc.h> >> #include <linux/clk.h> >> +#include <linux/iio/buffer.h> >> #include <linux/iio/iio.h> >> +#include <linux/interrupt.h> >> #include <linux/io.h> >> #include <linux/iopoll.h> >> #include <linux/kernel.h> >> @@ -20,19 +22,46 @@ >> #define JZ_ADC_REG_CFG 0x04 >> #define JZ_ADC_REG_CTRL 0x08 >> #define JZ_ADC_REG_STATUS 0x0c >> +#define JZ_ADC_REG_ADSAME 0x10 >> +#define JZ_ADC_REG_ADWAIT 0x14 >> #define JZ_ADC_REG_ADTCH 0x18 >> #define JZ_ADC_REG_ADBDAT 0x1c >> #define JZ_ADC_REG_ADSDAT 0x20 >> +#define JZ_ADC_REG_ADCMD 0x24 >> #define JZ_ADC_REG_ADCLK 0x28 >> >> #define JZ_ADC_REG_ENABLE_PD BIT(7) >> #define JZ_ADC_REG_CFG_AUX_MD (BIT(0) | BIT(1)) >> #define JZ_ADC_REG_CFG_BAT_MD BIT(4) >> +#define JZ_ADC_REG_CFG_SAMPLE_NUM(n) ((n) << 10) >> +#define JZ_ADC_REG_CFG_PULL_UP(n) ((n) << 16) >> +#define JZ_ADC_REG_CFG_CMD_SEL BIT(22) >> +#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK (BIT(31) | GENMASK(23, 10)) >> #define JZ_ADC_REG_ADCLK_CLKDIV_LSB 0 >> #define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB 16 >> #define JZ4770_ADC_REG_ADCLK_CLKDIV10US_LSB 8 >> #define JZ4770_ADC_REG_ADCLK_CLKDIVMS_LSB 16 >> >> +#define JZ_ADC_REG_ADCMD_YNADC BIT(7) >> +#define JZ_ADC_REG_ADCMD_YPADC BIT(8) >> +#define JZ_ADC_REG_ADCMD_XNADC BIT(9) >> +#define JZ_ADC_REG_ADCMD_XPADC BIT(10) >> +#define JZ_ADC_REG_ADCMD_VREFPYP BIT(11) >> +#define JZ_ADC_REG_ADCMD_VREFPXP BIT(12) >> +#define JZ_ADC_REG_ADCMD_VREFPXN BIT(13) >> +#define JZ_ADC_REG_ADCMD_VREFPAUX BIT(14) >> +#define JZ_ADC_REG_ADCMD_VREFPVDD33 BIT(15) >> +#define JZ_ADC_REG_ADCMD_VREFNYN BIT(16) >> +#define JZ_ADC_REG_ADCMD_VREFNXP BIT(17) >> +#define JZ_ADC_REG_ADCMD_VREFNXN BIT(18) >> +#define JZ_ADC_REG_ADCMD_VREFAUX BIT(19) >> +#define JZ_ADC_REG_ADCMD_YNGRU BIT(20) >> +#define JZ_ADC_REG_ADCMD_XNGRU BIT(21) >> +#define JZ_ADC_REG_ADCMD_XPGRU BIT(22) >> +#define JZ_ADC_REG_ADCMD_YPSUP BIT(23) >> +#define JZ_ADC_REG_ADCMD_XNSUP BIT(24) >> +#define JZ_ADC_REG_ADCMD_XPSUP BIT(25) >> + >> #define JZ_ADC_AUX_VREF 3300 >> #define JZ_ADC_AUX_VREF_BITS 12 >> #define JZ_ADC_BATTERY_LOW_VREF 2500 >> @@ -44,6 +73,14 @@ >> #define JZ4770_ADC_BATTERY_VREF 6600 >> #define JZ4770_ADC_BATTERY_VREF_BITS 12 >> >> +#define JZ_ADC_IRQ_AUX BIT(0) >> +#define JZ_ADC_IRQ_BATTERY BIT(1) >> +#define JZ_ADC_IRQ_TOUCH BIT(2) >> +#define JZ_ADC_IRQ_PEN_DOWN BIT(3) >> +#define JZ_ADC_IRQ_PEN_UP BIT(4) >> +#define JZ_ADC_IRQ_PEN_DOWN_SLEEP BIT(5) >> +#define JZ_ADC_IRQ_SLEEP BIT(7) >> + >> struct ingenic_adc; >> >> struct ingenic_adc_soc_data { >> @@ -69,6 +106,61 @@ struct ingenic_adc { >> bool low_vref_mode; >> }; >> >> +static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, >> unsigned long mask) >> +{ >> + struct ingenic_adc *adc = iio_priv(iio_dev); >> + >> + mutex_lock(&adc->lock); >> + >> + /* Init ADCMD */ >> + readl(adc->base + JZ_ADC_REG_ADCMD); >> + >> + if (mask & 0x3) { >> + /* Second channel (INGENIC_ADC_TOUCH_YP): sample YP vs. GND */ >> + writel(JZ_ADC_REG_ADCMD_XNGRU >> + | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33 >> + | JZ_ADC_REG_ADCMD_YPADC, >> + adc->base + JZ_ADC_REG_ADCMD); >> + >> + /* First channel (INGENIC_ADC_TOUCH_XP): sample XP vs. GND */ >> + writel(JZ_ADC_REG_ADCMD_YNGRU >> + | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33 >> + | JZ_ADC_REG_ADCMD_XPADC, >> + adc->base + JZ_ADC_REG_ADCMD); >> + } >> + >> + if (mask & 0xc) { >> + /* Fourth channel (INGENIC_ADC_TOUCH_YN): sample YN vs. GND */ >> + writel(JZ_ADC_REG_ADCMD_XNGRU >> + | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33 >> + | JZ_ADC_REG_ADCMD_YNADC, >> + adc->base + JZ_ADC_REG_ADCMD); >> + >> + /* Third channel (INGENIC_ADC_TOUCH_XN): sample XN vs. GND */ >> + writel(JZ_ADC_REG_ADCMD_YNGRU >> + | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33 >> + | JZ_ADC_REG_ADCMD_XNADC, >> + adc->base + JZ_ADC_REG_ADCMD); >> + } >> + >> + if (mask & 0x30) { >> + /* Sixth channel (INGENIC_ADC_TOUCH_YD): sample YP vs. YN */ >> + writel(JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33 >> + | JZ_ADC_REG_ADCMD_YPADC, >> + adc->base + JZ_ADC_REG_ADCMD); >> + >> + /* Fifth channel (INGENIC_ADC_TOUCH_XD): sample XP vs. XN */ >> + writel(JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33 >> + | JZ_ADC_REG_ADCMD_XPADC, >> + adc->base + JZ_ADC_REG_ADCMD); >> + } >> + >> + /* We're done */ >> + writel(0, adc->base + JZ_ADC_REG_ADCMD); >> + >> + mutex_unlock(&adc->lock); >> +} >> + >> static void ingenic_adc_set_config(struct ingenic_adc *adc, >> uint32_t mask, >> uint32_t val) >> @@ -288,6 +380,72 @@ static const struct iio_chan_spec >> jz4740_channels[] = { >> }; >> >> static const struct iio_chan_spec jz4770_channels[] = { >> + { >> + .type = IIO_POSITIONRELATIVE, >> + .indexed = 1, >> + .channel = INGENIC_ADC_TOUCH_XP, > > Ah. sorry. I should have noticed this much earlier. > > These aren't position channels as such. They are channels > that are then processed into position in combination. > > Problem is I'm not sure how we 'should' describe then. > > Perhaps this is the best we can do.
We could have the two differencial channels as IIO_POSITIONRELATIVE, and the four single-ended ones as IIO_VOLTAGE.
Cheers, -Paul
>> + .scan_index = 0, >> + .scan_type = { >> + .sign = 'u', >> + .realbits = 12, >> + .storagebits = 16, >> + }, >> + }, >> + { >> + .type = IIO_POSITIONRELATIVE, >> + .indexed = 1, >> + .channel = INGENIC_ADC_TOUCH_YP, >> + .scan_index = 1, >> + .scan_type = { >> + .sign = 'u', >> + .realbits = 12, >> + .storagebits = 16, >> + }, >> + }, >> + { >> + .type = IIO_POSITIONRELATIVE, >> + .indexed = 1, >> + .channel = INGENIC_ADC_TOUCH_XN, >> + .scan_index = 2, >> + .scan_type = { >> + .sign = 'u', >> + .realbits = 12, >> + .storagebits = 16, >> + }, >> + }, >> + { >> + .type = IIO_POSITIONRELATIVE, >> + .indexed = 1, >> + .channel = INGENIC_ADC_TOUCH_YN, >> + .scan_index = 3, >> + .scan_type = { >> + .sign = 'u', >> + .realbits = 12, >> + .storagebits = 16, >> + }, >> + }, >> + { >> + .type = IIO_POSITIONRELATIVE, >> + .indexed = 1, >> + .channel = INGENIC_ADC_TOUCH_XD, >> + .scan_index = 4, >> + .scan_type = { >> + .sign = 'u', >> + .realbits = 12, >> + .storagebits = 16, >> + }, >> + }, >> + { >> + .type = IIO_POSITIONRELATIVE, >> + .indexed = 1, >> + .channel = INGENIC_ADC_TOUCH_YD, >> + .scan_index = 5, >> + .scan_type = { >> + .sign = 'u', >> + .realbits = 12, >> + .storagebits = 16, >> + }, >> + }, >> { >> .extend_name = "aux", >> .type = IIO_VOLTAGE, >> @@ -490,13 +648,89 @@ static const struct iio_info ingenic_adc_info >> = { >> .of_xlate = ingenic_adc_of_xlate, >> }; >> >> +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev) >> +{ >> + struct ingenic_adc *adc = iio_priv(iio_dev); >> + int ret; >> + >> + ret = clk_enable(adc->clk); >> + if (ret) { >> + dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n", >> + ret); >> + return ret; >> + } >> + >> + /* 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); >> + writel(0, adc->base + JZ_ADC_REG_ADTCH); >> + >> + ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL, >> + JZ_ADC_REG_CFG_CMD_SEL); >> + ingenic_adc_set_adcmd(iio_dev, iio_dev->active_scan_mask[0]); >> + >> + ingenic_adc_enable(adc, 2, true); >> + >> + return 0; >> +} >> + >> +static int ingenic_adc_buffer_disable(struct iio_dev *iio_dev) >> +{ >> + struct ingenic_adc *adc = iio_priv(iio_dev); >> + >> + ingenic_adc_enable(adc, 2, false); >> + >> + ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL, 0); >> + >> + writeb(0xff, adc->base + JZ_ADC_REG_CTRL); >> + writeb(0xff, adc->base + JZ_ADC_REG_STATUS); >> + ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK, 0); >> + writew(0, adc->base + JZ_ADC_REG_ADSAME); >> + writew(0, adc->base + JZ_ADC_REG_ADWAIT); >> + clk_disable(adc->clk); >> + >> + return 0; >> +} >> + >> +static const struct iio_buffer_setup_ops ingenic_buffer_setup_ops >> = { >> + .postenable = &ingenic_adc_buffer_enable, >> + .predisable = &ingenic_adc_buffer_disable >> +}; >> + >> +static irqreturn_t ingenic_adc_irq(int irq, void *data) >> +{ >> + struct iio_dev *iio_dev = data; >> + struct ingenic_adc *adc = iio_priv(iio_dev); >> + unsigned long mask = iio_dev->active_scan_mask[0]; >> + unsigned int i; >> + u32 tdat[3]; >> + >> + for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) { >> + if (mask & 0x3) >> + tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH); >> + else >> + tdat[i] = 0; >> + } >> + >> + iio_push_to_buffers(iio_dev, tdat); >> + writeb(JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_STATUS); >> + >> + return IRQ_HANDLED; >> +} >> + >> static int ingenic_adc_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct iio_dev *iio_dev; >> struct ingenic_adc *adc; >> const struct ingenic_adc_soc_data *soc_data; >> - int ret; >> + int irq, ret; >> >> soc_data = device_get_match_data(dev); >> if (!soc_data) >> @@ -511,6 +745,17 @@ static int ingenic_adc_probe(struct >> platform_device *pdev) >> mutex_init(&adc->aux_lock); >> adc->soc_data = soc_data; >> >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; >> + >> + ret = devm_request_irq(dev, irq, ingenic_adc_irq, 0, >> + dev_name(dev), iio_dev); >> + if (ret < 0) { >> + dev_err(dev, "Failed to request irq: %d\n", ret); >> + return ret; >> + } >> + >> adc->base = devm_platform_ioremap_resource(pdev, 0); >> if (IS_ERR(adc->base)) >> return PTR_ERR(adc->base); >> @@ -550,7 +795,8 @@ static int ingenic_adc_probe(struct >> platform_device *pdev) >> >> iio_dev->dev.parent = dev; >> iio_dev->name = "jz-adc"; >> - iio_dev->modes = INDIO_DIRECT_MODE; >> + iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; >> + iio_dev->setup_ops = &ingenic_buffer_setup_ops; >> iio_dev->channels = soc_data->channels; >> iio_dev->num_channels = soc_data->num_channels; >> iio_dev->info = &ingenic_adc_info; >
|  |