diff mbox

[v2,1/2] devicetree: xilinx-xadc: Add optional label property

Message ID 1432135352-29848-1-git-send-email-xander.huff@ni.com
State New, archived
Headers show

Commit Message

Xander Huff May 20, 2015, 3:22 p.m. UTC
To better facilitate user-mode access to optional aux channels, allow
device trees to specify a custom label for defined channels.

Signed-off-by: Xander Huff <xander.huff@ni.com>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
Reviewed-by: Josh Cartwright <joshc@ni.com>
---
 Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jonathan Cameron June 7, 2015, 4:49 p.m. UTC | #1
On 20/05/15 16:22, Xander Huff wrote:
> To better facilitate user-mode access to optional aux channels, allow
> device trees to specify a custom label for defined channels.
> 
> Signed-off-by: Xander Huff <xander.huff@ni.com>
> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
> Reviewed-by: Josh Cartwright <joshc@ni.com>
> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index ce93bd8..bda4241 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1119,6 +1119,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>  			if (ret || reg > 16)
>  				continue;
>  
> +			of_property_read_string(child, "label",
> +						&chan->extend_name);
> +
>  			if (of_property_read_bool(child, "xlnx,bipolar"))
>  				chan->scan_type.sign = 's';
>  
> 
I think what Lars was suggesting was a change to the IIO side of this rather than
just changing the device tree binding.  Perhaps an additional per channel attribute
called _label which would allow for board specific labelling?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Cartwright June 8, 2015, 1:49 p.m. UTC | #2
On Sun, Jun 07, 2015 at 05:49:26PM +0100, Jonathan Cameron wrote:
> On 20/05/15 16:22, Xander Huff wrote:
> > To better facilitate user-mode access to optional aux channels, allow
> > device trees to specify a custom label for defined channels.
> > 
> > Signed-off-by: Xander Huff <xander.huff@ni.com>
> > Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
> > Reviewed-by: Josh Cartwright <joshc@ni.com>
> > ---
> >  drivers/iio/adc/xilinx-xadc-core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> > index ce93bd8..bda4241 100644
> > --- a/drivers/iio/adc/xilinx-xadc-core.c
> > +++ b/drivers/iio/adc/xilinx-xadc-core.c
> > @@ -1119,6 +1119,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
> >  			if (ret || reg > 16)
> >  				continue;
> >  
> > +			of_property_read_string(child, "label",
> > +						&chan->extend_name);
> > +
> >  			if (of_property_read_bool(child, "xlnx,bipolar"))
> >  				chan->scan_type.sign = 's';
> >  
> > 
> I think what Lars was suggesting was a change to the IIO side of this rather than
> just changing the device tree binding.

I'm not so sure.  His observation was just that the typical name of
these properties is 'label', which I think makes sense and is commonly
used.

Naming things is hard.

Perhaps, though, an improvement may be made if we lifted this out of the
xadc driver and moved it into the IIO core (I can't imagine that this
driver is the only one representing each channel with a separate node in
DT).

> Perhaps an additional per channel attribute called _label which would
> allow for board specific labelling?

This would seem a bit odd in DT-land; if a user wanted a different label
name for their board, they would just rename it in their device tree for
that board.  Fortunately, with the structured dtsi include files, this
is pretty painless to do.

  Josh
Jonathan Cameron June 14, 2015, 11:20 a.m. UTC | #3
On 08/06/15 14:49, Josh Cartwright wrote:
> On Sun, Jun 07, 2015 at 05:49:26PM +0100, Jonathan Cameron wrote:
>> On 20/05/15 16:22, Xander Huff wrote:
>>> To better facilitate user-mode access to optional aux channels, allow
>>> device trees to specify a custom label for defined channels.
>>>
>>> Signed-off-by: Xander Huff <xander.huff@ni.com>
>>> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
>>> Reviewed-by: Josh Cartwright <joshc@ni.com>
>>> ---
>>>  drivers/iio/adc/xilinx-xadc-core.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
>>> index ce93bd8..bda4241 100644
>>> --- a/drivers/iio/adc/xilinx-xadc-core.c
>>> +++ b/drivers/iio/adc/xilinx-xadc-core.c
>>> @@ -1119,6 +1119,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>>>  			if (ret || reg > 16)
>>>  				continue;
>>>  
>>> +			of_property_read_string(child, "label",
>>> +						&chan->extend_name);
>>> +
>>>  			if (of_property_read_bool(child, "xlnx,bipolar"))
>>>  				chan->scan_type.sign = 's';
>>>  
>>>
>> I think what Lars was suggesting was a change to the IIO side of this rather than
>> just changing the device tree binding.
> 
> I'm not so sure.  His observation was just that the typical name of
> these properties is 'label', which I think makes sense and is commonly
> used.
> 
> Naming things is hard.
> 
> Perhaps, though, an improvement may be made if we lifted this out of the
> xadc driver and moved it into the IIO core (I can't imagine that this
> driver is the only one representing each channel with a separate node in
> DT).
> 
>> Perhaps an additional per channel attribute called _label which would
>> allow for board specific labelling?
> 
> This would seem a bit odd in DT-land; if a user wanted a different label
> name for their board, they would just rename it in their device tree for
> that board.  Fortunately, with the structured dtsi include files, this
> is pretty painless to do.
I don't really mind how it is done.  From a userspace point of view in IIO
though I think these belong in their own attribute rather than in the naming
of the attributes. The extended name stuff just makes for really hideous
userspace code!

Hence you could use the device tree node naming to provide it, or some other
means, but I'd like to have it provided to userspace as say

in_voltage0_label which when read provides the name as a straightforward string.

This would obviously want to be provided by the IIO core.

As for moving the per channel DT node stuff into the IIO core, at this point
there are a few drivers doing it, but far from all, so the best bet would be
a utility function to help the drivers handle the manipulation rather than
direct support in the core.

Jonathan
> 
>   Josh
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt b/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt
index d71258e..19dcd96 100644
--- a/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt
@@ -68,6 +68,7 @@  Optional properties:
 			  Note each channel number should only be used at most
 			  once.
 		Optional properties:
+			* label: Custom label for the channel.
 			* xlnx,bipolar: If set the channel is used in bipolar
 			  mode.
 
@@ -107,6 +108,7 @@  Examples:
 			#size-cells = <0>;
 			channel@0 {
 				reg = <0>;
+				label = "vin_v";
 				xlnx,bipolar;
 			};
 		};