| Message ID | 1370449495-29981-13-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/05/2013 05:24 PM, Sebastian Andrzej Siewior wrote: > From: Pantelis Antoniou <panto@antoniou-consulting.com> > > Add an IIO map interface that consumers can use. > Nicely done and much cleaner. Thanks, > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > [bigeasy: use static AIN[0-8] names, plugged a mem leak, use kcalloc()] > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Acked-by: Jonathan Cameron <jic23@kernel.org> > --- > drivers/iio/adc/ti_am335x_adc.c | 57 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 51 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index 502e929..c6695a8 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -24,12 +24,15 @@ > #include <linux/iio/iio.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/iio/machine.h> > +#include <linux/iio/driver.h> > > #include <linux/mfd/ti_am335x_tscadc.h> > > struct tiadc_device { > struct ti_tscadc_dev *mfd_tscadc; > int channels; > + struct iio_map *map; > }; > > static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) > @@ -84,34 +87,76 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) > am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en); > } > > +static const char * const chan_name_ain[] = { > + "AIN0", > + "AIN1", > + "AIN2", > + "AIN3", > + "AIN4", > + "AIN5", > + "AIN6", > + "AIN7", > +}; > + > static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) > { > + struct tiadc_device *adc_dev = iio_priv(indio_dev); > struct iio_chan_spec *chan_array; > - int i; > + struct iio_chan_spec *chan; > + int i, ret; > > indio_dev->num_channels = channels; > - chan_array = kcalloc(indio_dev->num_channels, > + chan_array = kcalloc(channels, > sizeof(struct iio_chan_spec), GFP_KERNEL); > - > if (chan_array == NULL) > return -ENOMEM; > > - for (i = 0; i < (indio_dev->num_channels); i++) { > - struct iio_chan_spec *chan = chan_array + i; > + chan = chan_array; > + for (i = 0; i < channels; i++, chan++) { > + > chan->type = IIO_VOLTAGE; > chan->indexed = 1; > chan->channel = i; > chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW); > + chan->datasheet_name = chan_name_ain[i]; > + chan->scan_type.sign = 'u'; > + chan->scan_type.realbits = 12; > + chan->scan_type.storagebits = 32; > + chan->scan_type.shift = 0; > } > > indio_dev->channels = chan_array; > > - return indio_dev->num_channels; > + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), > + GFP_KERNEL); > + if (adc_dev->map == NULL) > + goto err_free_chan; > + > + for (i = 0; i < channels; i++) { > + adc_dev->map[i].adc_channel_label = > + chan_array[i].datasheet_name; > + adc_dev->map[i].consumer_dev_name = "any"; > + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; > + } > + > + ret = iio_map_array_register(indio_dev, adc_dev->map); > + if (ret) > + goto err_map; > + > + return 0; > +err_map: > + kfree(adc_dev->map); > +err_free_chan: > + kfree(chan_array); > + return -ENOMEM; > } > > static void tiadc_channels_remove(struct iio_dev *indio_dev) > { > + struct tiadc_device *adc_dev = iio_priv(indio_dev); > + > kfree(indio_dev->channels); > + kfree(adc_dev->map); > } > > static int tiadc_read_raw(struct iio_dev *indio_dev, > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2013 06:24 PM, Sebastian Andrzej Siewior wrote: [...] > - return indio_dev->num_channels; > + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), > + GFP_KERNEL); > + if (adc_dev->map == NULL) > + goto err_free_chan; > + > + for (i = 0; i < channels; i++) { > + adc_dev->map[i].adc_channel_label = > + chan_array[i].datasheet_name; > + adc_dev->map[i].consumer_dev_name = "any"; > + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; > + } This is not the way the IIO map interface is supposed to be used and I doubt it will actually work at all. The map either needs to be provided by board code with the proper consumer device and channel name filled in or in your case where you use devicetree you don't need to provide a map at all since this will all be handled by the generic IIO devicetree bindings. So I'd just drop this patch. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/09/2013 06:41 PM, Lars-Peter Clausen wrote: > On 06/05/2013 06:24 PM, Sebastian Andrzej Siewior wrote: > [...] >> - return indio_dev->num_channels; >> + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), >> + GFP_KERNEL); >> + if (adc_dev->map == NULL) >> + goto err_free_chan; >> + >> + for (i = 0; i < channels; i++) { >> + adc_dev->map[i].adc_channel_label = >> + chan_array[i].datasheet_name; >> + adc_dev->map[i].consumer_dev_name = "any"; >> + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; >> + } > > This is not the way the IIO map interface is supposed to be used and I doubt it > will actually work at all. The map either needs to be provided by board code > with the proper consumer device and channel name filled in or in your case > where you use devicetree you don't need to provide a map at all since this will > all be handled by the generic IIO devicetree bindings. So I'd just drop this patch. The channels are not described in the device tree. If so how would that be the case? Right now, I don't have any consumer. How do I test this easily if this is done correctly? If in doubt it will probably drop this until someone comes along who actually needs this. > > - Lars Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sebastian, On Jun 10, 2013, at 10:40 AM, Sebastian Andrzej Siewior wrote: > On 06/09/2013 06:41 PM, Lars-Peter Clausen wrote: >> On 06/05/2013 06:24 PM, Sebastian Andrzej Siewior wrote: >> [...] >>> - return indio_dev->num_channels; >>> + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), >>> + GFP_KERNEL); >>> + if (adc_dev->map == NULL) >>> + goto err_free_chan; >>> + >>> + for (i = 0; i < channels; i++) { >>> + adc_dev->map[i].adc_channel_label = >>> + chan_array[i].datasheet_name; >>> + adc_dev->map[i].consumer_dev_name = "any"; >>> + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; >>> + } >> >> This is not the way the IIO map interface is supposed to be used and I doubt it >> will actually work at all. The map either needs to be provided by board code >> with the proper consumer device and channel name filled in or in your case >> where you use devicetree you don't need to provide a map at all since this will >> all be handled by the generic IIO devicetree bindings. So I'd just drop this patch. > > The channels are not described in the device tree. If so how would that > be the case? Right now, I don't have any consumer. How do I test this > easily if this is done correctly? > If in doubt it will probably drop this until someone comes along who > actually needs this. > Please don't remove this immediately. We need the hardcoded names until we use DT bindings that get a handle of the IIO channels. I realize this is a temporary solution though until the driver is reworked to use DT properly. >> >> - Lars > > Sebastian Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/10/2013 10:10 AM, Pantelis Antoniou wrote: > Hi Sebastian, > > On Jun 10, 2013, at 10:40 AM, Sebastian Andrzej Siewior wrote: > >> On 06/09/2013 06:41 PM, Lars-Peter Clausen wrote: >>> On 06/05/2013 06:24 PM, Sebastian Andrzej Siewior wrote: >>> [...] >>>> - return indio_dev->num_channels; >>>> + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), >>>> + GFP_KERNEL); >>>> + if (adc_dev->map == NULL) >>>> + goto err_free_chan; >>>> + >>>> + for (i = 0; i < channels; i++) { >>>> + adc_dev->map[i].adc_channel_label = >>>> + chan_array[i].datasheet_name; >>>> + adc_dev->map[i].consumer_dev_name = "any"; >>>> + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; >>>> + } >>> >>> This is not the way the IIO map interface is supposed to be used and I doubt it >>> will actually work at all. The map either needs to be provided by board code >>> with the proper consumer device and channel name filled in or in your case >>> where you use devicetree you don't need to provide a map at all since this will >>> all be handled by the generic IIO devicetree bindings. So I'd just drop this patch. >> >> The channels are not described in the device tree. If so how would that >> be the case? Right now, I don't have any consumer. How do I test this >> easily if this is done correctly? >> If in doubt it will probably drop this until someone comes along who >> actually needs this. >> > > Please don't remove this immediately. We need the hardcoded names until > we use DT bindings that get a handle of the IIO channels. > > I realize this is a temporary solution though until the driver is reworked > to use DT properly. It's not a solution at all, since it simply won't work. You have two options, either specify the mapping in your board code based on the consumers, or use devicetree to get a handle to device and channel. Btw. from the consumers side it won't matter which way is used, the same code will work with both, so there is no need to add DT support to the consumer. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lars, On Jun 10, 2013, at 11:32 AM, Lars-Peter Clausen wrote: > On 06/10/2013 10:10 AM, Pantelis Antoniou wrote: >> Hi Sebastian, >> >> On Jun 10, 2013, at 10:40 AM, Sebastian Andrzej Siewior wrote: >> >>> On 06/09/2013 06:41 PM, Lars-Peter Clausen wrote: >>>> On 06/05/2013 06:24 PM, Sebastian Andrzej Siewior wrote: >>>> [...] >>>>> - return indio_dev->num_channels; >>>>> + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), >>>>> + GFP_KERNEL); >>>>> + if (adc_dev->map == NULL) >>>>> + goto err_free_chan; >>>>> + >>>>> + for (i = 0; i < channels; i++) { >>>>> + adc_dev->map[i].adc_channel_label = >>>>> + chan_array[i].datasheet_name; >>>>> + adc_dev->map[i].consumer_dev_name = "any"; >>>>> + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; >>>>> + } >>>> >>>> This is not the way the IIO map interface is supposed to be used and I doubt it >>>> will actually work at all. The map either needs to be provided by board code >>>> with the proper consumer device and channel name filled in or in your case >>>> where you use devicetree you don't need to provide a map at all since this will >>>> all be handled by the generic IIO devicetree bindings. So I'd just drop this patch. >>> >>> The channels are not described in the device tree. If so how would that >>> be the case? Right now, I don't have any consumer. How do I test this >>> easily if this is done correctly? >>> If in doubt it will probably drop this until someone comes along who >>> actually needs this. >>> >> >> Please don't remove this immediately. We need the hardcoded names until >> we use DT bindings that get a handle of the IIO channels. >> >> I realize this is a temporary solution though until the driver is reworked >> to use DT properly. > > It's not a solution at all, since it simply won't work. > > You have two options, either specify the mapping in your board code based on > the consumers, or use devicetree to get a handle to device and channel. Btw. > from the consumers side it won't matter which way is used, the same code will > work with both, so there is no need to add DT support to the consumer. > Well, when we did this initially there was no other way. I'll check out mainline and see if it will work for our case. We're slowly working towards rebasing all our stuff against mainline again. > - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/10/2013 09:40 AM, Sebastian Andrzej Siewior wrote: > On 06/09/2013 06:41 PM, Lars-Peter Clausen wrote: >> On 06/05/2013 06:24 PM, Sebastian Andrzej Siewior wrote: >> [...] >>> - return indio_dev->num_channels; >>> + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), >>> + GFP_KERNEL); >>> + if (adc_dev->map == NULL) >>> + goto err_free_chan; >>> + >>> + for (i = 0; i < channels; i++) { >>> + adc_dev->map[i].adc_channel_label = >>> + chan_array[i].datasheet_name; >>> + adc_dev->map[i].consumer_dev_name = "any"; >>> + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; >>> + } >> >> This is not the way the IIO map interface is supposed to be used and I doubt it >> will actually work at all. The map either needs to be provided by board code >> with the proper consumer device and channel name filled in or in your case >> where you use devicetree you don't need to provide a map at all since this will >> all be handled by the generic IIO devicetree bindings. So I'd just drop this patch. > > The channels are not described in the device tree. If so how would that > be the case? Right now, I don't have any consumer. How do I test this > easily if this is done correctly? > If in doubt it will probably drop this until someone comes along who > actually needs this. You don't need to describe the channels in devicetree, except setting the "#io-channel-cells" property of your ADC node to 1. You also need to initialize the of_node property of your iio_device's struct's 'dev' field to the your adc node. After that it's just a matter of using, e.g. io-channels = <&adc 1>; io-channel-names = "voltage"; in your consumer. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/10/2013 10:38 AM, Lars-Peter Clausen wrote: > > You don't need to describe the channels in devicetree, except setting the > "#io-channel-cells" property of your ADC node to 1. You also need to initialize > the of_node property of your iio_device's struct's 'dev' field to the your adc > node. Aha. So right now I do have in my device tree: adc { ti,adc-channels = <4>; }; for the producer ADC node. But this seems wrong. Correct would be adctsc: adc { ti,adc-channels = <4>; #io-channel-cells = <1>; }; And the consumer can then specify everything. This seems to be looking good. And with this the map piece can die. Thanks. > > After that it's just a matter of using, e.g. > > io-channels = <&adc 1>; > io-channel-names = "voltage"; > > in your consumer. > > - Lars Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" 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/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c index 502e929..c6695a8 100644 --- a/drivers/iio/adc/ti_am335x_adc.c +++ b/drivers/iio/adc/ti_am335x_adc.c @@ -24,12 +24,15 @@ #include <linux/iio/iio.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/iio/machine.h> +#include <linux/iio/driver.h> #include <linux/mfd/ti_am335x_tscadc.h> struct tiadc_device { struct ti_tscadc_dev *mfd_tscadc; int channels; + struct iio_map *map; }; static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) @@ -84,34 +87,76 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en); } +static const char * const chan_name_ain[] = { + "AIN0", + "AIN1", + "AIN2", + "AIN3", + "AIN4", + "AIN5", + "AIN6", + "AIN7", +}; + static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) { + struct tiadc_device *adc_dev = iio_priv(indio_dev); struct iio_chan_spec *chan_array; - int i; + struct iio_chan_spec *chan; + int i, ret; indio_dev->num_channels = channels; - chan_array = kcalloc(indio_dev->num_channels, + chan_array = kcalloc(channels, sizeof(struct iio_chan_spec), GFP_KERNEL); - if (chan_array == NULL) return -ENOMEM; - for (i = 0; i < (indio_dev->num_channels); i++) { - struct iio_chan_spec *chan = chan_array + i; + chan = chan_array; + for (i = 0; i < channels; i++, chan++) { + chan->type = IIO_VOLTAGE; chan->indexed = 1; chan->channel = i; chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW); + chan->datasheet_name = chan_name_ain[i]; + chan->scan_type.sign = 'u'; + chan->scan_type.realbits = 12; + chan->scan_type.storagebits = 32; + chan->scan_type.shift = 0; } indio_dev->channels = chan_array; - return indio_dev->num_channels; + adc_dev->map = kcalloc(channels + 1, sizeof(struct iio_map), + GFP_KERNEL); + if (adc_dev->map == NULL) + goto err_free_chan; + + for (i = 0; i < channels; i++) { + adc_dev->map[i].adc_channel_label = + chan_array[i].datasheet_name; + adc_dev->map[i].consumer_dev_name = "any"; + adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name; + } + + ret = iio_map_array_register(indio_dev, adc_dev->map); + if (ret) + goto err_map; + + return 0; +err_map: + kfree(adc_dev->map); +err_free_chan: + kfree(chan_array); + return -ENOMEM; } static void tiadc_channels_remove(struct iio_dev *indio_dev) { + struct tiadc_device *adc_dev = iio_priv(indio_dev); + kfree(indio_dev->channels); + kfree(adc_dev->map); } static int tiadc_read_raw(struct iio_dev *indio_dev,