[PATCH 5/5] iio: at91: introduce touch screen support in iio adc driver

[Mark Rutland]

On Tue, Aug 06, 2013 at 11:24:14AM +0100, Josh Wu wrote:
> Dear Mark
> 
> Thanks for the detailed comment. Check mine in below:
> 
> On 7/26/2013 12:45 AM, Mark Rutland wrote:
> > On Thu, Jul 25, 2013 at 08:56:38AM +0100, Josh Wu wrote:
> >> Hi, Dear Mark
> >>
> >> On 7/22/2013 9:17 PM, Mark Rutland wrote:
> >>> On Sun, Jul 14, 2013 at 09:04:29AM +0100, Josh Wu wrote:
> >>>> AT91 ADC hardware integrate touch screen support. So this patch add touch
> >>>> screen support for at91 adc iio driver.
> >>>> To enable touch screen support in adc, you need to add the dt parameters:
> >>>>     which type of touch are used? (4 or 5 wires), sample period time,
> >>>>     pen detect debounce time, average samples and pen detect resistor.
> >>>>
> >>>> In the meantime, since touch screen will use a interal period trigger of adc,
> >>>> so it is conflict to other hardware triggers. Driver will disable the hardware
> >>>> trigger support if touch screen is enabled.
> >>>>
> >>>> This driver has been tested in AT91SAM9X5-EK and SAMA5D3x-EK.
> >>>>
> >>>> Signed-off-by: Josh Wu <josh.wu at atmel.com>
> >>>> Cc: Dmitry Torokhov <dmitry.torokhov at gmail.com>
> >>>> ---
> >>>>    .../devicetree/bindings/arm/atmel-adc.txt          |   13 +
> >>>>    arch/arm/mach-at91/include/mach/at91_adc.h         |   34 ++
> >>>>    drivers/iio/adc/at91_adc.c                         |  389 ++++++++++++++++++--
> >>>>    3 files changed, 412 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> >>>> index 0db2945..925d656 100644
> >>>> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
> >>>> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> >>>> @@ -29,6 +29,19 @@ Optional properties:
> >>>>      - atmel,adc-sample-hold-time: Sample and Hold Time in microseconds
> >>>>      - atmel,adc-clock-rate: ADC clock rate. If not specified, use the default
> >>>>                             adc_op_clk.
> >>>> +  - atmel,adc-touchscreen-wires: Number of touch screen wires. Only support
> >>>> +                                4 and 5 wires touch screen.
> >>> Are 4 and 5 wire configurations that can exist, or the only ones
> >>> supported by the driver?
> >> It can be set:
> >>
> >> atmel,adc-touchscreen-wires = <4>;
> >> or
> >> atmel,adc-touchscreen-wires = <5>;
> >>
> >> according to your touch screen.
> > That doesn't answer my question.
> >
> > Is it possible that 3 or 6 wire configurations might exist, for example,
> > even if not supported by this driver? Or does the design of the adc
> > prevent this?
> 
> No, only 4-wire or 5-wire touch screen support in market so far.

Ok. The point that I was trying to get at was that "Only support 4 and 5
wires touch screen" seems to be a description of the driver rather than
a physical limitation that should limit what we can describe in dt.

> 
> >
> > Is there any documentation that might make this clearer?
> >
> >>>> +    NOTE: when adc touch screen enabled, the adc hardware trigger will be
> >>>> +          disabled. Since touch screen will occupied the trigger register.
> >>>> +  - atmel,adc-ts-pendet-debounce: Debounce time in microsecond for touch pen
> >>>> +                                 detect.
> >>> For consistency with the adc-touchscreen-wires property, and  other
> >>> properties with timing information, how about
> >>> atmel,adc-touchscreen-debounce-delay-us ?
> >> sound nice to me.
> > Additionally, is this likely to vary from board to board? This feels
> > like configuration that could be done based on the compatible string...
> >
> >>>> +  - atmel,adc-ts-sample-period-time: Sample Period Time in microsecond for
> >>>> +                                    touch screen
> >>> Again, please be consistent with ts or touchscreen. It may also be good
> >>> to have -us to make the units explicit (though it does leave the
> >>> property name being quite a mothful):
> >>>
> >>> atmel,adc-touchscreen-sample-period-us ?
> >> nice. I will use this one.
> > Looking again at the driver code, it looks like a value derived from
> > this eventually gets written to the hardware. Is this a fixed value at
> > integration time, or is this a configuration value? If it's the latter,
> > can the driver not derive a good value for this itself?
> 
> After a further thinking, I think it is better to remove this sample 
> period us & pen detect debouce us.
> Since it is a fixed value and no need to change it so far (in 
> at91sam9x5ek, sama5d3xek).

Ok.

> 
> >
> >>>> +  - atmel,adc-ts-filter-average: Numbers of sampling data will be averaged.
> >>>> +    0 means no average. 1 means average two samples. 2 means average four
> >>>> +    samples. 3 means average eight samples.
> >>> Is this averaging done in the hardware, or the kernel driver?
> >> It is done in the hardware.
> > Similarly, this seems to eventually get written to hardware, and thus
> > seems more like configuration than hardware description. Why does this
> > need to be in the DT?
> >
> > As an aside, in general it's nicer to describe a property as a logical
> > value rather than the raw value that gets programmed into hardware (e.g.
> > this property could by 2, 4, or 8 rather than 1, 2, or 4).
> >
> >> But for some soc, like AT91SAM9G45, hardware doesn't support hardware
> >> average.
> >> So I am wondering use it as for both hardware and softer average.
> >>
> >> BTW, you mentioned the kernel driver, do you mean a filter algorithm is
> >> already implemented in kernel library?
> > I do not know of any such filter algorithm in the kernel, though there
> > may be one. I was simply confused as to what this was used for.
> >
> >>> If it's the latter, this can be left for the kernel to decide.
> >>>
> >>>> +  - atmel,adc-ts-pendet-sensitivity: Pen Detection input pull-up resistor.
> >>>> +    It can be 0, 1, 2, 3.
> >>> I think "pendet" is a bit opaque. "pen-detect" may be better.
> >> yes. I'll change this.
> >>
> >>> What
> >>> physical property does this represent (are these discrete values, or an
> >>> enumeration)?
> >> This property is supported by hardware, it can change the adc internal
> >> resistor value for better pen detection,
> >>        * 0 = 200 kOhm, 1 = 150 kOhm, 2 = 100 kOhm, 3 = 50 kOhm
> >> In general, we just use default value 2 for 100kOhm.
> > This value eventually gets written to hardware, and seems more like
> > configuration than hardware description. Why does this need to be in the
> > DT?
> 
> I am a little bit confused by the hardware configuration and hardware 
> description.
> It seems I prefer to put all the hardware configurable value to DT. 
> Since I think this can be changed in DT.
> But as you mentioned above, only the hardware description can be in DT.
> Could you give me a more detail about the difference between hardware 
> configuration and hardware description?

It's admittedly a gray area, but the basic idea would be that anything
we might feasibly want to change the value of later shouldn't go into
the DT, while properties of the hardware that might influence those
decisions should be in the DT. It seems to me that the pen detect
resistor value is something we might want to change later, if people
found a particular touchscreen were too sensitive, for instance.
However, maybe not.

Thanks,
Mark.