[LINUX PATCH] iio: xadc: fix for extended name remains same issue

[LINUX PATCH] iio: xadc: fix for extended name remains same issue

[Ranjit Waghmode]

If we read channel details based on the device tree entry; channel’s scan_index
and channel’s address were overwritten by scan_index and address of channel
based on device tree. But extended name were remaining same because of which
sysfs entry names were continuous.

So this patch takes care of the above issue by updating 'channels' structure
inside xadc_parse_dt() function with the help of 'xadc_channels' structure
so that every channel's parameters will be updated properly. Also for the
sake of simplicity this patch updates default values for extend_name.

Signed-off-by: Ranjit Waghmode <ranjit.waghmode@xilinx.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 52 +++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index a221f73..b3f158f 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1015,23 +1015,23 @@ static const struct iio_chan_spec xadc_channels[] = {
 	XADC_CHAN_VOLTAGE(5, 7, XADC_REG_VCCO_DDR, "vccoddr", true),
 	XADC_CHAN_VOLTAGE(6, 12, XADC_REG_VREFP, "vrefp", false),
 	XADC_CHAN_VOLTAGE(7, 13, XADC_REG_VREFN, "vrefn", false),
-	XADC_CHAN_VOLTAGE(8, 11, XADC_REG_VPVN, NULL, false),
-	XADC_CHAN_VOLTAGE(9, 16, XADC_REG_VAUX(0), NULL, false),
-	XADC_CHAN_VOLTAGE(10, 17, XADC_REG_VAUX(1), NULL, false),
-	XADC_CHAN_VOLTAGE(11, 18, XADC_REG_VAUX(2), NULL, false),
-	XADC_CHAN_VOLTAGE(12, 19, XADC_REG_VAUX(3), NULL, false),
-	XADC_CHAN_VOLTAGE(13, 20, XADC_REG_VAUX(4), NULL, false),
-	XADC_CHAN_VOLTAGE(14, 21, XADC_REG_VAUX(5), NULL, false),
-	XADC_CHAN_VOLTAGE(15, 22, XADC_REG_VAUX(6), NULL, false),
-	XADC_CHAN_VOLTAGE(16, 23, XADC_REG_VAUX(7), NULL, false),
-	XADC_CHAN_VOLTAGE(17, 24, XADC_REG_VAUX(8), NULL, false),
-	XADC_CHAN_VOLTAGE(18, 25, XADC_REG_VAUX(9), NULL, false),
-	XADC_CHAN_VOLTAGE(19, 26, XADC_REG_VAUX(10), NULL, false),
-	XADC_CHAN_VOLTAGE(20, 27, XADC_REG_VAUX(11), NULL, false),
-	XADC_CHAN_VOLTAGE(21, 28, XADC_REG_VAUX(12), NULL, false),
-	XADC_CHAN_VOLTAGE(22, 29, XADC_REG_VAUX(13), NULL, false),
-	XADC_CHAN_VOLTAGE(23, 30, XADC_REG_VAUX(14), NULL, false),
-	XADC_CHAN_VOLTAGE(24, 31, XADC_REG_VAUX(15), NULL, false),
+	XADC_CHAN_VOLTAGE(8, 11, XADC_REG_VPVN, "vpvn", false),
+	XADC_CHAN_VOLTAGE(9, 16, XADC_REG_VAUX(0), "vaux0", false),
+	XADC_CHAN_VOLTAGE(10, 17, XADC_REG_VAUX(1), "vaux1", false),
+	XADC_CHAN_VOLTAGE(11, 18, XADC_REG_VAUX(2), "vaux2", false),
+	XADC_CHAN_VOLTAGE(12, 19, XADC_REG_VAUX(3), "vaux3", false),
+	XADC_CHAN_VOLTAGE(13, 20, XADC_REG_VAUX(4), "vaux4", false),
+	XADC_CHAN_VOLTAGE(14, 21, XADC_REG_VAUX(5), "vaux5", false),
+	XADC_CHAN_VOLTAGE(15, 22, XADC_REG_VAUX(6), "vaux6", false),
+	XADC_CHAN_VOLTAGE(16, 23, XADC_REG_VAUX(7), "vaux7", false),
+	XADC_CHAN_VOLTAGE(17, 24, XADC_REG_VAUX(8), "vaux8", false),
+	XADC_CHAN_VOLTAGE(18, 25, XADC_REG_VAUX(9), "vaux9", false),
+	XADC_CHAN_VOLTAGE(19, 26, XADC_REG_VAUX(10), "vaux10", false),
+	XADC_CHAN_VOLTAGE(20, 27, XADC_REG_VAUX(11), "vaux11", false),
+	XADC_CHAN_VOLTAGE(21, 28, XADC_REG_VAUX(12), "vaux12", false),
+	XADC_CHAN_VOLTAGE(22, 29, XADC_REG_VAUX(13), "vaux13", false),
+	XADC_CHAN_VOLTAGE(23, 30, XADC_REG_VAUX(14), "vaux14", false),
+	XADC_CHAN_VOLTAGE(24, 31, XADC_REG_VAUX(15), "vaux15", false),
 };

 static const struct iio_info xadc_info = {
@@ -1056,7 +1056,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	unsigned int *conf)
 {
 	struct xadc *xadc = iio_priv(indio_dev);
-	struct iio_chan_spec *channels, *chan;
+	struct iio_chan_spec *channels;
 	struct device_node *chan_node, *child;
 	unsigned int num_channels;
 	const char *external_mux;
@@ -1104,7 +1104,6 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 		return -ENOMEM;

 	num_channels = 9;
-	chan = &channels[9];

 	chan_node = of_get_child_by_name(np, "xlnx,channels");
 	if (chan_node) {
@@ -1114,22 +1113,17 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 				break;
 			}

+			channels[num_channels] = xadc_channels[reg + 9];
+			channels[num_channels].channel = num_channels - 1;
+
 			ret = of_property_read_u32(child, "reg", &reg);
 			if (ret || reg > 16)
 				continue;

 			if (of_property_read_bool(child, "xlnx,bipolar"))
-				chan->scan_type.sign = 's';
-
-			if (reg == 0) {
-				chan->scan_index = 11;
-				chan->address = XADC_REG_VPVN;
-			} else {
-				chan->scan_index = 15 + reg;
-				chan->address = XADC_REG_VAUX(reg - 1);
-			}
+				channels[num_channels].scan_type.sign = 's';
+
 			num_channels++;
-			chan++;
 		}
 	}
 	of_node_put(chan_node);
