| Message ID | 1351783496-11557-1-git-send-email-panto@antoniou-consulting.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/01/2012 04:24 PM, Pantelis Antoniou wrote: > Add an IIO map interface that consumers can use. Hi, Looks like you overlooked the review comments I had inline last time. I've put them in again, see below. > > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++-------- > 1 file changed, 49 insertions(+), 11 deletions(-) > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index 02a43c8..8595a90 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -20,8 +20,9 @@ > #include <linux/slab.h> > #include <linux/interrupt.h> > #include <linux/platform_device.h> > -#include <linux/io.h> > #include <linux/iio/iio.h> > +#include <linux/iio/machine.h> > +#include <linux/iio/driver.h> > > #include <linux/mfd/ti_am335x_tscadc.h> > #include <linux/platform_data/ti_am335x_adc.h> > @@ -29,6 +30,8 @@ > struct tiadc_device { > struct ti_tscadc_dev *mfd_tscadc; > int channels; > + char *buf; buf doesn't seem to be used anywhere > + struct iio_map *map; > }; > > static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) > @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) > tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); > } > > -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) > +static int tiadc_channel_init(struct iio_dev *indio_dev, > + struct tiadc_device *adc_dev) > { > struct iio_chan_spec *chan_array; > - int i; > - > - indio_dev->num_channels = channels; > - chan_array = kcalloc(indio_dev->num_channels, > - sizeof(struct iio_chan_spec), GFP_KERNEL); > + struct iio_chan_spec *chan; > + char *s; > + int i, len, size, ret; > + int channels = adc_dev->channels; > > + size = channels * (sizeof(struct iio_chan_spec) + 6); > + chan_array = kzalloc(size, 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; > + /* buffer space is after the array */ > + s = (char *)(chan_array + channels); > + chan = chan_array; > + for (i = 0; i < channels; i++, chan++, s += len + 1) { > + > + len = sprintf(s, "AIN%d", i); > + > chan->type = IIO_VOLTAGE; > chan->indexed = 1; > chan->channel = i; > - chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT; > + chan->datasheet_name = s; > + chan->scan_type.sign = 'u'; > + chan->scan_type.realbits = 12; > + chan->scan_type.storagebits = 32; > + chan->scan_type.shift = 0; The scan type assignment should go in a separate patch if possible. > } > > indio_dev->channels = chan_array; > + indio_dev->num_channels = channels; > + > + size = (channels + 1) * sizeof(struct iio_map); > + adc_dev->map = kzalloc(size, GFP_KERNEL); > + if (adc_dev->map == NULL) { > + kfree(chan_array); > + return -ENOMEM; > + } > + > + for (i = 0; i < indio_dev->num_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; > + } > + adc_dev->map[i].adc_channel_label = NULL; > + adc_dev->map[i].consumer_dev_name = NULL; > + adc_dev->map[i].consumer_channel = NULL; The map should be passed in via platform data or similar. All the fields of the map depend on the specific user, so you can't use a generic map. In fact if we were able to use a generic map, we wouldn't need a map at all. > + > + ret = iio_map_array_register(indio_dev, adc_dev->map); > + if (ret != 0) { > + kfree(adc_dev->map); > + kfree(chan_array); > + return -ENOMEM; > + } > > return indio_dev->num_channels; > } > @@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device *pdev) > > tiadc_step_config(adc_dev); > > - err = tiadc_channel_init(indio_dev, adc_dev->channels); > + err = tiadc_channel_init(indio_dev, adc_dev); > if (err < 0) > goto err_free_device; > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 31, 2012, at 7:52 PM, Lars-Peter Clausen wrote: > On 11/01/2012 04:24 PM, Pantelis Antoniou wrote: >> Add an IIO map interface that consumers can use. > > Hi, > > Looks like you overlooked the review comments I had inline last time. I've > put them in again, see below. > >> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> >> --- >> drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++-------- >> 1 file changed, 49 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c >> index 02a43c8..8595a90 100644 >> --- a/drivers/iio/adc/ti_am335x_adc.c >> +++ b/drivers/iio/adc/ti_am335x_adc.c >> @@ -20,8 +20,9 @@ >> #include <linux/slab.h> >> #include <linux/interrupt.h> >> #include <linux/platform_device.h> >> -#include <linux/io.h> >> #include <linux/iio/iio.h> >> +#include <linux/iio/machine.h> >> +#include <linux/iio/driver.h> >> >> #include <linux/mfd/ti_am335x_tscadc.h> >> #include <linux/platform_data/ti_am335x_adc.h> >> @@ -29,6 +30,8 @@ >> struct tiadc_device { >> struct ti_tscadc_dev *mfd_tscadc; >> int channels; >> + char *buf; > > buf doesn't seem to be used anywhere > Duh >> + struct iio_map *map; >> }; >> >> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) >> @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) >> tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); >> } >> >> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) >> +static int tiadc_channel_init(struct iio_dev *indio_dev, >> + struct tiadc_device *adc_dev) >> { >> struct iio_chan_spec *chan_array; >> - int i; >> - >> - indio_dev->num_channels = channels; >> - chan_array = kcalloc(indio_dev->num_channels, >> - sizeof(struct iio_chan_spec), GFP_KERNEL); >> + struct iio_chan_spec *chan; >> + char *s; >> + int i, len, size, ret; >> + int channels = adc_dev->channels; >> >> + size = channels * (sizeof(struct iio_chan_spec) + 6); >> + chan_array = kzalloc(size, 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; >> + /* buffer space is after the array */ >> + s = (char *)(chan_array + channels); >> + chan = chan_array; >> + for (i = 0; i < channels; i++, chan++, s += len + 1) { >> + >> + len = sprintf(s, "AIN%d", i); >> + >> chan->type = IIO_VOLTAGE; >> chan->indexed = 1; >> chan->channel = i; >> - chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT; >> + chan->datasheet_name = s; >> + chan->scan_type.sign = 'u'; >> + chan->scan_type.realbits = 12; >> + chan->scan_type.storagebits = 32; >> + chan->scan_type.shift = 0; > > The scan type assignment should go in a separate patch if possible. ok. > >> } >> >> indio_dev->channels = chan_array; >> + indio_dev->num_channels = channels; >> + >> + size = (channels + 1) * sizeof(struct iio_map); >> + adc_dev->map = kzalloc(size, GFP_KERNEL); >> + if (adc_dev->map == NULL) { >> + kfree(chan_array); >> + return -ENOMEM; >> + } >> + >> + for (i = 0; i < indio_dev->num_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; >> + } >> + adc_dev->map[i].adc_channel_label = NULL; >> + adc_dev->map[i].consumer_dev_name = NULL; >> + adc_dev->map[i].consumer_channel = NULL; > > The map should be passed in via platform data or similar. All the fields of > the map depend on the specific user, so you can't use a generic map. In fact > if we were able to use a generic map, we wouldn't need a map at all. There's no platform data in the board I'm using. It's board-generic using device tree only. > >> + >> + ret = iio_map_array_register(indio_dev, adc_dev->map); >> + if (ret != 0) { >> + kfree(adc_dev->map); >> + kfree(chan_array); >> + return -ENOMEM; >> + } >> >> return indio_dev->num_channels; >> } >> @@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device *pdev) >> >> tiadc_step_config(adc_dev); >> >> - err = tiadc_channel_init(indio_dev, adc_dev->channels); >> + err = tiadc_channel_init(indio_dev, adc_dev); >> if (err < 0) >> goto err_free_device; >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/31/2012 06:55 PM, Pantelis Antoniou wrote: > [...] >>> } >>> >>> indio_dev->channels = chan_array; >>> + indio_dev->num_channels = channels; >>> + >>> + size = (channels + 1) * sizeof(struct iio_map); >>> + adc_dev->map = kzalloc(size, GFP_KERNEL); >>> + if (adc_dev->map == NULL) { >>> + kfree(chan_array); >>> + return -ENOMEM; >>> + } >>> + >>> + for (i = 0; i < indio_dev->num_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; >>> + } >>> + adc_dev->map[i].adc_channel_label = NULL; >>> + adc_dev->map[i].consumer_dev_name = NULL; >>> + adc_dev->map[i].consumer_channel = NULL; >> >> The map should be passed in via platform data or similar. All the fields of >> the map depend on the specific user, so you can't use a generic map. In fact >> if we were able to use a generic map, we wouldn't need a map at all. > > There's no platform data in the board I'm using. It's board-generic using > device tree only. That's the 'or similar' ;) Unfortunately we do not have a device tree binding for IIO yet. But I think we should aim at a interface similar like we have in other subsystems like the clk, regulator or dma framework. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote: > On 10/31/2012 06:55 PM, Pantelis Antoniou wrote: >> [...] >>>> } >>>> >>>> indio_dev->channels = chan_array; >>>> + indio_dev->num_channels = channels; >>>> + >>>> + size = (channels + 1) * sizeof(struct iio_map); >>>> + adc_dev->map = kzalloc(size, GFP_KERNEL); >>>> + if (adc_dev->map == NULL) { >>>> + kfree(chan_array); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + for (i = 0; i < indio_dev->num_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; >>>> + } >>>> + adc_dev->map[i].adc_channel_label = NULL; >>>> + adc_dev->map[i].consumer_dev_name = NULL; >>>> + adc_dev->map[i].consumer_channel = NULL; >>> >>> The map should be passed in via platform data or similar. All the fields of >>> the map depend on the specific user, so you can't use a generic map. In fact >>> if we were able to use a generic map, we wouldn't need a map at all. >> >> There's no platform data in the board I'm using. It's board-generic using >> device tree only. > > That's the 'or similar' ;) Unfortunately we do not have a device tree > binding for IIO yet. But I think we should aim at a interface similar like > we have in other subsystems like the clk, regulator or dma framework. > > - Lars So in the meantime no-one can use IIO ADC in any OF only platform. In the meantime, this is pretty reasonable IMO. This is only for a specific board with known channel mappings. I'm not out to fix IIO, I'm out to fix a single board. Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 31, 2012, at 1:55 PM, Pantelis Antoniou wrote: > > On Oct 31, 2012, at 7:52 PM, Lars-Peter Clausen wrote: > >> On 11/01/2012 04:24 PM, Pantelis Antoniou wrote: >>> Add an IIO map interface that consumers can use. >> >> Hi, >> >> Looks like you overlooked the review comments I had inline last time. I've >> put them in again, see below. >> >>> >>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> >>> --- >>> drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++-------- >>> 1 file changed, 49 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c >>> index 02a43c8..8595a90 100644 >>> --- a/drivers/iio/adc/ti_am335x_adc.c >>> +++ b/drivers/iio/adc/ti_am335x_adc.c >>> @@ -20,8 +20,9 @@ >>> #include <linux/slab.h> >>> #include <linux/interrupt.h> >>> #include <linux/platform_device.h> >>> -#include <linux/io.h> >>> #include <linux/iio/iio.h> >>> +#include <linux/iio/machine.h> >>> +#include <linux/iio/driver.h> >>> >>> #include <linux/mfd/ti_am335x_tscadc.h> >>> #include <linux/platform_data/ti_am335x_adc.h> >>> @@ -29,6 +30,8 @@ >>> struct tiadc_device { >>> struct ti_tscadc_dev *mfd_tscadc; >>> int channels; >>> + char *buf; >> >> buf doesn't seem to be used anywhere >> > > Duh > >>> + struct iio_map *map; >>> }; >>> >>> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) >>> @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) >>> tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); >>> } >>> >>> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) >>> +static int tiadc_channel_init(struct iio_dev *indio_dev, >>> + struct tiadc_device *adc_dev) >>> { >>> struct iio_chan_spec *chan_array; >>> - int i; >>> - >>> - indio_dev->num_channels = channels; >>> - chan_array = kcalloc(indio_dev->num_channels, >>> - sizeof(struct iio_chan_spec), GFP_KERNEL); >>> + struct iio_chan_spec *chan; >>> + char *s; >>> + int i, len, size, ret; >>> + int channels = adc_dev->channels; >>> >>> + size = channels * (sizeof(struct iio_chan_spec) + 6); >>> + chan_array = kzalloc(size, 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; >>> + /* buffer space is after the array */ >>> + s = (char *)(chan_array + channels); >>> + chan = chan_array; >>> + for (i = 0; i < channels; i++, chan++, s += len + 1) { >>> + >>> + len = sprintf(s, "AIN%d", i); >>> + >>> chan->type = IIO_VOLTAGE; >>> chan->indexed = 1; >>> chan->channel = i; >>> - chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT; >>> + chan->datasheet_name = s; >>> + chan->scan_type.sign = 'u'; >>> + chan->scan_type.realbits = 12; >>> + chan->scan_type.storagebits = 32; >>> + chan->scan_type.shift = 0; >> >> The scan type assignment should go in a separate patch if possible. > > ok. > >> >>> } >>> >>> indio_dev->channels = chan_array; >>> + indio_dev->num_channels = channels; >>> + >>> + size = (channels + 1) * sizeof(struct iio_map); >>> + adc_dev->map = kzalloc(size, GFP_KERNEL); >>> + if (adc_dev->map == NULL) { >>> + kfree(chan_array); >>> + return -ENOMEM; >>> + } >>> + >>> + for (i = 0; i < indio_dev->num_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; >>> + } >>> + adc_dev->map[i].adc_channel_label = NULL; >>> + adc_dev->map[i].consumer_dev_name = NULL; >>> + adc_dev->map[i].consumer_channel = NULL; >> >> The map should be passed in via platform data or similar. All the fields of >> the map depend on the specific user, so you can't use a generic map. In fact >> if we were able to use a generic map, we wouldn't need a map at all. > > There's no platform data in the board I'm using. It's board-generic using > device tree only. How about a DT binding for the map? -Matt-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/31/2012 07:12 PM, Pantelis Antoniou wrote: > > On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote: > >> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote: >>> [...] >>>>> } >>>>> >>>>> indio_dev->channels = chan_array; >>>>> + indio_dev->num_channels = channels; >>>>> + >>>>> + size = (channels + 1) * sizeof(struct iio_map); >>>>> + adc_dev->map = kzalloc(size, GFP_KERNEL); >>>>> + if (adc_dev->map == NULL) { >>>>> + kfree(chan_array); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + for (i = 0; i < indio_dev->num_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; >>>>> + } >>>>> + adc_dev->map[i].adc_channel_label = NULL; >>>>> + adc_dev->map[i].consumer_dev_name = NULL; >>>>> + adc_dev->map[i].consumer_channel = NULL; >>>> >>>> The map should be passed in via platform data or similar. All the fields of >>>> the map depend on the specific user, so you can't use a generic map. In fact >>>> if we were able to use a generic map, we wouldn't need a map at all. >>> >>> There's no platform data in the board I'm using. It's board-generic using >>> device tree only. >> >> That's the 'or similar' ;) Unfortunately we do not have a device tree >> binding for IIO yet. But I think we should aim at a interface similar like >> we have in other subsystems like the clk, regulator or dma framework. >> >> - Lars > > So in the meantime no-one can use IIO ADC in any OF only platform. Yes, nobody can use it until somebody implements it. So far nobody needed it, so that's why it hasn't been implemented yet. The whole in kernel consumer API for IIO is still very young and only a very few drivers support it yet. > > In the meantime, this is pretty reasonable IMO. This is only for a specific > board with known channel mappings. Unfortunately it is not. It is adding a device specific hack to a generic driver and it is also completely misusing the API. > > I'm not out to fix IIO, I'm out to fix a single board. > It's not about fixing IIO, it's about extending IIO to be able to serve your needs. See, the issue is if everybody would work around the lack of DT bindings we'll never have DT bindings for IIO, so the right thing to do is to implement them instead of working around the lack of. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 31, 2012, at 8:36 PM, Lars-Peter Clausen wrote: > On 10/31/2012 07:12 PM, Pantelis Antoniou wrote: >> >> On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote: >> >>> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote: >>>> [...] >>>>>> } >>>>>> >>>>>> indio_dev->channels = chan_array; >>>>>> + indio_dev->num_channels = channels; >>>>>> + >>>>>> + size = (channels + 1) * sizeof(struct iio_map); >>>>>> + adc_dev->map = kzalloc(size, GFP_KERNEL); >>>>>> + if (adc_dev->map == NULL) { >>>>>> + kfree(chan_array); >>>>>> + return -ENOMEM; >>>>>> + } >>>>>> + >>>>>> + for (i = 0; i < indio_dev->num_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; >>>>>> + } >>>>>> + adc_dev->map[i].adc_channel_label = NULL; >>>>>> + adc_dev->map[i].consumer_dev_name = NULL; >>>>>> + adc_dev->map[i].consumer_channel = NULL; >>>>> >>>>> The map should be passed in via platform data or similar. All the fields of >>>>> the map depend on the specific user, so you can't use a generic map. In fact >>>>> if we were able to use a generic map, we wouldn't need a map at all. >>>> >>>> There's no platform data in the board I'm using. It's board-generic using >>>> device tree only. >>> >>> That's the 'or similar' ;) Unfortunately we do not have a device tree >>> binding for IIO yet. But I think we should aim at a interface similar like >>> we have in other subsystems like the clk, regulator or dma framework. >>> >>> - Lars >> >> So in the meantime no-one can use IIO ADC in any OF only platform. > > Yes, nobody can use it until somebody implements it. So far nobody needed > it, so that's why it hasn't been implemented yet. The whole in kernel > consumer API for IIO is still very young and only a very few drivers support > it yet. > >> >> In the meantime, this is pretty reasonable IMO. This is only for a specific >> board with known channel mappings. > > Unfortunately it is not. It is adding a device specific hack to a generic > driver and it is also completely misusing the API. > >> >> I'm not out to fix IIO, I'm out to fix a single board. >> > > It's not about fixing IIO, it's about extending IIO to be able to serve your > needs. See, the issue is if everybody would work around the lack of DT > bindings we'll never have DT bindings for IIO, so the right thing to do is > to implement them instead of working around the lack of. > > - Lars OK, OK, I see the point. It's just that I'm under the gun for more pressing matters ATM. Can at least get a small write-up about how the bindings should look like? There's absolutely nothing, not even a hint of one out there in the intertubes, on how the channel mapping should look like. Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/31/2012 07:43 PM, Pantelis Antoniou wrote: > > On Oct 31, 2012, at 8:36 PM, Lars-Peter Clausen wrote: > >> On 10/31/2012 07:12 PM, Pantelis Antoniou wrote: >>> >>> On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote: >>> >>>> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote: >>>>> [...] >>>>>>> } >>>>>>> >>>>>>> indio_dev->channels = chan_array; >>>>>>> + indio_dev->num_channels = channels; >>>>>>> + >>>>>>> + size = (channels + 1) * sizeof(struct iio_map); >>>>>>> + adc_dev->map = kzalloc(size, GFP_KERNEL); >>>>>>> + if (adc_dev->map == NULL) { >>>>>>> + kfree(chan_array); >>>>>>> + return -ENOMEM; >>>>>>> + } >>>>>>> + >>>>>>> + for (i = 0; i < indio_dev->num_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; >>>>>>> + } >>>>>>> + adc_dev->map[i].adc_channel_label = NULL; >>>>>>> + adc_dev->map[i].consumer_dev_name = NULL; >>>>>>> + adc_dev->map[i].consumer_channel = NULL; >>>>>> >>>>>> The map should be passed in via platform data or similar. All the fields of >>>>>> the map depend on the specific user, so you can't use a generic map. In fact >>>>>> if we were able to use a generic map, we wouldn't need a map at all. >>>>> >>>>> There's no platform data in the board I'm using. It's board-generic using >>>>> device tree only. >>>> >>>> That's the 'or similar' ;) Unfortunately we do not have a device tree >>>> binding for IIO yet. But I think we should aim at a interface similar like >>>> we have in other subsystems like the clk, regulator or dma framework. >>>> >>>> - Lars >>> >>> So in the meantime no-one can use IIO ADC in any OF only platform. >> >> Yes, nobody can use it until somebody implements it. So far nobody needed >> it, so that's why it hasn't been implemented yet. The whole in kernel >> consumer API for IIO is still very young and only a very few drivers support >> it yet. >> >>> >>> In the meantime, this is pretty reasonable IMO. This is only for a specific >>> board with known channel mappings. >> >> Unfortunately it is not. It is adding a device specific hack to a generic >> driver and it is also completely misusing the API. >> >>> >>> I'm not out to fix IIO, I'm out to fix a single board. >>> >> >> It's not about fixing IIO, it's about extending IIO to be able to serve your >> needs. See, the issue is if everybody would work around the lack of DT >> bindings we'll never have DT bindings for IIO, so the right thing to do is >> to implement them instead of working around the lack of. >> >> - Lars > > OK, OK, > ok :) > I see the point. It's just that I'm under the gun for more pressing matters ATM. > Can at least get a small write-up about how the bindings should look like? > > There's absolutely nothing, not even a hint of one out there in the intertubes, > on how the channel mapping should look like. Again, that's because nobody probably has given this much though yet. As I said the in-kernel consumer framework is still young and so far only a tiny fraction of the drivers support it. If you want to I can try to give this some though and come up with a small proof of concept, but this would have to wait until next week, since I have a few other things on my desk as well. I think a good start might be to take a closer look at the clk dt bindings (Documentation/devicetree/bindings/clock/clock-bindings.txt). - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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 02a43c8..8595a90 100644 --- a/drivers/iio/adc/ti_am335x_adc.c +++ b/drivers/iio/adc/ti_am335x_adc.c @@ -20,8 +20,9 @@ #include <linux/slab.h> #include <linux/interrupt.h> #include <linux/platform_device.h> -#include <linux/io.h> #include <linux/iio/iio.h> +#include <linux/iio/machine.h> +#include <linux/iio/driver.h> #include <linux/mfd/ti_am335x_tscadc.h> #include <linux/platform_data/ti_am335x_adc.h> @@ -29,6 +30,8 @@ struct tiadc_device { struct ti_tscadc_dev *mfd_tscadc; int channels; + char *buf; + struct iio_map *map; }; static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); } -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) +static int tiadc_channel_init(struct iio_dev *indio_dev, + struct tiadc_device *adc_dev) { struct iio_chan_spec *chan_array; - int i; - - indio_dev->num_channels = channels; - chan_array = kcalloc(indio_dev->num_channels, - sizeof(struct iio_chan_spec), GFP_KERNEL); + struct iio_chan_spec *chan; + char *s; + int i, len, size, ret; + int channels = adc_dev->channels; + size = channels * (sizeof(struct iio_chan_spec) + 6); + chan_array = kzalloc(size, 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; + /* buffer space is after the array */ + s = (char *)(chan_array + channels); + chan = chan_array; + for (i = 0; i < channels; i++, chan++, s += len + 1) { + + len = sprintf(s, "AIN%d", i); + chan->type = IIO_VOLTAGE; chan->indexed = 1; chan->channel = i; - chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT; + chan->datasheet_name = s; + 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; + indio_dev->num_channels = channels; + + size = (channels + 1) * sizeof(struct iio_map); + adc_dev->map = kzalloc(size, GFP_KERNEL); + if (adc_dev->map == NULL) { + kfree(chan_array); + return -ENOMEM; + } + + for (i = 0; i < indio_dev->num_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; + } + adc_dev->map[i].adc_channel_label = NULL; + adc_dev->map[i].consumer_dev_name = NULL; + adc_dev->map[i].consumer_channel = NULL; + + ret = iio_map_array_register(indio_dev, adc_dev->map); + if (ret != 0) { + kfree(adc_dev->map); + kfree(chan_array); + return -ENOMEM; + } return indio_dev->num_channels; } @@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device *pdev) tiadc_step_config(adc_dev); - err = tiadc_channel_init(indio_dev, adc_dev->channels); + err = tiadc_channel_init(indio_dev, adc_dev); if (err < 0) goto err_free_device;
Add an IIO map interface that consumers can use. Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> --- drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 11 deletions(-)