| Message ID | 20181004001432.21218-2-mka@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC | expand |
Hi, On Wed, Oct 3, 2018 at 5:14 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > The node has a reg property, therefore its name should include a unit > address. > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > DT conventions. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > Changes in v4: > - patch added to the series > --- > Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > index d0c188e5c922..ff7aa5038ba6 100644 > --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > @@ -112,7 +112,7 @@ Example: > io-channel-ranges; > > /* Channel node */ > - usb_id_nopull { > + usb-id-nopull@VADC_LR_MUX10_USB_ID { > reg = <VADC_LR_MUX10_USB_ID>; This is my understanding of how it should be done and matches some existing examples I see in the tree. AKA "pd_hdcp@RK3399_PD_HDCP". Thanks for cleaning it up! Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > The node has a reg property, therefore its name should include a unit > address. > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > DT conventions. This is ADC channels? If so, then DT convention would really be "adc@...". > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > Changes in v4: > - patch added to the series > --- > Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > index d0c188e5c922..ff7aa5038ba6 100644 > --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > @@ -112,7 +112,7 @@ Example: > io-channel-ranges; > > /* Channel node */ > - usb_id_nopull { > + usb-id-nopull@VADC_LR_MUX10_USB_ID { > reg = <VADC_LR_MUX10_USB_ID>; Defines are generally used when there's not well defined numbering such as clock ids. Interrupts and addresses are generally well defined and don't need defines. > qcom,decimation = <512>; > qcom,ratiometric; > -- > 2.19.0.605.g01d371f741-goog >
Hi Rob, On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > The node has a reg property, therefore its name should include a unit > > address. > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > DT conventions. > > This is ADC channels? yes > If so, then DT convention would really be "adc@...". ok, will update > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > Changes in v4: > > - patch added to the series > > --- > > Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > > index d0c188e5c922..ff7aa5038ba6 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > > @@ -112,7 +112,7 @@ Example: > > io-channel-ranges; > > > > /* Channel node */ > > - usb_id_nopull { > > + usb-id-nopull@VADC_LR_MUX10_USB_ID { > > reg = <VADC_LR_MUX10_USB_ID>; > > Defines are generally used when there's not well defined numbering such > as clock ids. Interrupts and addresses are generally well defined and > don't need defines. ok, I'll update this to use literals Thanks for the review! Matthias
On Thu, Oct 11, 2018 at 12:11:34PM -0700, Matthias Kaehlcke wrote: > Hi Rob, > > On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > > The node has a reg property, therefore its name should include a unit > > > address. > > > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > > DT conventions. > > > > This is ADC channels? > > yes > > > If so, then DT convention would really be "adc@...". > > ok, will update > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > Changes in v4: > > > - patch added to the series > > > --- > > > Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > > > index d0c188e5c922..ff7aa5038ba6 100644 > > > --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > > > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > > > @@ -112,7 +112,7 @@ Example: > > > io-channel-ranges; > > > > > > /* Channel node */ > > > - usb_id_nopull { > > > + usb-id-nopull@VADC_LR_MUX10_USB_ID { > > > reg = <VADC_LR_MUX10_USB_ID>; > > > > Defines are generally used when there's not well defined numbering such > > as clock ids. Interrupts and addresses are generally well defined and > > don't need defines. > > ok, I'll update this to use literals Actually I'm not convinced literals are the right answer here. It makes perfect sense for a standard ADC, with a 1:1 mapping of channel number and physical input. However in case of the VADC/ADC5 there can be multiple channel numbers per input pin, the different values are used to select a certain 'configuration' (eg. ADC5_BAT_THERM, ADC5_BAT_THERM_30K_PU, ADC5_BAT_THERM_100K_PU, ADC5_BAT_THERM_400K_PU). IMO using integer channel numbers here would be more error prone and obfuscating than using the defines.
On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > The node has a reg property, therefore its name should include a unit > > address. > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > DT conventions. > > This is ADC channels? If so, then DT convention would really be > "adc@...". Is it really? A grep for 'adc@' in arch/${ARCH}/boot/dts yields mostly ADC controller not channel nodes. I'm totally fine with changing the name to 'adc@...' if that's the preference/convention, just want to reconfirm since the actual use is a bit ambiguous.
On Fri, Oct 12, 2018 at 10:15:23AM -0700, Matthias Kaehlcke wrote: > On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > > The node has a reg property, therefore its name should include a unit > > > address. > > > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > > DT conventions. > > > > This is ADC channels? If so, then DT convention would really be > > "adc@...". > > Is it really? A grep for 'adc@' in arch/${ARCH}/boot/dts yields > mostly ADC controller not channel nodes. > > I'm totally fine with changing the name to 'adc@...' if that's the > preference/convention, just want to reconfirm since the actual use is > a bit ambiguous. Could we please reach a conclusion on this? Summarizing the options on the table so far are: 1. usb-id-nopull@VADC_LR_MUX10_USB_ID 2. usb-id-nopull@57 3. adc@VADC_LR_MUX10_USB_ID 4. adc@57 My personal preference goes to something <node name>@<define> since the unit address doesn't just resolve to an ADC channel number but also includes configuation information. A literal like '57' conveys less information than the define, it's easier to introduce errors and these errors are harder to spot. If 'adc@...' really was the convention (or should be) I'd be clearly in favor of following it. As mentioned above, in practice the use of the 'adc@...' node name seems to be more prevalent for ADC controllers than channels, so I'm more inclined towards 'usb-id-nopull@...' or similar. All that said, these are just my preferences for the reasons outlined above, if DT maintainers really want it to be 'adc@57' or some variation of that, I'm fine with that too. Please let me know and we can move forward with this trivial series. Thanks Matthias
On Thu, 18 Oct 2018 12:40:10 -0700 Matthias Kaehlcke <mka@chromium.org> wrote: > On Fri, Oct 12, 2018 at 10:15:23AM -0700, Matthias Kaehlcke wrote: > > On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > > > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > > > The node has a reg property, therefore its name should include a unit > > > > address. > > > > > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > > > DT conventions. > > > > > > This is ADC channels? If so, then DT convention would really be > > > "adc@...". > > > > Is it really? A grep for 'adc@' in arch/${ARCH}/boot/dts yields > > mostly ADC controller not channel nodes. > > > > I'm totally fine with changing the name to 'adc@...' if that's the > > preference/convention, just want to reconfirm since the actual use is > > a bit ambiguous. > > Could we please reach a conclusion on this? > > Summarizing the options on the table so far are: > > 1. usb-id-nopull@VADC_LR_MUX10_USB_ID > 2. usb-id-nopull@57 > 3. adc@VADC_LR_MUX10_USB_ID > 4. adc@57 > > My personal preference goes to something <node name>@<define> > since the unit address doesn't just resolve to an ADC channel number > but also includes configuation information. A literal like '57' > conveys less information than the define, it's easier to introduce > errors and these errors are harder to spot. I agree that to my mind this is the most sensible option. > > If 'adc@...' really was the convention (or should be) I'd be clearly > in favor of following it. As mentioned above, in practice the use of > the 'adc@...' node name seems to be more prevalent for ADC controllers > than channels, so I'm more inclined towards 'usb-id-nopull@...' or > similar. > > All that said, these are just my preferences for the reasons outlined > above, if DT maintainers really want it to be 'adc@57' or some > variation of that, I'm fine with that too. Please let me know and we > can move forward with this trivial series. Rob, what's your view on this? Thanks, Jonathan > > Thanks > > Matthias
On Sun, Oct 21, 2018 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 18 Oct 2018 12:40:10 -0700 > Matthias Kaehlcke <mka@chromium.org> wrote: > > > On Fri, Oct 12, 2018 at 10:15:23AM -0700, Matthias Kaehlcke wrote: > > > On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > > > > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > > > > The node has a reg property, therefore its name should include a unit > > > > > address. > > > > > > > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > > > > DT conventions. > > > > > > > > This is ADC channels? If so, then DT convention would really be > > > > "adc@...". > > > > > > Is it really? A grep for 'adc@' in arch/${ARCH}/boot/dts yields > > > mostly ADC controller not channel nodes. > > > > > > I'm totally fine with changing the name to 'adc@...' if that's the > > > preference/convention, just want to reconfirm since the actual use is > > > a bit ambiguous. > > > > Could we please reach a conclusion on this? > > > > Summarizing the options on the table so far are: > > > > 1. usb-id-nopull@VADC_LR_MUX10_USB_ID > > 2. usb-id-nopull@57 > > 3. adc@VADC_LR_MUX10_USB_ID > > 4. adc@57 > > > > My personal preference goes to something <node name>@<define> > > since the unit address doesn't just resolve to an ADC channel number > > but also includes configuation information. A literal like '57' > > conveys less information than the define, it's easier to introduce > > errors and these errors are harder to spot. > > I agree that to my mind this is the most sensible option. If you really want the defines, then fine. Of course, that only works if the function is fixed. It won't work if the function is defined per board. Eventually, examples using defines will have to also include the headers. I plan to make the examples build-able. > > If 'adc@...' really was the convention (or should be) I'd be clearly > > in favor of following it. As mentioned above, in practice the use of > > the 'adc@...' node name seems to be more prevalent for ADC controllers > > than channels, so I'm more inclined towards 'usb-id-nopull@...' or > > similar. > > > > All that said, these are just my preferences for the reasons outlined > > above, if DT maintainers really want it to be 'adc@57' or some > > variation of that, I'm fine with that too. Please let me know and we > > can move forward with this trivial series. > > Rob, what's your view on this? I want node names that reflect the class of the node (not a specific model) and consistency across bindings. What that looks like for multi-channel ADCs is really up to you. There was another binding recently which mapped sub-nodes to inputs rather than channels. Maybe that's needed too if you have more inputs than simultaneous channels. Also, if your goal is to just quiet dtc warnings, then I'd prefer you not. They often point to bigger issues even if they can be fixed with trivial changes. Of course, if not fixed someone else will come along and try the trivial fix again. Rob
On Tue, 30 Oct 2018 11:00:16 -0500 Rob Herring <robh@kernel.org> wrote: > On Sun, Oct 21, 2018 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 18 Oct 2018 12:40:10 -0700 > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > On Fri, Oct 12, 2018 at 10:15:23AM -0700, Matthias Kaehlcke wrote: > > > > On Fri, Oct 05, 2018 at 03:47:43PM -0500, Rob Herring wrote: > > > > > On Wed, Oct 03, 2018 at 05:14:31PM -0700, Matthias Kaehlcke wrote: > > > > > > The node has a reg property, therefore its name should include a unit > > > > > > address. > > > > > > > > > > > > Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow > > > > > > DT conventions. > > > > > > > > > > This is ADC channels? If so, then DT convention would really be > > > > > "adc@...". > > > > > > > > Is it really? A grep for 'adc@' in arch/${ARCH}/boot/dts yields > > > > mostly ADC controller not channel nodes. > > > > > > > > I'm totally fine with changing the name to 'adc@...' if that's the > > > > preference/convention, just want to reconfirm since the actual use is > > > > a bit ambiguous. > > > > > > Could we please reach a conclusion on this? > > > > > > Summarizing the options on the table so far are: > > > > > > 1. usb-id-nopull@VADC_LR_MUX10_USB_ID > > > 2. usb-id-nopull@57 > > > 3. adc@VADC_LR_MUX10_USB_ID > > > 4. adc@57 > > > > > > My personal preference goes to something <node name>@<define> > > > since the unit address doesn't just resolve to an ADC channel number > > > but also includes configuation information. A literal like '57' > > > conveys less information than the define, it's easier to introduce > > > errors and these errors are harder to spot. > > > > I agree that to my mind this is the most sensible option. > > If you really want the defines, then fine. Of course, that only works > if the function is fixed. It won't work if the function is defined per > board. > > Eventually, examples using defines will have to also include the > headers. I plan to make the examples build-able. > > > > If 'adc@...' really was the convention (or should be) I'd be clearly > > > in favor of following it. As mentioned above, in practice the use of > > > the 'adc@...' node name seems to be more prevalent for ADC controllers > > > than channels, so I'm more inclined towards 'usb-id-nopull@...' or > > > similar. > > > > > > All that said, these are just my preferences for the reasons outlined > > > above, if DT maintainers really want it to be 'adc@57' or some > > > variation of that, I'm fine with that too. Please let me know and we > > > can move forward with this trivial series. > > > > Rob, what's your view on this? > > I want node names that reflect the class of the node (not a specific > model) and consistency across bindings. What that looks like for > multi-channel ADCs is really up to you. There was another binding > recently which mapped sub-nodes to inputs rather than channels. Maybe > that's needed too if you have more inputs than simultaneous channels. From a DT point of view the simultaneous channels vs inputs isn't meant to be currently visible. It's a userspace problem and from IIO point of view a 'channel' is an input (you just might not be able to enable all channels at once!) channel@ would work for me, though that might then match other types of device where this has a different meaning. Perhaps adc_chan@ ? > > Also, if your goal is to just quiet dtc warnings, then I'd prefer you > not. They often point to bigger issues even if they can be fixed with > trivial changes. Of course, if not fixed someone else will come along > and try the trivial fix again. Agreed. Would be good to have these consistent however as it is a fairly common thing to represent. Jonathan > > Rob
diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt index d0c188e5c922..ff7aa5038ba6 100644 --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt @@ -112,7 +112,7 @@ Example: io-channel-ranges; /* Channel node */ - usb_id_nopull { + usb-id-nopull@VADC_LR_MUX10_USB_ID { reg = <VADC_LR_MUX10_USB_ID>; qcom,decimation = <512>; qcom,ratiometric;
The node has a reg property, therefore its name should include a unit address. Also change the name from 'usb_id_nopull' to 'usb-id-nopull' to follow DT conventions. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Changes in v4: - patch added to the series --- Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)