| Message ID | 1412253128-32165-1-git-send-email-iivanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 good to me: Reviewed-by: Lars-Peter Clausen <lars@metafoo.de> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 good to me: > > Reviewed-by: Lars-Peter Clausen <lars@metafoo.de> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 good to me: > > > > Reviewed-by: Lars-Peter Clausen <lars@metafoo.de> > > > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)