Message ID | 20210723081621.29477-3-billy_tsai@aspeedtech.com |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | Add support for ast2600 ADC | expand |
On Fri, 23 Jul 2021 16:16:15 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > This patch add more description about aspeed adc and add two property > for ast2600: > - vref: used to configure reference voltage. > - battery-sensing: used to enable battery sensing mode for last channel. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> Hi Billy, A few comments inline. Thanks, Jonathan > --- > .../bindings/iio/adc/aspeed,adc.yaml | 28 +++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml > index 23f3da1ffca3..a562a7fbc30c 100644 > --- a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml > @@ -10,14 +10,26 @@ maintainers: > - Joel Stanley <joel@jms.id.au> > > description: I think you need a | after description if you want the formatting to be maintained (otherwise it will undo the line breaks). > - This device is a 10-bit converter for 16 voltage channels. All inputs are > - single ended. > + • 10-bits resolution for 16 voltage channels. > + At ast2400/ast2500 the device has only one engine with 16 voltage channels. > + At ast2600 the device split into two individual engine and each contains 8 voltage channels. Please wrap lines at 80 chars unless it badly hurts readability. engines > + • Channel scanning can be non-continuous. > + • Programmable ADC clock frequency. > + • Programmable upper and lower bound for each channels. I would use threshold rather than bound. A bound restricts the value, and I think this is measuring it? > + • Interrupt when larger or less than bounds for each channels. > + • Support hysteresis for each channels. > + • Buildin a compensating method. Built-in > + Additional feature at ast2600 of ast2600 > + • Internal or External reference voltage. > + • Support 2 Internal reference voltage 1.2v or 2.5v. > + • Integrate dividing circuit for battery sensing. > > properties: > compatible: > enum: > - aspeed,ast2400-adc > - aspeed,ast2500-adc > + - aspeed,ast2600-adc > > reg: > maxItems: 1 > @@ -33,6 +45,18 @@ properties: > "#io-channel-cells": > const: 1 > > + vref: > + minItems: 900 > + maxItems: 2700 > + default: 2500 > + description: > + ADC Reference voltage in millivolts. I'm not clear from this description. Is this describing an externally connected voltage reference? If so it needs to be done as a regulator. If it's a classic high precision reference, the dts can just use a fixed regulator. > + > + battery-sensing: > + type: boolean > + description: > + Inform the driver that last channel will be used to sensor battery. This isn't (I think?) a standard dt binding, so it needs a manufacturer prefix. aspeed,battery-sensing > + > required: > - compatible > - reg
Hi Jonathan, Thanks for your review. I will fix them. About the vref I reply inline. On 2021/7/23, 10:52 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote: On Fri, 23 Jul 2021 16:16:15 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > > + • Internal or External reference voltage. > > + • Support 2 Internal reference voltage 1.2v or 2.5v. > > + • Integrate dividing circuit for battery sensing. > > > > properties: > > compatible: > > enum: > > - aspeed,ast2400-adc > > - aspeed,ast2500-adc > > + - aspeed,ast2600-adc > > > > reg: > > maxItems: 1 > > @@ -33,6 +45,18 @@ properties: > > "#io-channel-cells": > > const: 1 > > > > + vref: > > + minItems: 900 > > + maxItems: 2700 > > + default: 2500 > > + description: > > + ADC Reference voltage in millivolts. > I'm not clear from this description. Is this describing an externally > connected voltage reference? If so it needs to be done as a regulator. > If it's a classic high precision reference, the dts can just use > a fixed regulator. In the ast2600, the ADC supports two internal reference voltages of 1.2v or 2.5v, as well as external voltages. When the user selects a voltage of 1.2v or 2.5v, my driver will first select to use the internal voltage. As you mention at patch #4, you suggest to use two property to handle this feature. vref: indicate the regulator handler. Like other dt-bindings used. aspeed,int_vref_mv: indicate the chosen of 1.2v or 2.5v and use "model_data->vref_fixed" to exclude ast2400 and ast2500 Is it right? Thanks > > + > > + battery-sensing: > > + type: boolean > > + description: > > + Inform the driver that last channel will be used to sensor battery. > This isn't (I think?) a standard dt binding, so it needs a manufacturer > prefix. > aspeed,battery-sensing Best Regards, Billy Tsai
On Mon, 26 Jul 2021 07:21:29 +0000 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > Hi Jonathan, > > Thanks for your review. I will fix them. > About the vref I reply inline. > > On 2021/7/23, 10:52 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote: > > On Fri, 23 Jul 2021 16:16:15 +0800 > Billy Tsai <billy_tsai@aspeedtech.com> wrote: > > > > + • Internal or External reference voltage. > > > + • Support 2 Internal reference voltage 1.2v or 2.5v. > > > + • Integrate dividing circuit for battery sensing. > > > > > > properties: > > > compatible: > > > enum: > > > - aspeed,ast2400-adc > > > - aspeed,ast2500-adc > > > + - aspeed,ast2600-adc > > > > > > reg: > > > maxItems: 1 > > > @@ -33,6 +45,18 @@ properties: > > > "#io-channel-cells": > > > const: 1 > > > > > > + vref: > > > + minItems: 900 > > > + maxItems: 2700 > > > + default: 2500 > > > + description: > > > + ADC Reference voltage in millivolts. > > > I'm not clear from this description. Is this describing an externally > > connected voltage reference? If so it needs to be done as a regulator. > > If it's a classic high precision reference, the dts can just use > > a fixed regulator. > > In the ast2600, the ADC supports two internal reference voltages of 1.2v or 2.5v, > as well as external voltages. When the user selects a voltage of 1.2v or 2.5v, my > driver will first select to use the internal voltage. Understood. > As you mention at patch #4, you suggest to use two property to handle this feature. > vref: indicate the regulator handler. Like other dt-bindings used. vref-supply would be the binding but otherwise yes. > aspeed,int_vref_mv: indicate the chosen of 1.2v or 2.5v > and use "model_data->vref_fixed" to exclude ast2400 and ast2500 > Is it right? Ideally you would check in the driver and also use something here to allow device trees to be checked automatically. Something like allOf: - if: not: properties: compatible: contains: const: aspeed,ast2600-adc then: int_vref_mv: false that will ensure the property is only used for the ast2600. Note the above is from memory, so may well have incorrect syntax. I've never found yaml intuitive! Thanks, Jonathan > > Thanks > > > > + > > > + battery-sensing: > > > + type: boolean > > > + description: > > > + Inform the driver that last channel will be used to sensor battery. > > > This isn't (I think?) a standard dt binding, so it needs a manufacturer > > prefix. > > > aspeed,battery-sensing > > Best Regards, > Billy Tsai > >
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml index 23f3da1ffca3..a562a7fbc30c 100644 --- a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml +++ b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml @@ -10,14 +10,26 @@ maintainers: - Joel Stanley <joel@jms.id.au> description: - This device is a 10-bit converter for 16 voltage channels. All inputs are - single ended. + • 10-bits resolution for 16 voltage channels. + At ast2400/ast2500 the device has only one engine with 16 voltage channels. + At ast2600 the device split into two individual engine and each contains 8 voltage channels. + • Channel scanning can be non-continuous. + • Programmable ADC clock frequency. + • Programmable upper and lower bound for each channels. + • Interrupt when larger or less than bounds for each channels. + • Support hysteresis for each channels. + • Buildin a compensating method. + Additional feature at ast2600 + • Internal or External reference voltage. + • Support 2 Internal reference voltage 1.2v or 2.5v. + • Integrate dividing circuit for battery sensing. properties: compatible: enum: - aspeed,ast2400-adc - aspeed,ast2500-adc + - aspeed,ast2600-adc reg: maxItems: 1 @@ -33,6 +45,18 @@ properties: "#io-channel-cells": const: 1 + vref: + minItems: 900 + maxItems: 2700 + default: 2500 + description: + ADC Reference voltage in millivolts. + + battery-sensing: + type: boolean + description: + Inform the driver that last channel will be used to sensor battery. + required: - compatible - reg
This patch add more description about aspeed adc and add two property for ast2600: - vref: used to configure reference voltage. - battery-sensing: used to enable battery sensing mode for last channel. Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> --- .../bindings/iio/adc/aspeed,adc.yaml | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)