| Message ID | 20181106164116.6299-2-dev@pschenker.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] iio: adc: add STMPE ADC driver using IIO framework | expand |
On Tue, 6 Nov 2018 17:41:15 +0100 Philippe Schenker <dev@pschenker.ch> wrote: > From: Stefan Agner <stefan@agner.ch> > > This adds the devicetree bindings for the STMPE ADC. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > --- > .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++ > 1 file changed, 34 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > new file mode 100644 > index 000000000000..752ef35a794d > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > @@ -0,0 +1,34 @@ > +STMPE ADC driver > +---------------- > + > +Required properties: > + - compatible: "st,stmpe-adc" > + > +Optional properties: > +Note that the ADC is shared with the STMPE touchscreen, so if using both the > +settings should be the same. Ah, guessing there is already a binding in place for that which we need to closely match. Should there be one overarching device with both as children though if it is the same hardware? > +If the settings are not the same, the settings of the driver initialized last > +will be active. > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks, > + 1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, > + 5 -> 96 clocks, 6 -> 144 clocks), recommended is 4. Could of questions here. Is this a hardware dictated limit or is it dependent on application? Basically I'm asking if it should be in userspace rather than here. Ideally I'd love to see it in a unit of time, but failing that can we have it in clock cycles please rather than an enum? Always nice if a binding is human readable. st,sample-clock-cycles would be a better name. > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC) > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external > + reference) If there is an external reference, I'd expect to see a regulator providing it. Usually the mere presence of such an optional regulator does the job of saying if it should be used. > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz) So this is some internal clock? What dictates the right setting? I.e. how should anyone know which one to pick? > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be > + requestable due to different usage (e.g. touch) That's a little bit ugly. Would prefer to see an explicit set of channels brought out as children in DT. There are some patches around standardising some properties for doing that under review at the moment. [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate file > + > +Node name must be stmpe_adc and should be child node of stmpe node to > +which it belongs. > + > +Example: > + > + stmpe_adc { adc { is the moderately standard naming for ADCS. > + compatible = "st,stmpe-adc"; > + st,sample-time = <4>; > + st,mod-12b = <1>; > + st,ref-sel = <0>; > + st,adc-freq = <1>; > + st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */ > + };
On Sun, 11 Nov 2018 15:42:07 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 6 Nov 2018 17:41:15 +0100 > Philippe Schenker <dev@pschenker.ch> wrote: > > > From: Stefan Agner <stefan@agner.ch> > > > > This adds the devicetree bindings for the STMPE ADC. > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > --- > > .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > new file mode 100644 > > index 000000000000..752ef35a794d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > @@ -0,0 +1,34 @@ > > +STMPE ADC driver > > +---------------- > > + > > +Required properties: > > + - compatible: "st,stmpe-adc" > > + > > +Optional properties: > > +Note that the ADC is shared with the STMPE touchscreen, so if using both the > > +settings should be the same. > Ah, guessing there is already a binding in place for that which we need to closely > match. Should there be one overarching device with both as children though if > it is the same hardware? ah, there is an mfd, can these become elements of the binding for that so they are only in one place? It's a bit ugly to do that given there are other children, but certainly better than having them replicated like this... > > > +If the settings are not the same, the settings of the driver initialized last > > +will be active. > > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks, > > + 1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, > > + 5 -> 96 clocks, 6 -> 144 clocks), recommended is 4. > Could of questions here. Is this a hardware dictated limit or is it dependent on > application? Basically I'm asking if it should be in userspace rather than here. > > Ideally I'd love to see it in a unit of time, but failing that can we have it > in clock cycles please rather than an enum? Always nice if a binding is > human readable. st,sample-clock-cycles would be a better name. > > > > > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC) > > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external > > + reference) > > If there is an external reference, I'd expect to see a regulator providing it. > Usually the mere presence of such an optional regulator does the job of saying if > it should be used. > > > > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz) > So this is some internal clock? What dictates the right setting? I.e. how > should anyone know which one to pick? > > > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be > > + requestable due to different usage (e.g. touch) > That's a little bit ugly. Would prefer to see an explicit set of channels > brought out as children in DT. There are some patches around standardising > some properties for doing that under review at the moment. > > [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate file > > > + > > +Node name must be stmpe_adc and should be child node of stmpe node to > > +which it belongs. > > + > > +Example: > > + > > + stmpe_adc { > > adc { > is the moderately standard naming for ADCS. > > > + compatible = "st,stmpe-adc"; > > + st,sample-time = <4>; > > + st,mod-12b = <1>; > > + st,ref-sel = <0>; > > + st,adc-freq = <1>; > > + st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */ > > + }; >
On Sun, 2018-11-11 at 15:42 +0000, Jonathan Cameron wrote: > On Tue, 6 Nov 2018 17:41:15 +0100 > Philippe Schenker <dev@pschenker.ch> wrote: > > > From: Stefan Agner <stefan@agner.ch> > > > > This adds the devicetree bindings for the STMPE ADC. > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > --- > > .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > new file mode 100644 > > index 000000000000..752ef35a794d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > @@ -0,0 +1,34 @@ > > +STMPE ADC driver > > +---------------- > > + > > +Required properties: > > + - compatible: "st,stmpe-adc" > > + > > +Optional properties: > > +Note that the ADC is shared with the STMPE touchscreen, so if using both > > the > > +settings should be the same. > Ah, guessing there is already a binding in place for that which we need to > closely > match. Should there be one overarching device with both as children though if > it is the same hardware? I moved ADC settings to the mfd... > > > +If the settings are not the same, the settings of the driver initialized > > last > > +will be active. > > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks, > > + 1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, > > + 5 -> 96 clocks, 6 -> 144 clocks), recommended is 4. > Could of questions here. Is this a hardware dictated limit or is it dependent > on > application? Basically I'm asking if it should be in userspace rather than > here. > > Ideally I'd love to see it in a unit of time, but failing that can we have it > in clock cycles please rather than an enum? Always nice if a binding is > human readable. st,sample-clock-cycles would be a better name. > This is none of our design-decision. We did it the same way as it was already in drivers/input/touchscreen/stmpe-ts.c. > > > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC) > > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external > > + reference) > > If there is an external reference, I'd expect to see a regulator providing it. > Usually the mere presence of such an optional regulator does the job of saying > if > it should be used. Same here, we did it the same way as it was in stmpe-ts.c > > > > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> > > 6.5 MHz) > So this is some internal clock? What dictates the right setting? I.e. how > should anyone know which one to pick? I think as a user of this driver at this point you need to read the datasheet anyway, to be able to decide which parameters you need for the application. > > > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be > > + requestable due to different usage (e.g. touch) > That's a little bit ugly. Would prefer to see an explicit set of channels > brought out as children in DT. There are some patches around standardising > some properties for doing that under review at the moment. > > [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate > file This is also implemented with the "norequest-mask" in stmpe-ts.c. That's why we did it the same way and I don't like to break others devicetree by changing stmpe-ts.c > > > + > > +Node name must be stmpe_adc and should be child node of stmpe node to > > +which it belongs. > > + > > +Example: > > + > > + stmpe_adc { > > adc { > is the moderately standard naming for ADCS. This is none of our naming decisions please see line 1311 in drivers/mfd/stmpe.c > > > + compatible = "st,stmpe-adc"; > > + st,sample-time = <4>; > > + st,mod-12b = <1>; > > + st,ref-sel = <0>; > > + st,adc-freq = <1>; > > + st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */ > > + };
On Thu, 15 Nov 2018 10:38:03 +0100 Philippe Schenker <dev@pschenker.ch> wrote: > On Sun, 2018-11-11 at 15:42 +0000, Jonathan Cameron wrote: > > On Tue, 6 Nov 2018 17:41:15 +0100 > > Philippe Schenker <dev@pschenker.ch> wrote: > > > > > From: Stefan Agner <stefan@agner.ch> > > > > > > This adds the devicetree bindings for the STMPE ADC. > > > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > --- > > > .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > > b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > > new file mode 100644 > > > index 000000000000..752ef35a794d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt > > > @@ -0,0 +1,34 @@ > > > +STMPE ADC driver > > > +---------------- > > > + > > > +Required properties: > > > + - compatible: "st,stmpe-adc" > > > + > > > +Optional properties: > > > +Note that the ADC is shared with the STMPE touchscreen, so if using both > > > the > > > +settings should be the same. > > Ah, guessing there is already a binding in place for that which we need to > > closely > > match. Should there be one overarching device with both as children though if > > it is the same hardware? > > I moved ADC settings to the mfd... > > > > > > +If the settings are not the same, the settings of the driver initialized > > > last > > > +will be active. > > > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks, > > > + 1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, > > > + 5 -> 96 clocks, 6 -> 144 clocks), recommended is 4. > > Could of questions here. Is this a hardware dictated limit or is it dependent > > on > > application? Basically I'm asking if it should be in userspace rather than > > here. > > > > Ideally I'd love to see it in a unit of time, but failing that can we have it > > in clock cycles please rather than an enum? Always nice if a binding is > > human readable. st,sample-clock-cycles would be a better name. > > > > This is none of our design-decision. We did it the same way as it was already in > drivers/input/touchscreen/stmpe-ts.c. > > > > > > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC) > > > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external > > > + reference) > > > > If there is an external reference, I'd expect to see a regulator providing it. > > Usually the mere presence of such an optional regulator does the job of saying > > if > > it should be used. > > Same here, we did it the same way as it was in stmpe-ts.c > > > > > > > > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> > > > 6.5 MHz) > > So this is some internal clock? What dictates the right setting? I.e. how > > should anyone know which one to pick? > > I think as a user of this driver at this point you need to read the datasheet > anyway, to be able to decide which parameters you need for the application. > > > > > > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be > > > + requestable due to different usage (e.g. touch) > > That's a little bit ugly. Would prefer to see an explicit set of channels > > brought out as children in DT. There are some patches around standardising > > some properties for doing that under review at the moment. > > > > [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate > > file > > This is also implemented with the "norequest-mask" in stmpe-ts.c. That's why we > did it the same way and I don't like to break others devicetree by changing > stmpe-ts.c > > > > > > + > > > +Node name must be stmpe_adc and should be child node of stmpe node to > > > +which it belongs. > > > + > > > +Example: > > > + > > > + stmpe_adc { > > > > adc { > > is the moderately standard naming for ADCS. > > This is none of our naming decisions please see line 1311 in drivers/mfd/stmpe.c Fair enough on all these - we are stuck with less than ideal legacy :( Jonathan > > > > > > + compatible = "st,stmpe-adc"; > > > + st,sample-time = <4>; > > > + st,mod-12b = <1>; > > > + st,ref-sel = <0>; > > > + st,adc-freq = <1>; > > > + st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */ > > > + }; >
diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt new file mode 100644 index 000000000000..752ef35a794d --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt @@ -0,0 +1,34 @@ +STMPE ADC driver +---------------- + +Required properties: + - compatible: "st,stmpe-adc" + +Optional properties: +Note that the ADC is shared with the STMPE touchscreen, so if using both the +settings should be the same. +If the settings are not the same, the settings of the driver initialized last +will be active. +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks, + 1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, + 5 -> 96 clocks, 6 -> 144 clocks), recommended is 4. +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC) +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external + reference) +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz) +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be + requestable due to different usage (e.g. touch) + +Node name must be stmpe_adc and should be child node of stmpe node to +which it belongs. + +Example: + + stmpe_adc { + compatible = "st,stmpe-adc"; + st,sample-time = <4>; + st,mod-12b = <1>; + st,ref-sel = <0>; + st,adc-freq = <1>; + st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */ + };