[RFC v2 7/7] IIO: ADC: add stm32 DFSDM support

[Arnaud Pouliquen]

Hello Jonathan,

Late answer... sorry for this.

I will integrate your remark in V2.
Please find my answers in-line

Regards
Arnaud

On 02/19/2017 03:46 PM, Jonathan Cameron wrote:
> On 13/02/17 16:38, Arnaud Pouliquen wrote:
>> Add driver for stm32 DFSDM IP. This IP converts a sigma delta stream in
>> n bit samples through a low pass filter and an integrator.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen at st.com>
> I think this fits together rather nicely.  As before, various comments that
> are irrelevant to an RFC (I just couldn't stop myself ;) and a few more
> relevant ones.
> 
> So as far as I'm concerned: Looking forward to the full version!
> (as long as Mark and others are happy of course)
> 
> I definitely want to ultimately see buffered and dma support on the
> ADC driver side of things as well but that can come later.
Ok, I suppose that DMA in cyclic mode should also be required for some
other IIO driver...
> 
> Jonathan
>> ---
>>  drivers/iio/adc/Kconfig            |  13 +
>>  drivers/iio/adc/Makefile           |   1 +
>>  drivers/iio/adc/stm32-dfsdm-adc.c  | 483 +++++++++++++++++++++++++++++++++++++
>>  drivers/iio/adc/stm32-dfsdm-core.c | 273 +++++++++++++++++++++
>>  drivers/iio/adc/stm32-dfsdm.h      | 141 +++++++++++
>>  5 files changed, 911 insertions(+)
>>  create mode 100644 drivers/iio/adc/stm32-dfsdm-adc.c
>>  create mode 100644 drivers/iio/adc/stm32-dfsdm-core.c
>>  create mode 100644 drivers/iio/adc/stm32-dfsdm.h
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index d4366ac..ab917b6 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -452,6 +452,19 @@ config STM32_ADC
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called stm32-adc.
>>  
>> +config STM32_DFSDM_ADC
>> +	tristate "STMicroelectronics STM32 dfsdm adc"
>> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>> +	select REGMAP
>> +	select REGMAP_MMIO
>> +	select IIO_HW_CONSUMER
>> +	help
>> +	  Select this option to enable the  driver for STMicroelectronics
>> +	  STM32 digital filter for sigma delta converter (ADC).
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called stm32-adc-dfsdm-adc.
>> +
>>  config STX104
>>  	tristate "Apex Embedded Systems STX104 driver"
>>  	depends on X86 && ISA_BUS_API
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index bd67144..5bcad23 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>  obj-$(CONFIG_STX104) += stx104.o
>>  obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
>>  obj-$(CONFIG_STM32_ADC) += stm32-adc.o
>> +obj-$(CONFIG_STM32_DFSDM_ADC) += stm32-dfsdm-adc.o stm32-dfsdm-core.o
>>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>> new file mode 100644
>> index 0000000..8f9c3263
>> --- /dev/null
>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
>> @@ -0,0 +1,483 @@
>> +/*
>> + * This file is part of STM32 DFSDM ADC driver
>> + *
>> + * Copyright (C) 2016, STMicroelectronics - All Rights Reserved
>> + * Author: Arnaud Pouliquen <arnaud.pouliquen at st.com>.
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/iio/hw_consumer.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#include <sound/stm32-adfsdm.h>
>> +
>> +#include "stm32-dfsdm.h"
>> +
>> +enum stm32_dfsdm_mode {
>> +	DFSDM_ADC, /* ADC mode, access through IIO ABI */
>> +	DFSDM_AUDIO /* Audio mode, access through ASoC ABI */
>> +};
>> +
>> +struct stm32_dfsdm_adc {
>> +	struct stm32_dfsdm *common;
>> +
>> +	unsigned int fl_id;
>> +	unsigned int oversamp;
>> +	unsigned int clk_freq;
>> +
>> +	enum stm32_dfsdm_mode mode;
>> +	struct platform_device *audio_pdev;
>> +
>> +	void (*overrun_cb)(void *context);
>> +	void *cb_context;
>> +
>> +	/* Hardware consumer structure for Front End iio */
> IIO
>> +	struct iio_hw_consumer *hwc;
>> +};
>> +
>> +static const enum stm32_dfsdm_mode stm32_dfsdm_data_adc = DFSDM_ADC;
>> +static const enum stm32_dfsdm_mode stm32_dfsdm_data_audio = DFSDM_AUDIO;
>> +
>> +struct stm32_dfsdm_adc_devdata {
>> +	enum stm32_dfsdm_mode mode;
>> +	const struct iio_info *info;
>> +};
>> +
>> +static int stm32_dfsdm_set_osrs(struct stm32_dfsdm_adc *adc, bool fast,
>> +				unsigned int oversamp)
>> +{
>> +	/*
>> +	 * TODO
>> +	 * This function tries to compute filter oversampling and integrator
>> +	 * oversampling, base on oversampling ratio requested by user.
>> +	 */
>> +
>> +	return 0;
>> +};
>> +
>> +static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
>> +				   const struct iio_chan_spec *chan, int *res)
>> +{
>> +	/* TODO: Perform conversion instead of sending fake value */
>> +	dev_dbg(&indio_dev->dev, "%s\n", __func__);
> :( Definitely an RFC
>> +
>> +	*res = chan->channel + 0xFFFF00;
>> +	return 0;
>> +}
>> +
>> +static int stm32_dfsdm_write_raw(struct iio_dev *indio_dev,
>> +				 struct iio_chan_spec const *chan,
>> +				 int val, int val2, long mask)
>> +{
>> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		ret = stm32_dfsdm_set_osrs(adc, 0, val);
>> +		if (!ret)
>> +			adc->oversamp = val;
> If no reason to carry on,(i.e. nothing to unwind) return directly from within
> the switch statement.
>> +		break;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		if (adc->mode == DFSDM_AUDIO)
>> +			ret = stm32_dfsdm_set_osrs(adc, 0, val);
>> +		else
>> +			ret = -EINVAL;
>> +		break;
>> +
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>> +				struct iio_chan_spec const *chan, int *val,
>> +				int *val2, long mask)
>> +{
>> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	dev_dbg(&indio_dev->dev, "%s\n", __func__);
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (adc->hwc) {
>> +			ret = iio_hw_consumer_enable(adc->hwc);
>> +			if (ret < 0) {
>> +				dev_err(&indio_dev->dev,
>> +					"%s: iio enable failed (channel %d)\n",
>> +					__func__, chan->channel);
>> +				return ret;
>> +			}
> Not an error if hwc not available?
ASoC framework requests to handle a codec driver. So in case of PDM
usecase, no IIO SD modulator device is used, instead an ASoC generic
dmic-codec device is probed. In other words the link between the DFSDM
and the external SD-modulator has to be done in ASOC for PDMs and in
IIO for ADCs.

>> +		}
>> +		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
>> +		if (ret < 0) {
>> +			dev_err(&indio_dev->dev,
>> +				"%s: conversion failed (channel %d)\n",
>> +				__func__, chan->channel);
>> +			return ret;
>> +		}
>> +
>> +		if (adc->hwc)
>> +			iio_hw_consumer_disable(adc->hwc);
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		*val = adc->oversamp;
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = DIV_ROUND_CLOSEST(adc->clk_freq, adc->oversamp);
>> +
>> +		return IIO_VAL_INT;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info stm32_dfsdm_info_adc = {
>> +	.read_raw = stm32_dfsdm_read_raw,
>> +	.write_raw = stm32_dfsdm_write_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static const struct iio_info stm32_dfsdm_info_audio = {
>> +	.read_raw = stm32_dfsdm_read_raw,
>> +	.write_raw = stm32_dfsdm_write_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
> Hohum. These two are the same, why two copies?  Mind you for the audio
> you can't write or read anything so you could drop the callbacks.
yes to rework.
>> +
>> +const struct stm32_dfsdm_adc_devdata stm32_dfsdm_devdata_adc = {
>> +	.mode = DFSDM_ADC,
>> +	.info = &stm32_dfsdm_info_adc,
>> +};
>> +
>> +const struct stm32_dfsdm_adc_devdata stm32_dfsdm_devdata_audio = {
>> +	.mode = DFSDM_AUDIO,
>> +	.info = &stm32_dfsdm_info_audio,
>> +};
>> +
>> +static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
>> +{
>> +	/* TODO */
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void stm32_dfsdm_set_sysclk(struct stm32_dfsdm_adc *adc,
>> +				   unsigned int freq)
>> +{
>> +	struct iio_dev *iio = iio_priv_to_dev(adc);
>> +
>> +	dev_dbg(&iio->dev, "%s:\n", __func__);
>> +
>> +	adc->clk_freq = freq;
>> +};
>> +
>> +	/* Set expected audio sampling rate */
>> +static int stm32_dfsdm_set_hwparam(struct stm32_dfsdm_adc *adc,
>> +				   struct stm32_dfsdm_hw_param *params)
>> +{
>> +	struct iio_dev *iio = iio_priv_to_dev(adc);
>> +
>> +	dev_dbg(&iio->dev, "%s for rate %d\n", __func__, params->rate);
>> +
>> +	return stm32_dfsdm_set_osrs(adc, 0, params->rate);
>> +};
>> +
>> +	/* Called when ASoC starts an audio stream setup. */
>> +static int stm32_dfsdm_audio_startup(struct stm32_dfsdm_adc *adc)
>> +{
>> +	struct iio_dev *iio = iio_priv_to_dev(adc);
>> +
>> +	dev_dbg(&iio->dev, "%s\n", __func__);
>> +
>> +	return 0;
>> +};
>> +
>> +	/* Shuts down the audio stream. */
> Odd indenting.
>> +static void stm32_dfsdm_audio_shutdown(struct stm32_dfsdm_adc *adc)
>> +{
>> +	struct iio_dev *iio = iio_priv_to_dev(adc);
>> +
>> +	dev_dbg(&iio->dev, "%s\n", __func__);
>> +};
>> +
>> +	/*
>> +	 * Provides DMA source physicla addr to allow ALsa to handle DMA
>> +	 * transfers.
> physical - please run a spell checker over the comments.
>> +	 */
>> +static dma_addr_t stm32_dfsdm_get_dma_source(struct stm32_dfsdm_adc *adc)
>> +{
>> +	struct iio_dev *iio = iio_priv_to_dev(adc);
>> +
>> +	dev_dbg(&iio->dev, "%s\n", __func__);
>> +
>> +	return (dma_addr_t)(adc->common->phys_base + DFSDM_RDATAR(adc->fl_id));
>> +};
>> +
>> +/* Register callback to treat underrun and overrun issues */
>> +static void stm32_dfsdm_register_xrun_cb(struct stm32_dfsdm_adc *adc,
>> +					 void (*overrun_cb)(void *context),
>> +					 void *context)
>> +{
>> +	struct iio_dev *iio = iio_priv_to_dev(adc);
>> +
>> +	dev_dbg(&iio->dev, "%s\n", __func__);
>> +	adc->overrun_cb = overrun_cb;
>> +	adc->cb_context = context;
>> +};
>> +
>> +const struct stm32_adfsdm_codec_ops stm32_dfsdm_audio_ops = {
>> +	.set_sysclk = stm32_dfsdm_set_sysclk,
>> +	.set_hwparam = stm32_dfsdm_set_hwparam,
>> +	.audio_startup = stm32_dfsdm_audio_startup,
>> +	.audio_shutdown = stm32_dfsdm_audio_shutdown,
>> +	.register_xrun_cb = stm32_dfsdm_register_xrun_cb,
>> +	.get_dma_source = stm32_dfsdm_get_dma_source
>> +};
> Hmm. I'm wondering if it might make sense to farm the audio stuff off
> to a separate file.  Potentially we'll have systems that are built with
> no audio support at all, so would be odd to have this stuff still provided.
Ok, this make sense if the API become generic.
> 
> What do you think?  Also provides an obvious clean bit to be Mark's problem
> (even if it's in the IIO directory ;)
I can't answer instead of Mark,( and perhaps i misunderstood...) but
regarding HDMI story (drivers shared between ASoC and DRM), not sure
that Mark accepts to have an ASoC drivers in IIO directory. So need a
part in ASoC that is customer of IIO driver...

>> +
>> +static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>> +					 struct iio_chan_spec *chan,
>> +					 int chan_idx)
>> +{
>> +	struct iio_chan_spec *ch = &chan[chan_idx];
>> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	dev_dbg(&indio_dev->dev, "%s:\n", __func__);
>> +	ret = of_property_read_u32_index(indio_dev->dev.of_node,
>> +					 "st,adc-channels", chan_idx,
>> +					 &ch->channel);
>> +	if (ret < 0) {
>> +		dev_err(&indio_dev->dev,
>> +			" error parsing 'st,adc-channels' for idx %d\n",
>> +			chan_idx);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_string_index(indio_dev->dev.of_node,
>> +					    "st,adc-channel-names", chan_idx,
>> +					    &ch->datasheet_name);
>> +	if (ret < 0) {
>> +		dev_err(&indio_dev->dev,
>> +			" error parsing 'st,adc-channel-names' for idx %d\n",
>> +			chan_idx);
>> +		return ret;
>> +	}
>> +
>> +	ch->type = IIO_VOLTAGE;
>> +	ch->indexed = 1;
>> +	ch->scan_index = chan_idx;
>> +	if (adc->mode == DFSDM_ADC) {
>> +		/*
>> +		 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>> +		 * IIO_CHAN_INFO_SAMP_FREQ: used to indicate sampling frequency
>> +		 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used set oversampling
>> +		 */
>> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +					 BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> +					 BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> Very nice.  I was just thinking you should do this before I got here.
> Channels with no properties but still with an existence from the point of
> view of consumers using the buffered interface.  Potentially you 'could'
> provide some of the info as read only but why bother...
>> +	}
>> +
>> +	ch->scan_type.sign = 'u';
>> +	ch->scan_type.realbits = 24;
>> +	ch->scan_type.storagebits = 32;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dfsdm_adc_chan_init(struct iio_dev *indio_dev)
>> +{
>> +	struct iio_chan_spec *channels;
>> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	unsigned int num_ch;
>> +	int ret, chan_idx;
>> +
>> +	num_ch = of_property_count_u32_elems(indio_dev->dev.of_node,
>> +					     "st,adc-channels");
>> +	if (num_ch < 0 || num_ch >= adc->common->num_chs) {
>> +		dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
>> +		return num_ch < 0 ? num_ch : -EINVAL;
>> +	}
>> +
>> +	channels = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*channels),
>> +				GFP_KERNEL);
>> +	if (!channels)
>> +		return -ENOMEM;
>> +
>> +	if (adc->mode == DFSDM_ADC) {
>> +		/*
>> +		 * Bind to sd modulator iio device for ADC only.
>> +		 * For Audio the PDM microphone will be handled by ASoC
>> +		 */
>> +		adc->hwc = iio_hw_consumer_alloc(&indio_dev->dev);
>> +		if (IS_ERR(adc->hwc)) {
>> +			dev_err(&indio_dev->dev, "no backend found\n");
> Deferred probing a possibility? I'm not quite sure and you know what is going
> on here better than me ;)
Reading your comment i have a doubt... i will cross check
Idea here is that there is a customer-provider relationchip with the
sd-modulator driver. So need that sd-modulator driver is probed first.

>> +			return PTR_ERR(adc->hwc);
>> +		}
>> +	}
>> +
>> +	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
>> +		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, channels,
>> +						    chan_idx);
>> +		if (ret < 0)
>> +			goto free_hwc;
>> +	}
>> +
>> +	indio_dev->num_channels = num_ch;
>> +	indio_dev->channels = channels;
>> +
>> +	return 0;
>> +
>> +free_hwc:
>> +	if (adc->hwc)
>> +		iio_hw_consumer_free(adc->hwc);
>> +	return ret;
>> +}
>> +
[...]