[PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Ivan T. Ivanov]

When #iio-cells is greater than '0', the driver could provide
a custom of_xlate function that reads the *args* and returns
the appropriate index in registered IIO channels array.

Add simple translation function, suitable for the most 1:1
mapped channels in IIO chips, and use it when driver did not
provide custom implementation.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/iio/inkern.c    | 32 +++++++++++++++++++++++++++-----
 include/linux/iio/iio.h |  8 ++++++++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index f084610..6c3e478 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -100,6 +100,28 @@ static int iio_dev_node_match(struct device *dev, void *data)
 	return dev->of_node == data && dev->type == &iio_device_type;
 }
 
+/**
+ * __of_iio_simple_xlate - translate iiospec to the IIO channel index
+ * @indio_dev:	pointer to the iio_dev structure
+ * @iiospec:	IIO specifier as found in the device tree
+ *
+ * This is simple translation function, suitable for the most 1:1 mapped
+ * channels in IIO chips. This function performs only one sanity check:
+ * whether IIO index is less than num_channels (that is specified in the
+ * iio_dev).
+ */
+static int __of_iio_simple_xlate(struct iio_dev *indio_dev,
+				const struct of_phandle_args *iiospec)
+{
+	if (!iiospec->args_count)
+		return 0;
+
+	if (iiospec->args[0] >= indio_dev->num_channels)
+		return -EINVAL;
+
+	return iiospec->args[0];
+}
+
 static int __of_iio_channel_get(struct iio_channel *channel,
 				struct device_node *np, int index)
 {
@@ -122,18 +144,18 @@ static int __of_iio_channel_get(struct iio_channel *channel,
 
 	indio_dev = dev_to_iio_dev(idev);
 	channel->indio_dev = indio_dev;
-	index = iiospec.args_count ? iiospec.args[0] : 0;
-	if (index >= indio_dev->num_channels) {
-		err = -EINVAL;
+	if (!indio_dev->of_xlate)
+		indio_dev->of_xlate = __of_iio_simple_xlate;
+	index = indio_dev->of_xlate(indio_dev, &iiospec);
+	if (index < 0)
 		goto err_put;
-	}
 	channel->channel = &indio_dev->channels[index];
 
 	return 0;
 
 err_put:
 	iio_device_put(indio_dev);
-	return err;
+	return index;
 }
 
 static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 15dc6bc..d5bb219 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -13,6 +13,7 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 #include <linux/iio/types.h>
+#include <linux/of.h>
 /* IIO TODO LIST */
 /*
  * Provide means of adjusting timer accuracy.
@@ -413,6 +414,11 @@ struct iio_buffer_setup_ops {
  * @currentmode:	[DRIVER] current operating mode
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
+ * @of_xlate:		[DRIVER] function pointer to obtain channel specifier
+ *			index. When #iio-cells is greater than '0', the driver
+ *			could provide a custom of_xlate function that reads
+ *			the *args* and returns the appropriate index in
+ *			registered IIO channels array.
  * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
  * @buffer:		[DRIVER] any buffer present
  * @buffer_list:	[INTERN] list of all buffers currently attached
@@ -451,6 +457,8 @@ struct iio_dev {
 	int				currentmode;
 	struct device			dev;
 
+	int (*of_xlate)(struct iio_dev *indio_dev,
+			const struct of_phandle_args *iiospec);
 	struct iio_event_interface	*event_interface;
 
 	struct iio_buffer		*buffer;
-- 
1.9.1

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Lars-Peter Clausen]

On 10/02/2014 02:32 PM, Ivan T. Ivanov wrote:
> When #iio-cells is greater than '0', the driver could provide
> a custom of_xlate function that reads the *args* and returns
> the appropriate index in registered IIO channels array.

Do you have an example of a device that doesn't want to use the default 
mapping? If yes please include it in the commit message, otherwise it is 
fairly hard to say whether this makes sense or not.

>
> Add simple translation function, suitable for the most 1:1
> mapped channels in IIO chips, and use it when driver did not
> provide custom implementation.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Ivan T. Ivanov]

On Thu, 2014-10-02 at 15:30 +0200, Lars-Peter Clausen wrote:
> On 10/02/2014 02:32 PM, Ivan T. Ivanov wrote:
> > When #iio-cells is greater than '0', the driver could provide
> > a custom of_xlate function that reads the *args* and returns
> > the appropriate index in registered IIO channels array.
> 
> Do you have an example of a device that doesn't want to use the default 
> mapping? If yes please include it in the commit message, otherwise it is 
> fairly hard to say whether this makes sense or not.

Still not mainlined. You can find more detailed description of the 
issue here[1] and driver here[2].

Regards,
Ivan

[1] https://lkml.org/lkml/2014/10/2/123
[2] http://www.spinics.net/lists/devicetree/msg50717.html

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Srinivas Pandruvada]

On Thu, 2014-10-02 at 16:49 +0300, Ivan T. Ivanov wrote:
> On Thu, 2014-10-02 at 15:30 +0200, Lars-Peter Clausen wrote:
> > On 10/02/2014 02:32 PM, Ivan T. Ivanov wrote:
> > > When #iio-cells is greater than '0', the driver could provide
> > > a custom of_xlate function that reads the *args* and returns
> > > the appropriate index in registered IIO channels array.
> > 
> > Do you have an example of a device that doesn't want to use the default 
> > mapping? If yes please include it in the commit message, otherwise it is 
> > fairly hard to say whether this makes sense or not.
> 
> Still not mainlined. You can find more detailed description of the 
> issue here[1] and driver here[2].
I see your need. I wonder this mapping be done in individual client
driver instead of creating another callback.

Thanks,
Srinivas

> 
> Regards,
> Ivan
> 
> [1] https://lkml.org/lkml/2014/10/2/123
> [2] http://www.spinics.net/lists/devicetree/msg50717.html
> 
> 

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Ivan T. Ivanov]

On Fri, 2014-10-03 at 07:31 -0700, Srinivas Pandruvada wrote:
> On Thu, 2014-10-02 at 16:49 +0300, Ivan T. Ivanov wrote:
> > On Thu, 2014-10-02 at 15:30 +0200, Lars-Peter Clausen wrote:
> > > On 10/02/2014 02:32 PM, Ivan T. Ivanov wrote:
> > > > When #iio-cells is greater than '0', the driver could provide
> > > > a custom of_xlate function that reads the *args* and returns
> > > > the appropriate index in registered IIO channels array.
> > > 
> > > Do you have an example of a device that doesn't want to use the default 
> > > mapping? If yes please include it in the commit message, otherwise it is 
> > > fairly hard to say whether this makes sense or not.
> > 
> > Still not mainlined. You can find more detailed description of the 
> > issue here[1] and driver here[2].
> I see your need. I wonder this mapping be done in individual client
> driver instead of creating another callback.

I am not sure I got you question. The idea is to provide default
1:1 mapping in core, which can be overwritten by client callback.

Regards,
Ivan

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Jonathan Cameron]

On 02/10/14 13:32, Ivan T. Ivanov wrote:
> When #iio-cells is greater than '0', the driver could provide
> a custom of_xlate function that reads the *args* and returns
> the appropriate index in registered IIO channels array.
> 
> Add simple translation function, suitable for the most 1:1
> mapped channels in IIO chips, and use it when driver did not
> provide custom implementation.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
Any more comments on this?  Been sat a while and the discussions seems
to have died out.

As Ivan has pointed out, very similar approaches are used
elsewhere (gpio for example).

> ---
>  drivers/iio/inkern.c    | 32 +++++++++++++++++++++++++++-----
>  include/linux/iio/iio.h |  8 ++++++++
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index f084610..6c3e478 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -100,6 +100,28 @@ static int iio_dev_node_match(struct device *dev, void *data)
>  	return dev->of_node == data && dev->type == &iio_device_type;
>  }
>  
> +/**
> + * __of_iio_simple_xlate - translate iiospec to the IIO channel index
> + * @indio_dev:	pointer to the iio_dev structure
> + * @iiospec:	IIO specifier as found in the device tree
> + *
> + * This is simple translation function, suitable for the most 1:1 mapped
> + * channels in IIO chips. This function performs only one sanity check:
> + * whether IIO index is less than num_channels (that is specified in the
> + * iio_dev).
> + */
> +static int __of_iio_simple_xlate(struct iio_dev *indio_dev,
> +				const struct of_phandle_args *iiospec)
> +{
> +	if (!iiospec->args_count)
> +		return 0;
> +
> +	if (iiospec->args[0] >= indio_dev->num_channels)
> +		return -EINVAL;
> +
> +	return iiospec->args[0];
> +}
> +
>  static int __of_iio_channel_get(struct iio_channel *channel,
>  				struct device_node *np, int index)
>  {
> @@ -122,18 +144,18 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>  
>  	indio_dev = dev_to_iio_dev(idev);
>  	channel->indio_dev = indio_dev;
> -	index = iiospec.args_count ? iiospec.args[0] : 0;
> -	if (index >= indio_dev->num_channels) {
> -		err = -EINVAL;
> +	if (!indio_dev->of_xlate)
> +		indio_dev->of_xlate = __of_iio_simple_xlate;
> +	index = indio_dev->of_xlate(indio_dev, &iiospec);
> +	if (index < 0)
>  		goto err_put;
> -	}
>  	channel->channel = &indio_dev->channels[index];
>  
>  	return 0;
>  
>  err_put:
>  	iio_device_put(indio_dev);
> -	return err;
> +	return index;
>  }
>  
>  static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 15dc6bc..d5bb219 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -13,6 +13,7 @@
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  #include <linux/iio/types.h>
> +#include <linux/of.h>
>  /* IIO TODO LIST */
>  /*
>   * Provide means of adjusting timer accuracy.
> @@ -413,6 +414,11 @@ struct iio_buffer_setup_ops {
>   * @currentmode:	[DRIVER] current operating mode
>   * @dev:		[DRIVER] device structure, should be assigned a parent
>   *			and owner
> + * @of_xlate:		[DRIVER] function pointer to obtain channel specifier
> + *			index. When #iio-cells is greater than '0', the driver
> + *			could provide a custom of_xlate function that reads
> + *			the *args* and returns the appropriate index in
> + *			registered IIO channels array.
>   * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
>   * @buffer:		[DRIVER] any buffer present
>   * @buffer_list:	[INTERN] list of all buffers currently attached
> @@ -451,6 +457,8 @@ struct iio_dev {
>  	int				currentmode;
>  	struct device			dev;
>  
> +	int (*of_xlate)(struct iio_dev *indio_dev,
> +			const struct of_phandle_args *iiospec);
>  	struct iio_event_interface	*event_interface;
>  
>  	struct iio_buffer		*buffer;
> 

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Lars-Peter Clausen]

On 10/18/2014 01:42 PM, Jonathan Cameron wrote:
> On 02/10/14 13:32, Ivan T. Ivanov wrote:
>> When #iio-cells is greater than '0', the driver could provide
>> a custom of_xlate function that reads the *args* and returns
>> the appropriate index in registered IIO channels array.
>>
>> Add simple translation function, suitable for the most 1:1
>> mapped channels in IIO chips, and use it when driver did not
>> provide custom implementation.
>>
>> Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> Any more comments on this?  Been sat a while and the discussions seems
> to have died out.
>
> As Ivan has pointed out, very similar approaches are used
> elsewhere (gpio for example).

Looks good to me:

Reviewed-by: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>

When we initially added the DT support to IIO I was hoping that we can get 
away with just using the simple and generic xlate function for all devices. 
But it looks as if some more complex devices need to overwrite it. We should 
be careful about adding new driver specific xlate implementations and make 
sure that it is actually needed.

One thing we might want to consider though is instead of adding the xlate 
callback to the iio_dev struct add it to the iio_info struct since it should 
be the same for different device instances of the same driver. And this is 
also where all the other callbacks are.

- Lars

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Jonathan Cameron]

On 18/10/14 12:50, Lars-Peter Clausen wrote:
> On 10/18/2014 01:42 PM, Jonathan Cameron wrote:
>> On 02/10/14 13:32, Ivan T. Ivanov wrote:
>>> When #iio-cells is greater than '0', the driver could provide
>>> a custom of_xlate function that reads the *args* and returns
>>> the appropriate index in registered IIO channels array.
>>>
>>> Add simple translation function, suitable for the most 1:1
>>> mapped channels in IIO chips, and use it when driver did not
>>> provide custom implementation.
>>>
>>> Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
>> Any more comments on this?  Been sat a while and the discussions seems
>> to have died out.
>>
>> As Ivan has pointed out, very similar approaches are used
>> elsewhere (gpio for example).
> 
> Looks good to me:
> 
> Reviewed-by: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> 
> When we initially added the DT support to IIO I was hoping that we can get away
> with just using the simple and generic xlate function for all devices. But it
> looks as if some more complex devices need to overwrite it. We should be careful
> about adding new driver specific xlate implementations and make sure that it is
> actually needed.
> 
> One thing we might want to consider though is instead of adding the xlate
> callback to the iio_dev struct add it to the iio_info struct since it should be
> the same for different device instances of the same driver. And this is also
> where all the other callbacks are.
Good point - would definitely prefer that.

J
> 
> - Lars

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Srinivas Pandruvada]

On Sat, 2014-10-18 at 12:42 +0100, Jonathan Cameron wrote:
> On 02/10/14 13:32, Ivan T. Ivanov wrote:
> > When #iio-cells is greater than '0', the driver could provide
> > a custom of_xlate function that reads the *args* and returns
> > the appropriate index in registered IIO channels array.
> > 
> > Add simple translation function, suitable for the most 1:1
> > mapped channels in IIO chips, and use it when driver did not
> > provide custom implementation.
> > 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> Any more comments on this?  Been sat a while and the discussions seems
> to have died out.
> 
> As Ivan has pointed out, very similar approaches are used
> elsewhere (gpio for example).
Looks useful to me.
Thanks,
Srinivas
> 
> > ---
> >  drivers/iio/inkern.c    | 32 +++++++++++++++++++++++++++-----
> >  include/linux/iio/iio.h |  8 ++++++++
> >  2 files changed, 35 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index f084610..6c3e478 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -100,6 +100,28 @@ static int iio_dev_node_match(struct device *dev, void *data)
> >  	return dev->of_node == data && dev->type == &iio_device_type;
> >  }
> >  
> > +/**
> > + * __of_iio_simple_xlate - translate iiospec to the IIO channel index
> > + * @indio_dev:	pointer to the iio_dev structure
> > + * @iiospec:	IIO specifier as found in the device tree
> > + *
> > + * This is simple translation function, suitable for the most 1:1 mapped
> > + * channels in IIO chips. This function performs only one sanity check:
> > + * whether IIO index is less than num_channels (that is specified in the
> > + * iio_dev).
> > + */
> > +static int __of_iio_simple_xlate(struct iio_dev *indio_dev,
> > +				const struct of_phandle_args *iiospec)
> > +{
> > +	if (!iiospec->args_count)
> > +		return 0;
> > +
> > +	if (iiospec->args[0] >= indio_dev->num_channels)
> > +		return -EINVAL;
> > +
> > +	return iiospec->args[0];
> > +}
> > +
> >  static int __of_iio_channel_get(struct iio_channel *channel,
> >  				struct device_node *np, int index)
> >  {
> > @@ -122,18 +144,18 @@ static int __of_iio_channel_get(struct iio_channel *channel,
> >  
> >  	indio_dev = dev_to_iio_dev(idev);
> >  	channel->indio_dev = indio_dev;
> > -	index = iiospec.args_count ? iiospec.args[0] : 0;
> > -	if (index >= indio_dev->num_channels) {
> > -		err = -EINVAL;
> > +	if (!indio_dev->of_xlate)
> > +		indio_dev->of_xlate = __of_iio_simple_xlate;
> > +	index = indio_dev->of_xlate(indio_dev, &iiospec);
> > +	if (index < 0)
> >  		goto err_put;
> > -	}
> >  	channel->channel = &indio_dev->channels[index];
> >  
> >  	return 0;
> >  
> >  err_put:
> >  	iio_device_put(indio_dev);
> > -	return err;
> > +	return index;
> >  }
> >  
> >  static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 15dc6bc..d5bb219 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -13,6 +13,7 @@
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> >  #include <linux/iio/types.h>
> > +#include <linux/of.h>
> >  /* IIO TODO LIST */
> >  /*
> >   * Provide means of adjusting timer accuracy.
> > @@ -413,6 +414,11 @@ struct iio_buffer_setup_ops {
> >   * @currentmode:	[DRIVER] current operating mode
> >   * @dev:		[DRIVER] device structure, should be assigned a parent
> >   *			and owner
> > + * @of_xlate:		[DRIVER] function pointer to obtain channel specifier
> > + *			index. When #iio-cells is greater than '0', the driver
> > + *			could provide a custom of_xlate function that reads
> > + *			the *args* and returns the appropriate index in
> > + *			registered IIO channels array.
> >   * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
> >   * @buffer:		[DRIVER] any buffer present
> >   * @buffer_list:	[INTERN] list of all buffers currently attached
> > @@ -451,6 +457,8 @@ struct iio_dev {
> >  	int				currentmode;
> >  	struct device			dev;
> >  
> > +	int (*of_xlate)(struct iio_dev *indio_dev,
> > +			const struct of_phandle_args *iiospec);
> >  	struct iio_event_interface	*event_interface;
> >  
> >  	struct iio_buffer		*buffer;
> > 

Re: [PATCH] iio: inkern: Add of_xlate function to struct iio_dev

[Ivan T. Ivanov]

On Sat, 2014-10-18 at 13:14 +0100, Jonathan Cameron wrote:
> On 18/10/14 12:50, Lars-Peter Clausen wrote:
> >  On 10/18/2014 01:42 PM, Jonathan Cameron wrote:
> > >  On 02/10/14 13:32, Ivan T. Ivanov wrote:
> > > >  When #iio-cells is greater than '0', the driver could provide
> > > >  a custom of_xlate function that reads the *args* and returns
> > > >  the appropriate index in registered IIO channels array.
> > > > >
> > > >  Add simple translation function, suitable for the most 1:1
> > > >  mapped channels in IIO chips, and use it when driver did not
> > > >  provide custom implementation.
> > > > >
> > > >  Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> > >  Any more comments on this?  Been sat a while and the 
> > > discussions seems
> > >  to have died out.
> > > >
> > >  As Ivan has pointed out, very similar approaches are used
> > >  elsewhere (gpio for example).
> > 
> >  Looks good to me:
> > 
> >  Reviewed-by: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> > 
> >  When we initially added the DT support to IIO I was hoping that 
> > we can get away
> >  with just using the simple and generic xlate function for all 
> > devices. But it
> >  looks as if some more complex devices need to overwrite it. We 
> > should be careful
> >  about adding new driver specific xlate implementations and make 
> > sure that it is
> >  actually needed.
> > 
> >  One thing we might want to consider though is instead of adding 
> > the xlate
> >  callback to the iio_dev struct add it to the iio_info struct 
> > since it should be
> >  the same for different device instances of the same driver. And 
> > this is also
> >  where all the other callbacks are.
> Good point - would definitely prefer that.

Thank you. Will rework it as suggested.

Regards,
Ivan

> 
> J
> > 
> >  - Lars
>