| Message ID | 20230119212632.185881-2-marijn.suijten@somainline.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arm64: dts: qcom: Use labels with generic node names for ADC channels | expand |
On Thu, 19 Jan 2023 22:26:31 +0100, Marijn Suijten wrote: > As discussed in [1] it is more convenient to use a generic adc-chan node > name for ADC channels while storing a friendly - board-specific instead > of PMIC-specific - name in the label, if/when desired to overwrite the > channel description already contained (but previously unused) in the > driver [2]. > > Replace the .* name pattern with the adc-chan literal, but leave the > label property optional for bindings to choose to fall back a channel > label hardcoded in the driver [2] instead. > > [1]: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u > [2]: https://lore.kernel.org/linux-arm-msm/20230116220909.196926-4-marijn.suijten@somainline.org/ > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > .../bindings/iio/adc/qcom,spmi-vadc.yaml | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.example.dtb: adc@3100: 'conn-therm@4f' does not match any of the regexes: '^adc-chan@[0-9a-f]+$', 'pinctrl-[0-9]+' From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.example.dtb: adc@3100: 'conn-therm@147', 'xo-therm@44' do not match any of the regexes: '^adc-chan@[0-9a-f]+$', 'pinctrl-[0-9]+' From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230119212632.185881-2-marijn.suijten@somainline.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Thu, 19 Jan 2023 22:26:31 +0100 Marijn Suijten <marijn.suijten@somainline.org> wrote: > As discussed in [1] it is more convenient to use a generic adc-chan node > name for ADC channels while storing a friendly - board-specific instead > of PMIC-specific - name in the label, if/when desired to overwrite the > channel description already contained (but previously unused) in the > driver [2]. > > Replace the .* name pattern with the adc-chan literal, but leave the > label property optional for bindings to choose to fall back a channel > label hardcoded in the driver [2] instead. > > [1]: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u > [2]: https://lore.kernel.org/linux-arm-msm/20230116220909.196926-4-marijn.suijten@somainline.org/ > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> Other than the use in the tm5 thermal example that Rob's bot found, this looks good to me. I think ideal would be to fix that in a precursor patch then do this one. Note that the existing two patches should be in the other order 1. Update the dtsi 2. Tighten the bounds to check they are right. Doesn't matter much though as the two patches will probably go through different trees. Thanks, Jonathan > --- > .../bindings/iio/adc/qcom,spmi-vadc.yaml | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > index bd6e0d6f6e0c..9b1a60fe7599 100644 > --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > @@ -54,7 +54,7 @@ required: > - '#io-channel-cells' > > patternProperties: > - "^.*@[0-9a-f]+$": > + "^adc-chan@[0-9a-f]+$": > type: object > additionalProperties: false > description: | > @@ -148,7 +148,7 @@ allOf: > > then: > patternProperties: > - "^.*@[0-9a-f]+$": > + "^adc-chan@[0-9a-f]+$": > properties: > qcom,decimation: > enum: [ 512, 1024, 2048, 4096 ] > @@ -171,7 +171,7 @@ allOf: > > then: > patternProperties: > - "^.*@[0-9a-f]+$": > + "^adc-chan@[0-9a-f]+$": > properties: > qcom,decimation: > enum: [ 256, 512, 1024 ] > @@ -194,7 +194,7 @@ allOf: > > then: > patternProperties: > - "^.*@[0-9a-f]+$": > + "^adc-chan@[0-9a-f]+$": > properties: > qcom,decimation: > enum: [ 250, 420, 840 ] > @@ -217,7 +217,7 @@ allOf: > > then: > patternProperties: > - "^.*@[0-9a-f]+$": > + "^adc-chan@[0-9a-f]+$": > properties: > qcom,decimation: > enum: [ 85, 340, 1360 ] > @@ -292,16 +292,18 @@ examples: > #io-channel-cells = <1>; > > /* Other properties are omitted */ > - xo-therm@44 { > + adc-chan@44 { > reg = <PMK8350_ADC7_AMUX_THM1_100K_PU>; > qcom,ratiometric; > qcom,hw-settle-time = <200>; > + label = "xo_therm"; > }; > > - conn-therm@47 { > + adc-chan@47 { > reg = <PM8350_ADC7_AMUX_THM4_100K_PU(1)>; > qcom,ratiometric; > qcom,hw-settle-time = <200>; > + label = "conn_therm"; > }; > }; > };
On 2023-01-21 17:08:25, Jonathan Cameron wrote: > On Thu, 19 Jan 2023 22:26:31 +0100 > Marijn Suijten <marijn.suijten@somainline.org> wrote: > > > As discussed in [1] it is more convenient to use a generic adc-chan node > > name for ADC channels while storing a friendly - board-specific instead > > of PMIC-specific - name in the label, if/when desired to overwrite the > > channel description already contained (but previously unused) in the > > driver [2]. > > > > Replace the .* name pattern with the adc-chan literal, but leave the > > label property optional for bindings to choose to fall back a channel > > label hardcoded in the driver [2] instead. > > > > [1]: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u > > [2]: https://lore.kernel.org/linux-arm-msm/20230116220909.196926-4-marijn.suijten@somainline.org/ > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > Other than the use in the tm5 thermal example that Rob's bot found, this looks > good to me. Yep, shouldn't have ran dt_binding_check and dtbs_check with DT_SCHEMA_FILES=just/the/one/edited/here. > I think ideal would be to fix that in a precursor patch then > do this one. Can't that be part of the current one? At least the change requested by dt-bindings here is backwards-compatible; the adc-chan@xx format with optional label property was already allowed. > Note that the existing two patches should be in the other order > 1. Update the dtsi > 2. Tighten the bounds to check they are right. Hmm, I'm never sure what goes first: drivers, bindings, or DT (considering there's an ABI it shouldn't matter whether drivers or DT go first, leaving just dt-bindings which could be used to TDD the DT... or check adjustment after the fact). Is this relationship - and the order following from it - documented somewhere? > Doesn't matter much though as the two patches will probably go through > different trees. Should be right, indeed. - Marijn
On Mon, 23 Jan 2023 00:37:41 +0100 Marijn Suijten <marijn.suijten@somainline.org> wrote: > On 2023-01-21 17:08:25, Jonathan Cameron wrote: > > On Thu, 19 Jan 2023 22:26:31 +0100 > > Marijn Suijten <marijn.suijten@somainline.org> wrote: > > > > > As discussed in [1] it is more convenient to use a generic adc-chan node > > > name for ADC channels while storing a friendly - board-specific instead > > > of PMIC-specific - name in the label, if/when desired to overwrite the > > > channel description already contained (but previously unused) in the > > > driver [2]. > > > > > > Replace the .* name pattern with the adc-chan literal, but leave the > > > label property optional for bindings to choose to fall back a channel > > > label hardcoded in the driver [2] instead. > > > > > > [1]: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u > > > [2]: https://lore.kernel.org/linux-arm-msm/20230116220909.196926-4-marijn.suijten@somainline.org/ > > > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > Other than the use in the tm5 thermal example that Rob's bot found, this looks > > good to me. > > Yep, shouldn't have ran dt_binding_check and dtbs_check with > DT_SCHEMA_FILES=just/the/one/edited/here. > > > I think ideal would be to fix that in a precursor patch then > > do this one. > > Can't that be part of the current one? At least the change requested by > dt-bindings here is backwards-compatible; the adc-chan@xx format with > optional label property was already allowed. Sure you can merge it in, or do it as a precursor. I'd split it though purely as it can be picked up by a different maintainer if that makes sense (at cost of some errors as things filter through the various trees). > > > Note that the existing two patches should be in the other order > > 1. Update the dtsi > > 2. Tighten the bounds to check they are right. > > Hmm, I'm never sure what goes first: drivers, bindings, or DT > (considering there's an ABI it shouldn't matter whether drivers or DT > go first, leaving just dt-bindings which could be used to TDD the DT... > or check adjustment after the fact). Is this relationship - and the > order following from it - documented somewhere? In this particular case we in theory want bisectability. As you note the updated tighter naming is already allowed, so we can make that change first. Normally we are adding new bindings and it doesn't matter on order as we just have an undocumented binding if the driver goes first. As noted it all become irrelevant when things go through different trees anyway! J > > > Doesn't matter much though as the two patches will probably go through > > different trees. > > Should be right, indeed. > > - Marijn
diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml index bd6e0d6f6e0c..9b1a60fe7599 100644 --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml @@ -54,7 +54,7 @@ required: - '#io-channel-cells' patternProperties: - "^.*@[0-9a-f]+$": + "^adc-chan@[0-9a-f]+$": type: object additionalProperties: false description: | @@ -148,7 +148,7 @@ allOf: then: patternProperties: - "^.*@[0-9a-f]+$": + "^adc-chan@[0-9a-f]+$": properties: qcom,decimation: enum: [ 512, 1024, 2048, 4096 ] @@ -171,7 +171,7 @@ allOf: then: patternProperties: - "^.*@[0-9a-f]+$": + "^adc-chan@[0-9a-f]+$": properties: qcom,decimation: enum: [ 256, 512, 1024 ] @@ -194,7 +194,7 @@ allOf: then: patternProperties: - "^.*@[0-9a-f]+$": + "^adc-chan@[0-9a-f]+$": properties: qcom,decimation: enum: [ 250, 420, 840 ] @@ -217,7 +217,7 @@ allOf: then: patternProperties: - "^.*@[0-9a-f]+$": + "^adc-chan@[0-9a-f]+$": properties: qcom,decimation: enum: [ 85, 340, 1360 ] @@ -292,16 +292,18 @@ examples: #io-channel-cells = <1>; /* Other properties are omitted */ - xo-therm@44 { + adc-chan@44 { reg = <PMK8350_ADC7_AMUX_THM1_100K_PU>; qcom,ratiometric; qcom,hw-settle-time = <200>; + label = "xo_therm"; }; - conn-therm@47 { + adc-chan@47 { reg = <PM8350_ADC7_AMUX_THM4_100K_PU(1)>; qcom,ratiometric; qcom,hw-settle-time = <200>; + label = "conn_therm"; }; }; };
As discussed in [1] it is more convenient to use a generic adc-chan node name for ADC channels while storing a friendly - board-specific instead of PMIC-specific - name in the label, if/when desired to overwrite the channel description already contained (but previously unused) in the driver [2]. Replace the .* name pattern with the adc-chan literal, but leave the label property optional for bindings to choose to fall back a channel label hardcoded in the driver [2] instead. [1]: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u [2]: https://lore.kernel.org/linux-arm-msm/20230116220909.196926-4-marijn.suijten@somainline.org/ Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- .../bindings/iio/adc/qcom,spmi-vadc.yaml | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)