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@xxxxxxxxx>
Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
---
.../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.
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).
+ - 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?
Thanks in advance.
Thanks,
Mark.
[... ...]
Best Regards,
Josh Wu
--
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