--
2.1.2

Re: [LINUX PATCH] iio: xadc: fix for extended name remains same issue

[Lars-Peter Clausen]

On 11/24/2015 07:17 AM, Ranjit Waghmode wrote:
> If we read channel details based on the device tree entry; channel’s scan_index
> and channel’s address were overwritten by scan_index and address of channel
> based on device tree. But extended name were remaining same because of which
> sysfs entry names were continuous.

I don't understand this part. Those channels don't have a extended name,
they are always NULL. Those are added as part of this patch as well, but the
patch description makes it sound like it is a fix for a existing issue.


> 
> So this patch takes care of the above issue by updating 'channels' structure
> inside xadc_parse_dt() function with the help of 'xadc_channels' structure
> so that every channel's parameters will be updated properly. Also for the
> sake of simplicity this patch updates default values for extend_name.

We can't really do that, that would break existing applications. And to be
honest I don't think regular channels should have a extended name.

Re: [LINUX PATCH] iio: xadc: fix for extended name remains same issue

[Ranjit Abhimanyu Waghmode]

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]



On 11/24/2015 1:14 PM, Lars-Peter Clausen wrote:
> On 11/24/2015 07:17 AM, Ranjit Waghmode wrote: >> If we read channel details based on the device tree entry; channel’s scan_index >> and channel’s address were overwritten by scan_index and address of channel >> based on device tree. But extended name were remaining same because of which >> sysfs entry names were continuous. > > I don't understand this part. Those channels don't have a extended name, > they are always NULL. Those are added as part of this patch as well, but the > patch description makes it sound like it is a fix for a existing issue. > >

Sorry about the confusing description.

Actually when we try to enable the external channels in XADC by adding
entries in dts, we can see that the channel numbers are not considered
and they are taken as entries.

For e.g if we have entries in device tree like below:

xlnx,channels {
    #address-cells = <1>;
    #size-cells = <0>;

    channel@3 {
        reg = <3>;
    };
    channel@1 {
        reg = <1>;
    };
    channel@4 {
        reg = <4>;
    };
    channel@5 {
        reg = <5>;
    };
    channel@7 {
        reg = <7>;
    };
    channel@8 {
        reg = <8>;
    };
    channel@9 {
        reg = <9>;
    };
};


Then here observed that channel's scan_index and channel's address are
getting overwritten by scan_index of channel based on the device tree
entry. But the extended name remains same that is why it shows names are
continuous. It seems difficult to follow the VPVN and AUX channel
entries in sysfs entries.

>> >> So this patch takes care of the above issue by updating 'channels' structure >> inside xadc_parse_dt() function with the help of 'xadc_channels' structure >> so that every channel's parameters will be updated properly. Also for the >> sake of simplicity this patch updates default values for extend_name. > > We can't really do that, that would break existing applications. And to be > honest I don't think regular channels should have a extended name. >

So based on the above description of problem I tried to update the code.
But if you feel that it is not the ideal way of doing then can you please
help me in understanding how could I overcome this issue.

Regards,
Ranjit


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.


[-- Attachment #2: Type: text/html, Size: 3796 bytes --]