Message-ID: <99cbc8e6-18fb-eb45-c27e-9244d1d2eb3b@st.com>
Date:   Tue, 15 Nov 2016 14:24:58 +0100
From:   Fabrice Gasnier <fabrice.gasnier@...com>
To:     Jonathan Cameron <jic23@...nel.org>, <linux-iio@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     <lee.jones@...aro.org>, <linux@...linux.org.uk>,
        <robh+dt@...nel.org>, <mark.rutland@....com>,
        <mcoquelin.stm32@...il.com>, <alexandre.torgue@...com>,
        <lars@...afoo.de>, <knaack.h@....de>, <pmeerw@...erw.net>
Subject: Re: [PATCH v2 3/6] iio: adc: Add support for STM32 ADC

On 11/12/2016 06:08 PM, Jonathan Cameron wrote:
> On 10/11/16 16:18, Fabrice Gasnier wrote:
>> This patch adds support for STMicroelectronics STM32 MCU's analog to
>> digital converter.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
> Nice and clean - few minor things inline.
>
> Jonathan
>> ---
>>   drivers/iio/adc/Kconfig     |  10 +
>>   drivers/iio/adc/Makefile    |   1 +
>>   drivers/iio/adc/stm32-adc.c | 525 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 536 insertions(+)
>>   create mode 100644 drivers/iio/adc/stm32-adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 7edcf32..61ba674 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -419,6 +419,16 @@ config ROCKCHIP_SARADC
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called rockchip_saradc.
>>   
>> +config STM32_ADC
>> +	tristate "STMicroelectronics STM32 adc"
>> +	depends on MFD_STM32_ADC
>> +	help
>> +	  Say yes here to build support for STMicroelectronics stm32 Analog
>> +	  to Digital Converter (ADC).
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called stm32-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 7a40c04..df7a221 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -41,6 +41,7 @@ obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>>   obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>>   obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>   obj-$(CONFIG_STX104) += stx104.o
>> +obj-$(CONFIG_STM32_ADC) += stm32-adc.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-adc.c b/drivers/iio/adc/stm32-adc.c
>> new file mode 100644
>> index 0000000..2be5fee
>> --- /dev/null
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -0,0 +1,525 @@
>> +/*
>> + * This file is part of STM32 ADC driver
>> + *
>> + * Copyright (C) 2016, STMicroelectronics - All Rights Reserved
>> + * Author: Fabrice Gasnier <fabrice.gasnier@...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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/stm32-adc-core.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +
>> +/* STM32F4 - Registers for each ADC instance */
> Not really sure the X adds anything in these, but doesn't do much harm
> I guess ;)
You're right. I'll remove the X. I initially added that to remind this 
stands for ADC1, 2 or 3...
Moreover, removing it will make register name match with reference manual.
>> +#define STM32F4_ADCX_SR			0x00
>> +#define STM32F4_ADCX_CR1		0x04
>> +#define STM32F4_ADCX_CR2		0x08
>> +#define STM32F4_ADCX_SMPR1		0x0C
>> +#define STM32F4_ADCX_SMPR2		0x10
>> +#define STM32F4_ADCX_HTR		0x24
>> +#define STM32F4_ADCX_LTR		0x28
>> +#define STM32F4_ADCX_SQR1		0x2C
>> +#define STM32F4_ADCX_SQR2		0x30
>> +#define STM32F4_ADCX_SQR3		0x34
>> +#define STM32F4_ADCX_JSQR		0x38
>> +#define STM32F4_ADCX_JDR1		0x3C
>> +#define STM32F4_ADCX_JDR2		0x40
>> +#define STM32F4_ADCX_JDR3		0x44
>> +#define STM32F4_ADCX_JDR4		0x48
>> +#define STM32F4_ADCX_DR			0x4C
>> +
>> +/* STM32F4_ADCX_SR - bit fields */
>> +#define STM32F4_OVR			BIT(5)
>> +#define STM32F4_STRT			BIT(4)
>> +#define STM32F4_EOC			BIT(1)
>> +
>> +/* STM32F4_ADCX_CR1 - bit fields */
>> +#define STM32F4_OVRIE			BIT(26)
>> +#define STM32F4_SCAN			BIT(8)
>> +#define STM32F4_EOCIE			BIT(5)
>> +
>> +/* STM32F4_ADCX_CR2 - bit fields */
>> +#define STM32F4_SWSTART			BIT(30)
>> +#define STM32F4_EXTEN_MASK		GENMASK(29, 28)
>> +#define STM32F4_EOCS			BIT(10)
>> +#define STM32F4_ADON			BIT(0)
>> +
>> +/* STM32F4_ADCX_SQR1 - bit fields */
>> +#define STM32F4_L_SHIFT			20
>> +#define STM32F4_L_MASK			GENMASK(23, 20)
>> +
>> +/* STM32F4_ADCX_SQR3 - bit fields */
>> +#define STM32F4_SQ1_SHIFT		0
>> +#define STM32F4_SQ1_MASK		GENMASK(4, 0)
>> +
>> +#define STM32_ADC_MAX_SQ		16	/* SQ1..SQ16 */
>> +#define STM32_ADC_TIMEOUT_US		100000
>> +#define STM32_ADC_TIMEOUT	(msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
>> +
>> +/**
>> + * struct stm32_adc - private data of each ADC IIO instance
>> + * @common:		reference to ADC block common data
>> + * @offset:		ADC instance register offset in ADC block
>> + * @completion:		end of single conversion completion
>> + * @buffer:		data buffer
>> + * @clk:		optional adc clock, for this adc instance
>> + * @irq:		interrupt for this adc instance
>> + * @lock:		spinlock
>> + */
>> +struct stm32_adc {
>> +	struct stm32_adc_common	*common;
>> +	u32			offset;
>> +	struct completion	completion;
>> +	u16			*buffer;
>> +	struct clk		*clk;
>> +	int			irq;
>> +	spinlock_t		lock;		/* interrupt lock */
>> +};
>> +
>> +/**
>> + * struct stm32_adc_chan_spec - specification of stm32 adc channel
>> + * @type:	IIO channel type
>> + * @channel:	channel number (single ended)
>> + * @name:	channel name (single ended)
>> + */
>> +struct stm32_adc_chan_spec {
>> +	enum iio_chan_type	type;
>> +	int			channel;
>> +	const char		*name;
>> +};
>> +
>> +/* Input definitions common for all STM32F4 instances */
>> +static const struct stm32_adc_chan_spec stm32f4_adc123_channels[] = {
>> +	{ IIO_VOLTAGE, 0, "in0" },
>> +	{ IIO_VOLTAGE, 1, "in1" },
>> +	{ IIO_VOLTAGE, 2, "in2" },
>> +	{ IIO_VOLTAGE, 3, "in3" },
>> +	{ IIO_VOLTAGE, 4, "in4" },
>> +	{ IIO_VOLTAGE, 5, "in5" },
>> +	{ IIO_VOLTAGE, 6, "in6" },
>> +	{ IIO_VOLTAGE, 7, "in7" },
>> +	{ IIO_VOLTAGE, 8, "in8" },
>> +	{ IIO_VOLTAGE, 9, "in9" },
>> +	{ IIO_VOLTAGE, 10, "in10" },
>> +	{ IIO_VOLTAGE, 11, "in11" },
>> +	{ IIO_VOLTAGE, 12, "in12" },
>> +	{ IIO_VOLTAGE, 13, "in13" },
>> +	{ IIO_VOLTAGE, 14, "in14" },
>> +	{ IIO_VOLTAGE, 15, "in15" },
>> +};
>> +
>> +/**
>> + * STM32 ADC registers access routines
>> + * @adc: stm32 adc instance
>> + * @reg: reg offset in adc instance
>> + *
>> + * Note: All instances share same base, with 0x0, 0x100 or 0x200 offset resp.
>> + * for adc1, adc2 and adc3.
>> + */
>> +static u32 stm32_adc_readl(struct stm32_adc *adc, u32 reg)
>> +{
>> +	return readl_relaxed(adc->common->base + adc->offset + reg);
>> +}
>> +
>> +static u32 stm32_adc_readw(struct stm32_adc *adc, u32 reg)
>> +{
>> +	return readw_relaxed(adc->common->base + adc->offset + reg);
>> +}
>> +
>> +static void stm32_adc_writel(struct stm32_adc *adc, u32 reg, u32 val)
>> +{
>> +	writel_relaxed(val, adc->common->base + adc->offset + reg);
>> +}
>> +
>> +static void stm32_adc_set_bits(struct stm32_adc *adc, u32 reg, u32 bits)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&adc->lock, flags);
>> +	stm32_adc_writel(adc, reg, stm32_adc_readl(adc, reg) | bits);
>> +	spin_unlock_irqrestore(&adc->lock, flags);
>> +}
>> +
>> +static void stm32_adc_clr_bits(struct stm32_adc *adc, u32 reg, u32 bits)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&adc->lock, flags);
>> +	stm32_adc_writel(adc, reg, stm32_adc_readl(adc, reg) & ~bits);
>> +	spin_unlock_irqrestore(&adc->lock, flags);
>> +}
>> +
>> +/**
>> + * stm32_adc_conv_irq_enable() - Enable end of conversion interrupt
>> + * @adc: stm32 adc instance
>> + */
>> +static void stm32_adc_conv_irq_enable(struct stm32_adc *adc)
>> +{
>> +	stm32_adc_set_bits(adc, STM32F4_ADCX_CR1, STM32F4_EOCIE);
>> +};
>> +
>> +/**
>> + * stm32_adc_conv_irq_disable() - Disable end of conversion interrupt
>> + * @adc: stm32 adc instance
>> + */
>> +static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>> +{
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_CR1, STM32F4_EOCIE);
>> +}
>> +
>> +/**
>> + * stm32_adc_start_conv() - Start conversions for regular channels.
>> + * @adc: stm32 adc instance
>> + */
>> +static void stm32_adc_start_conv(struct stm32_adc *adc)
>> +{
>> +	stm32_adc_set_bits(adc, STM32F4_ADCX_CR1, STM32F4_SCAN);
>> +	stm32_adc_set_bits(adc, STM32F4_ADCX_CR2, STM32F4_EOCS | STM32F4_ADON);
>> +
>> +	/* Wait for Power-up time (tSTAB from datasheet) */
>> +	usleep_range(2, 3);
>> +
>> +	/* Software start ? (e.g. trigger detection disabled ?) */
>> +	if (!(stm32_adc_readl(adc, STM32F4_ADCX_CR2) & STM32F4_EXTEN_MASK))
>> +		stm32_adc_set_bits(adc, STM32F4_ADCX_CR2, STM32F4_SWSTART);
>> +}
>> +
>> +static void stm32_adc_stop_conv(struct stm32_adc *adc)
>> +{
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_CR2, STM32F4_EXTEN_MASK);
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_SR, STM32F4_STRT);
>> +
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_CR1, STM32F4_SCAN);
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_CR2, STM32F4_ADON);
>> +}
>> +
>> +/**
>> + * stm32_adc_single_conv() - Performs a single conversion
>> + * @indio_dev: IIO device
>> + * @chan: IIO channel
>> + * @res: conversion result
>> + *
>> + * The function performs a single conversion on a given channel:
>> + * - Program sequencer with one channel (e.g. in SQ1 with len = 1)
>> + * - Use SW trigger
>> + * - Start conversion, then wait for interrupt completion.
>> + */
>> +static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>> +				 const struct iio_chan_spec *chan,
>> +				 int *res)
>> +{
>> +	struct stm32_adc *adc = iio_priv(indio_dev);
>> +	long timeout;
>> +	u32 val;
>> +	u16 result;
>> +	int ret;
>> +
>> +	reinit_completion(&adc->completion);
>> +
>> +	adc->buffer = &result;
>> +
>> +	/* Program chan number in regular sequence */
>> +	val = stm32_adc_readl(adc, STM32F4_ADCX_SQR3);
>> +	val &= ~STM32F4_SQ1_MASK;
>> +	val |= chan->channel << STM32F4_SQ1_SHIFT;
>> +	stm32_adc_writel(adc, STM32F4_ADCX_SQR3, val);
>> +
>> +	/* Set regular sequence len (0 for 1 conversion) */
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_SQR1, STM32F4_L_MASK);
>> +
>> +	/* Trigger detection disabled (conversion can be launched in SW) */
>> +	stm32_adc_clr_bits(adc, STM32F4_ADCX_CR2, STM32F4_EXTEN_MASK);
>> +
>> +	stm32_adc_conv_irq_enable(adc);
>> +
>> +	stm32_adc_start_conv(adc);
>> +
>> +	timeout = wait_for_completion_interruptible_timeout(
>> +					&adc->completion, STM32_ADC_TIMEOUT);
>> +	if (timeout == 0) {
>> +		dev_warn(&indio_dev->dev, "Conversion timed out!\n");
>> +		ret = -ETIMEDOUT;
>> +	} else if (timeout < 0) {
>> +		dev_warn(&indio_dev->dev, "Interrupted conversion!\n");
>> +		ret = -EINTR;
>> +	} else {
>> +		*res = result;
>> +		ret = IIO_VAL_INT;
>> +	}
>> +
>> +	stm32_adc_stop_conv(adc);
>> +
>> +	stm32_adc_conv_irq_disable(adc);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_adc_read_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      int *val, int *val2, long mask)
>> +{
>> +	struct stm32_adc *adc = iio_priv(indio_dev);
>> +	int ret = -EINVAL;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = iio_device_claim_direct_mode(indio_dev);
>> +		if (ret)
>> +			return ret;
>> +		if (chan->type == IIO_VOLTAGE)
>> +			ret = stm32_adc_single_conv(indio_dev, chan, val);
>> +		else
>> +			ret = -EINVAL;
>> +		iio_device_release_direct_mode(indio_dev);
> return directly here.  Basically always preferred to return directly if
> there is not cleanup to be done.
I will fix it.
>> +		break;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = adc->common->vref_mv;
>> +		*val2 = chan->scan_type.realbits;
>> +		ret = IIO_VAL_FRACTIONAL_LOG2;
> return directly here.
I will fix it.
>> +		break;
>> +	default:
> return -EINVAL here.
I will fix it.
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static irqreturn_t stm32_adc_isr(int irq, void *data)
>> +{
>> +	struct stm32_adc *adc = data;
>> +	u32 status = stm32_adc_readl(adc, STM32F4_ADCX_SR);
>> +	irqreturn_t ret = IRQ_NONE;
>> +
>> +	if (status & STM32F4_EOC) {
>> +		*adc->buffer = stm32_adc_readw(adc, STM32F4_ADCX_DR);
>> +		complete(&adc->completion);
>> +		ret = IRQ_HANDLED;
> Slightly tidier to return IRQ_HANDLED here and directly return
> IRQ_NONE below.
I will fix it.
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_adc_of_xlate(struct iio_dev *indio_dev,
>> +			      const struct of_phandle_args *iiospec)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < indio_dev->num_channels; i++)
>> +		if (indio_dev->channels[i].channel == iiospec->args[0])
>> +			return i;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +/**
>> + * stm32_adc_debugfs_reg_access - read or write register value
>> + *
>> + * To read a value from an ADC register:
>> + *   echo [ADC reg offset] > direct_reg_access
>> + *   cat direct_reg_access
>> + *
>> + * To write a value in a ADC register:
>> + *   echo [ADC_reg_offset] [value] > direct_reg_access
>> + */
>> +static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>> +					unsigned reg, unsigned writeval,
>> +					unsigned *readval)
>> +{
>> +	struct stm32_adc *adc = iio_priv(indio_dev);
>> +
>> +	if (!readval)
>> +		stm32_adc_writel(adc, reg, writeval);
>> +	else
>> +		*readval = stm32_adc_readl(adc, reg);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct iio_info stm32_adc_iio_info = {
>> +	.read_raw = stm32_adc_read_raw,
>> +	.debugfs_reg_access = stm32_adc_debugfs_reg_access,
>> +	.of_xlate = stm32_adc_of_xlate,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>> +				    struct iio_chan_spec *chan,
>> +				    const struct stm32_adc_chan_spec *channel,
>> +				    int scan_index)
>> +{
>> +	chan->type = channel->type;
>> +	chan->channel = channel->channel;
>> +	chan->datasheet_name = channel->name;
>> +	chan->extend_name = channel->name;
> Don't set extend_name. That name doesn't add sufficient information to
> make it worth adding custom ABI to the userspace interface.
I will fix it.
>
>
>> +	chan->scan_index = scan_index;
>> +	chan->indexed = 1;
>> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>> +	chan->scan_type.sign = 'u';
>> +	chan->scan_type.realbits = 12;
>> +	chan->scan_type.storagebits = 16;
>> +}
>> +
>> +static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>> +{
>> +	struct device_node *node = indio_dev->dev.of_node;
>> +	struct property *prop;
>> +	const __be32 *cur;
>> +	struct iio_chan_spec *channels;
>> +	int scan_index = 0, num_channels;
>> +	u32 val;
>> +
>> +	num_channels = of_property_count_u32_elems(node, "st,adc-channels");
>> +	if (num_channels < 0 ||
>> +	    num_channels >= ARRAY_SIZE(stm32f4_adc123_channels)) {
>> +		dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
>> +		return num_channels < 0 ? num_channels : -EINVAL;
>> +	}
>> +
>> +	channels = devm_kcalloc(&indio_dev->dev, num_channels,
>> +				sizeof(struct iio_chan_spec), GFP_KERNEL);
>> +	if (!channels)
>> +		return -ENOMEM;
>> +
>> +	of_property_for_each_u32(node, "st,adc-channels", prop, cur, val) {
>> +		if (val >= ARRAY_SIZE(stm32f4_adc123_channels)) {
>> +			dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
>> +			return -EINVAL;
>> +		}
>> +		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
>> +					&stm32f4_adc123_channels[val],
>> +					scan_index);
>> +		scan_index++;
>> +	}
>> +
>> +	indio_dev->num_channels = scan_index;
>> +	indio_dev->channels = channels;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_adc_probe(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct stm32_adc *adc;
>> +	int ret;
>> +
>> +	if (!pdev->dev.of_node)
>> +		return -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	adc = iio_priv(indio_dev);
>> +	adc->common = dev_get_drvdata(pdev->dev.parent);
>> +	spin_lock_init(&adc->lock);
>> +	init_completion(&adc->completion);
>> +
>> +	indio_dev->name = dev_name(&pdev->dev);
>> +	indio_dev->dev.parent = &pdev->dev;
>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>> +	indio_dev->info = &stm32_adc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	platform_set_drvdata(pdev, adc);
>> +
>> +	ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset);
>> +	if (ret != 0) {
>> +		dev_err(&pdev->dev, "missing reg property\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	adc->irq = platform_get_irq(pdev, 0);
>> +	if (adc->irq < 0) {
>> +		dev_err(&pdev->dev, "failed to get irq\n");
>> +		return adc->irq;
>> +	}
>> +
>> +	ret = devm_request_irq(&pdev->dev, adc->irq, stm32_adc_isr,
>> +			       0, pdev->name, adc);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request IRQ\n");
>> +		return ret;
>> +	}
>> +
>> +	adc->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(adc->clk)) {
> Could it concievably be deferred?  Would be happier if this explicitly
> checked for -ENODEV or whatever gets returned when not clock has
> been specified.
I will fix it.
>> +		adc->clk = NULL;
>> +		dev_dbg(&pdev->dev, "No child clk found\n");
>> +	} else {
>> +		ret = clk_prepare_enable(adc->clk);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "clk enable failed\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = stm32_adc_chan_of_init(indio_dev);
>> +	if (ret < 0)
>> +		goto err_clk_disable;
>> +
>> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> This use of devm registration is going to cause a race in the remove.
> The userspace interface will not be removed until after the remove
> function has run.  That disables the clock thus leaving us a window
> where we could try and access the device with no clock enabled.
>
> Basic rule of thumb is that use of devm must not effect the ordering
> of unrolling what you do in probe when it comes to remove.
> (which more or less means that you can't use devm_iio_device_register
> unless you have no remove at all).
I will fix it.

Thanks,
Fabrice
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "iio dev register failed\n");
>> +		goto err_clk_disable;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_clk_disable:
>> +	if (adc->clk)
>> +		clk_disable_unprepare(adc->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_adc_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32_adc *adc = platform_get_drvdata(pdev);
>> +
>> +	if (adc->clk)
>> +		clk_disable_unprepare(adc->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_adc_of_match[] = {
>> +	{ .compatible = "st,stm32f4-adc" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_adc_of_match);
>> +
>> +static struct platform_driver stm32_adc_driver = {
>> +	.probe = stm32_adc_probe,
>> +	.remove = stm32_adc_remove,
>> +	.driver = {
>> +		.name = "stm32-adc",
>> +		.of_match_table = stm32_adc_of_match,
>> +	},
>> +};
>> +module_platform_driver(stm32_adc_driver);
>> +
>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@...com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 ADC IIO driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:stm32-adc");
>>

Powered by blists - more mailing lists