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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html