linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC
@ 2018-10-04  0:14 Matthias Kaehlcke
  2018-10-04  0:14 ` [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull' Matthias Kaehlcke
  2018-10-04  0:14 ` [PATCH v5 2/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
  0 siblings, 2 replies; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-04  0:14 UTC (permalink / raw)
  To: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson, Matthias Kaehlcke

This series adds a channel for the die temperature to the QCOM SPMI
PMIC5 ADC. It also fixes an example in the DT documentation.

The die temperature is going to be used by the temperature alarm driver
(https://lore.kernel.org/patchwork/project/lkml/list/?series=361416).

The former name of the series was "arm64: dts: qcom: pm8998: Add ADC
node and die temperature channel"
(https://patchwork.kernel.org/project/linux-arm-msm/list/?series=17165),
2 of the 3 patches of this series already landed in Andy's tree, hence
the new name.

Matthias Kaehlcke (2):
  dt-bindings: iio: vadc: Update example to include unit address for
    node 'usb-id-nopull'
  arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC

 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 2 +-
 arch/arm64/boot/dts/qcom/pm8998.dtsi                         | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-04  0:14 [PATCH v5 0/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
@ 2018-10-04  0:14 ` Matthias Kaehlcke
  2018-10-04 15:14   ` Doug Anderson
  2018-10-05 20:47   ` Rob Herring
  2018-10-04  0:14 ` [PATCH v5 2/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
  1 sibling, 2 replies; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-04  0:14 UTC (permalink / raw)
  To: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson, Matthias Kaehlcke

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>;
 			qcom,decimation = <512>;
 			qcom,ratiometric;
-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 2/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC
  2018-10-04  0:14 [PATCH v5 0/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
  2018-10-04  0:14 ` [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull' Matthias Kaehlcke
@ 2018-10-04  0:14 ` Matthias Kaehlcke
  2018-10-04 16:46   ` Doug Anderson
  1 sibling, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-04  0:14 UTC (permalink / raw)
  To: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson, Matthias Kaehlcke

Add a channel node for the die temperature to the ADC.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v4:
- added unit address to 'die-temp' node
---
 arch/arm64/boot/dts/qcom/pm8998.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 048f19fa0150..32b141637d13 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -75,6 +75,11 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			#io-channel-cells = <1>;
+
+			die-temp@ADC5_DIE_TEMP {
+				reg = <ADC5_DIE_TEMP>;
+				label = "die_temp";
+			};
 		};
 
 		rtc@6000 {
-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-04  0:14 ` [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull' Matthias Kaehlcke
@ 2018-10-04 15:14   ` Doug Anderson
  2018-10-05 20:47   ` Rob Herring
  1 sibling, 0 replies; 13+ messages in thread
From: Doug Anderson @ 2018-10-04 15:14 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-arm-msm,
	open list:ARM/QUALCOMM SUPPORT, devicetree, linux-iio, LKML

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>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC
  2018-10-04  0:14 ` [PATCH v5 2/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
@ 2018-10-04 16:46   ` Doug Anderson
  0 siblings, 0 replies; 13+ messages in thread
From: Doug Anderson @ 2018-10-04 16:46 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-arm-msm,
	open list:ARM/QUALCOMM SUPPORT, devicetree, linux-iio, LKML

Hi,
On Wed, Oct 3, 2018 at 5:14 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Add a channel node for the die temperature to the ADC.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v4:
> - added unit address to 'die-temp' node

This probably should have been "changes in v5"

> ---
>  arch/arm64/boot/dts/qcom/pm8998.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index 048f19fa0150..32b141637d13 100644
> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> @@ -75,6 +75,11 @@
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         #io-channel-cells = <1>;
> +
> +                       die-temp@ADC5_DIE_TEMP {
> +                               reg = <ADC5_DIE_TEMP>;

Thanks for adding the unit address.  Note to Andy that this ought to
go into the queue for 4.20 since you'll have the ADC5_DIE_TEMP then.


-Doug

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-04  0:14 ` [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull' Matthias Kaehlcke
  2018-10-04 15:14   ` Doug Anderson
@ 2018-10-05 20:47   ` Rob Herring
  2018-10-11 19:11     ` Matthias Kaehlcke
  2018-10-12 17:15     ` Matthias Kaehlcke
  1 sibling, 2 replies; 13+ messages in thread
From: Rob Herring @ 2018-10-05 20:47 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Andy Gross, David Brown, Mark Rutland, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson

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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-05 20:47   ` Rob Herring
@ 2018-10-11 19:11     ` Matthias Kaehlcke
  2018-10-11 20:52       ` Matthias Kaehlcke
  2018-10-12 17:15     ` Matthias Kaehlcke
  1 sibling, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-11 19:11 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andy Gross, David Brown, Mark Rutland, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-11 19:11     ` Matthias Kaehlcke
@ 2018-10-11 20:52       ` Matthias Kaehlcke
  0 siblings, 0 replies; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-11 20:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andy Gross, David Brown, Mark Rutland, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-05 20:47   ` Rob Herring
  2018-10-11 19:11     ` Matthias Kaehlcke
@ 2018-10-12 17:15     ` Matthias Kaehlcke
  2018-10-18 19:40       ` Matthias Kaehlcke
  1 sibling, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-12 17:15 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andy Gross, David Brown, Mark Rutland, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-12 17:15     ` Matthias Kaehlcke
@ 2018-10-18 19:40       ` Matthias Kaehlcke
  2018-10-21 14:17         ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2018-10-18 19:40 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andy Gross, David Brown, Mark Rutland, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-18 19:40       ` Matthias Kaehlcke
@ 2018-10-21 14:17         ` Jonathan Cameron
  2018-10-30 16:00           ` Rob Herring
  0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2018-10-21 14:17 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Rob Herring, Andy Gross, David Brown, Mark Rutland,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-arm-msm, linux-soc, devicetree, linux-iio, linux-kernel,
	Douglas Anderson

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-21 14:17         ` Jonathan Cameron
@ 2018-10-30 16:00           ` Rob Herring
  2018-11-02  9:28             ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2018-10-30 16:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Matthias Kaehlcke, Andy Gross, David Brown, Mark Rutland,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald,
	linux-arm-msm, open list:ARM/QUALCOMM SUPPORT, devicetree,
	linux-iio, linux-kernel, Doug Anderson

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull'
  2018-10-30 16:00           ` Rob Herring
@ 2018-11-02  9:28             ` Jonathan Cameron
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-11-02  9:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: Jonathan Cameron, Matthias Kaehlcke, Andy Gross, David Brown,
	Mark Rutland, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald,
	linux-arm-msm, open list:ARM/QUALCOMM SUPPORT, devicetree,
	linux-iio, linux-kernel, Doug Anderson

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-11-02  9:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-04  0:14 [PATCH v5 0/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
2018-10-04  0:14 ` [PATCH v5 1/2] dt-bindings: iio: vadc: Update example to include unit address for node 'usb-id-nopull' Matthias Kaehlcke
2018-10-04 15:14   ` Doug Anderson
2018-10-05 20:47   ` Rob Herring
2018-10-11 19:11     ` Matthias Kaehlcke
2018-10-11 20:52       ` Matthias Kaehlcke
2018-10-12 17:15     ` Matthias Kaehlcke
2018-10-18 19:40       ` Matthias Kaehlcke
2018-10-21 14:17         ` Jonathan Cameron
2018-10-30 16:00           ` Rob Herring
2018-11-02  9:28             ` Jonathan Cameron
2018-10-04  0:14 ` [PATCH v5 2/2] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC Matthias Kaehlcke
2018-10-04 16:46   ` Doug Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).