On Sunday 20 July 2014 23:37:18 Hartmut Knaack wrote:
> Jonathan Cameron schrieb:
> > On 18/07/14 20:29, Arnd Bergmann wrote:
> >> - I simply register the input device from the adc driver
> >> itself, as the at91 code does. The driver also supports
> >> sub-nodes, but I don't understand how they are meant
> >> to be used, so using those might be better.
> > So, the alternative you are (I think) referring to is to use
> > the buffered in kernel client interface. That way a separate
> > touch screen driver can use the output channels provided by IIO
> > in much the same way you might use a regulator or similar.
> > Note that whilst this is similar to the simple polled interface
> > used for things like the iio_hwmon driver, the data flow is
> > quite different (clearly the polled interfce would be
> > inappropriate here).
> >
> > Whilst we've discussed it in the past for touch screen drivers
> > like this, usually the hardware isn't generic enough to be
> > of any real use if not being used as a touch screen. As such
> > it's often simpler to just have the support directly in the
> > driver (as you've observed the at91 driver does this).
Ok, I see. That's exactly the information I was looking for.
> >
> > Whilst the interface has been there a while, it's not really had
> > all that much use. The original target was the simpler case
> > of 3D accelerometer where we have a generic iio to input
> > bridge driver. Time constraints meant that I haven't yet actually
> > formally submitted the input side of this. Whilst there are lots
> > of other things that can use this interface, right now nothing
> > actually does so.
Ok.
> >> - The new exynos_read_s3c64xx_ts() function is intentionally
> >> very similar to the existing exynos_read_raw() functions.
> >> It should probably be changed, either by merging the two
> >> into one, or by simplifying the exynos_read_s3c64xx_ts()
> >> function. This depends a bit on the answers to the questions
> >> above.
> > I'd be tempted to not bother keeping them that similar. It's
> > not a generic IIO channel so simplify it where possible.
Ok.
> >> index 5f95638513d2..cf1d9f3e2492 100644
> >> --- a/drivers/iio/adc/exynos_adc.c
> >> +++ b/drivers/iio/adc/exynos_adc.c
> >> @@ -34,6 +34,7 @@
> >> #include <linux/regulator/consumer.h>
> >> #include <linux/of_platform.h>
> >> #include <linux/err.h>
> >> +#include <linux/input.h>
> > Might want to make the input side optional at compile time...
> > I supose the existing parts are unlikely to be used much in headless
> > devices, but you never know. Maybe we just leave this until someone
> > shouts they want to be able to avoid compiling it in.
I expected the input stuff to just be left out by the compiler
if CONFIG_INPUT is not set. I'll try it out and change it if necessary.
> >> struct completion completion;
> >>
> >> + bool read_ts;
> >> u32 value;
> >> + u32 value2;
> > As I state further down, I'd rather keep a little
> > clear of the naming used in IIO for bits that aren't
> > going through IIO (less confusing!). Maybe just
> > have
> > u32 x, y;
Ok.
> >> unsigned int version;
> >> };
> >>
> >> @@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> >> return ret;
> >> }
> >>
> >> +static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *chan,
> >> + int *val,
> >> + int *val2,
> >> + long mask)
> >> +{
> >> + struct exynos_adc *info = iio_priv(indio_dev);
> >> + unsigned long timeout;
> >> + int ret;
> >> +
> >> + if (mask != IIO_CHAN_INFO_RAW)
> >> + return -EINVAL;
> >> +
> >> + mutex_lock(&indio_dev->mlock);
> >> + info->read_ts = 1;
> Since info->read_ts is of type bool, use true/false.
Ok
> >> +
> >> + reinit_completion(&info->completion);
> >> +
> >> + writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
> >> + ADC_V1_TSC(info->regs));
> >> +
> >> + /* Select the ts channel to be used and Trigger conversion */
> >> + info->data->start_conv(info, 0);
> > 0 is a rather magic value. A define perhaps?
I'm not entirely sure about why we pass 0 here, it's either channel zero
being used for touchscreen, or the channel number being ignore after
we write to the TSC register above. I copied it from the original driver,
but it might be helpful if someone with access to the specs could comment
here.
I'll add a macro for now.
> >> +
> >> + timeout = wait_for_completion_timeout
> >> + (&info->completion, EXYNOS_ADC_TIMEOUT);
> >> + if (timeout == 0) {
> >> + dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> >> + if (info->data->init_hw)
> >> + info->data->init_hw(info);
> >> + ret = -ETIMEDOUT;
> >> + } else {
> >> + *val = info->value;
> >> + *val2 = info->value2;
> > This is definitely abuse as those two values are not intended for
> > different values. If you want to do this please use different naming
> > and don't try to fiddle it into the IIO read raw framework.
> > As you've suggested above, better to simplify this code and drop the
> > bits cloned from the other handler.
Ok, adding ts_x and ts_y members.
> >> + ret = IIO_VAL_INT;
> >> + }
> >> +
> >> + info->read_ts = 0;
> >> + mutex_unlock(&indio_dev->mlock);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >> {
> >> struct exynos_adc *info = (struct exynos_adc *)dev_id;
> >>
> >> /* Read value */
> >> - info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >> + if (info->read_ts) {
> >> + info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >> + info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> > ADC_DATY_MASK would be more obviously correct.
Agreed. I thought about it, but then kept it as it was in the original
driver. Will change now.
> >> + writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> > Perhaps the above is cryptic enough to warrant a comment?
This is also taken directly from the old driver, I have no idea what
it really does...
> >> + /* data from s3c2410_ts driver */
> >> + info->input->name = "S3C24xx TouchScreen";
> >> + info->input->id.bustype = BUS_HOST;
> >> + info->input->id.vendor = 0xDEAD;
> >> + info->input->id.product = 0xBEEF;
> >> + info->input->id.version = 0x0200;
> >> +
> >> + ret = input_register_device(info->input);
> >> + if (ret) {
> >> + input_free_device(info->input);
> >> + goto err;
> Just return ret, without goto (and get rid of label err).
ok
> >> + }
> >> +
> >> + if (info->tsirq > 0)
> >> + ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
> >> + 0, "touchscreen", info);
> > info->tsirq
> > (that had me really confused for a moment ;)
> > Also, perhaps a more specific name. touchscreen_updown or similar as the
> > main interrupt is also used during touchscreen operation.
> >> + if (ret < 0) {
> >> + dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
> >> + info->irq);
> >> + goto err_input;
> >> + }
> >> +
> >> + return 0;
> >> +
> Probably better to get rid of the labels and move the code up, as it is only used once.
Ok. I try not to mix goto and early return, so I've removed all the labels
here.
> >> static int exynos_adc_probe(struct platform_device *pdev)
> >> {
> >> struct exynos_adc *info = NULL;
> >> struct device_node *np = pdev->dev.of_node;
> >> struct iio_dev *indio_dev = NULL;
> >> struct resource *mem;
> >> + bool has_ts;
> >> int ret = -ENODEV;
> >> int irq;
> >>
> >> @@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >> dev_err(&pdev->dev, "no irq resource?\n");
> >> return irq;
> >> }
> >> -
> >> info->irq = irq;
> >> +
> >> + irq = platform_get_irq(pdev, 1);
> >> + if (irq == -EPROBE_DEFER)
> >> + return irq;
> What about other possible error codes?
We handle them later, in the request_threaded_irq call. In particular,
if the touchscreen is not used because either the "has-touchscreen" flag
is not set or because the input layer is not available, failing to get
the irq line should not be treated as an error.
Checking -EPROBE_DEFER however makes sense, so we get out of the probe function
early and don't have to undo and later retry everything.
Thanks everybody for the review!
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
