Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: =?UTF-8?Q?Re=3a_=5bPATCH=5d_iio=3a_adc=3a_rockchip=5fsaradc=3a_Add_?=
 =?UTF-8?B?c3VwcG9ydCBpaW8gYnVmZmVyc+OAkOivt+azqOaEj++8jOmCruS7tueUsWxpbnV4?=
 =?UTF-8?Q?-rockchip-bounces+xxm=3drock-chips=2ecom=40lists=2einfradead=2eor?=
 =?UTF-8?B?Z+S7o+WPkeOAkQ==?=
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Heiko Stuebner <heiko@sntech.de>, lars@metafoo.de,
        linux-iio@vger.kernel.org,
        Heiko Stuebner <heiko.stuebner@theobroma-systems.com>,
        linux-kernel@vger.kernel.org, kever.yang@rock-chips.com,
        linux-rockchip@lists.infradead.org, pmeerw@pmeerw.net,
        knaack.h@gmx.de
References: <20200301112353.887028-1-heiko@sntech.de>
 <67e46e36-ebac-ebe3-b4f4-9edb88fb0dcf@rock-chips.com>
 <20200303203236.2cbcfeee@archlinux>
From:   xxm <xxm@rock-chips.com>
Message-ID: <9036a57e-be5e-49e1-6f0e-77b3a5e9fa12@rock-chips.com>
Date:   Wed, 4 Mar 2020 09:39:10 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.1
MIME-Version: 1.0
In-Reply-To: <20200303203236.2cbcfeee@archlinux>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

在 2020/3/4 4:32, Jonathan Cameron 写道:
> On Mon, 2 Mar 2020 10:11:02 +0800
> xxm <xxm@rock-chips.com> wrote:
> 
>> Hi, Heiko
>>
>> 在 2020/3/1 19:23, Heiko Stuebner 写道:
>>> From: Simon Xue <xxm@rock-chips.com>
>>>
>>> Add the ability to also support access via (triggered) buffers
>>> next to the existing direct mode.
>>>
>>> Device in question is the Odroid Go Advance that connects a joystick
>>> to two of the saradc channels for X and Y axis and the new (and still
>>> pending) adc joystick driver of course wants to use triggered buffers
>>> from the iio subsystem.
>>>
>>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>>> [some simplifications and added commit description]
>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>> ---
>>>    drivers/iio/adc/Kconfig           |   2 +
>>>    drivers/iio/adc/rockchip_saradc.c | 137 ++++++++++++++++++++++--------
>>>    2 files changed, 102 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 82e33082958c..55d2499ff757 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -787,6 +787,8 @@ config ROCKCHIP_SARADC
>>>    	tristate "Rockchip SARADC driver"
>>>    	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
>>>    	depends on RESET_CONTROLLER
>>> +	select IIO_BUFFER
>>> +	select IIO_TRIGGERED_BUFFER
>>>    	help
>>>    	  Say yes here to build support for the SARADC found in SoCs from
>>>    	  Rockchip.
>>> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
>>> index 582ba047c4a6..402b2210a682 100644
>>> --- a/drivers/iio/adc/rockchip_saradc.c
>>> +++ b/drivers/iio/adc/rockchip_saradc.c
>>> @@ -15,7 +15,11 @@
>>>    #include <linux/delay.h>
>>>    #include <linux/reset.h>
>>>    #include <linux/regulator/consumer.h>
>>> +#include <linux/iio/buffer.h>
>>>    #include <linux/iio/iio.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>>    
>>>    #define SARADC_DATA			0x00
>>>    
>>> @@ -34,7 +38,6 @@
>>>    #define SARADC_TIMEOUT			msecs_to_jiffies(100)
>>>    
>>>    struct rockchip_saradc_data {
>>> -	int				num_bits;
>>>    	const struct iio_chan_spec	*channels;
>>>    	int				num_channels;
>>>    	unsigned long			clk_rate;
>>> @@ -49,8 +52,37 @@ struct rockchip_saradc {
>>>    	struct reset_control	*reset;
>>>    	const struct rockchip_saradc_data *data;
>>>    	u16			last_val;
>>> +	const struct iio_chan_spec *last_chan;
>>>    };
>>>    
>>> +static void rockchip_saradc_power_down(struct rockchip_saradc *info)
>>> +{
>>> +	/* Clear irq & power down adc */
>>> +	writel_relaxed(0, info->regs + SARADC_CTRL);
>>> +}
>>> +
>>> +static int rockchip_saradc_conversion(struct rockchip_saradc *info,
>>> +				   struct iio_chan_spec const *chan)
>>> +{
>>> +	reinit_completion(&info->completion);
>>> +
>>> +	/* 8 clock periods as delay between power up and start cmd */
>>> +	writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
>>> +
>>> +	info->last_chan = chan;
>>> +
>>> +	/* Select the channel to be used and trigger conversion */
>>> +	writel(SARADC_CTRL_POWER_CTRL
>>> +			| (chan->channel & SARADC_CTRL_CHN_MASK)
>>> +			| SARADC_CTRL_IRQ_ENABLE,
>>> +		   info->regs + SARADC_CTRL);
>>> +
>>> +	if (!wait_for_completion_timeout(&info->completion, SARADC_TIMEOUT))
>>> +		return -ETIMEDOUT;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
>>>    				    struct iio_chan_spec const *chan,
>>>    				    int *val, int *val2, long mask)
>>> @@ -62,24 +94,12 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
>>>    	case IIO_CHAN_INFO_RAW:
>>>    		mutex_lock(&indio_dev->mlock);
>>>    
>>> -		reinit_completion(&info->completion);
>>> -
>>> -		/* 8 clock periods as delay between power up and start cmd */
>>> -		writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
>>> -
>>> -		/* Select the channel to be used and trigger conversion */
>>> -		writel(SARADC_CTRL_POWER_CTRL
>>> -				| (chan->channel & SARADC_CTRL_CHN_MASK)
>>> -				| SARADC_CTRL_IRQ_ENABLE,
>>> -		       info->regs + SARADC_CTRL);
>>> -
>>> -		if (!wait_for_completion_timeout(&info->completion,
>>> -						 SARADC_TIMEOUT)) {
>>> -			writel_relaxed(0, info->regs + SARADC_CTRL);
>>> +		ret = rockchip_saradc_conversion(info, chan);
>>> +		if (ret) {
>>> +			rockchip_saradc_power_down(info);
>>>    			mutex_unlock(&indio_dev->mlock);
>>> -			return -ETIMEDOUT;
>>> +			return ret;
>>>    		}
>>> -
>>>    		*val = info->last_val;
>>>    		mutex_unlock(&indio_dev->mlock);
>>>    		return IIO_VAL_INT;
>>> @@ -91,7 +111,7 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
>>>    		}
>>>    
>>>    		*val = ret / 1000;
>>> -		*val2 = info->data->num_bits;
>>> +		*val2 = chan->scan_type.realbits;
>>>    		return IIO_VAL_FRACTIONAL_LOG2;
>>>    	default:
>>>    		return -EINVAL;
>>> @@ -104,10 +124,9 @@ static irqreturn_t rockchip_saradc_isr(int irq, void *dev_id)
>>>    
>>>    	/* Read value */
>>>    	info->last_val = readl_relaxed(info->regs + SARADC_DATA);
>>> -	info->last_val &= GENMASK(info->data->num_bits - 1, 0);
>>> +	info->last_val &= GENMASK(info->last_chan->scan_type.realbits - 1, 0);
>>>    
>>> -	/* Clear irq & power down adc */
>>> -	writel_relaxed(0, info->regs + SARADC_CTRL);
>>> +	rockchip_saradc_power_down(info);
>>>    
>>>    	complete(&info->completion);
>>>    
>>> @@ -118,51 +137,55 @@ static const struct iio_info rockchip_saradc_iio_info = {
>>>    	.read_raw = rockchip_saradc_read_raw,
>>>    };
>>>    
>>> -#define ADC_CHANNEL(_index, _id) {				\
>>> +#define ADC_CHANNEL(_index, _id, _res) {			\
>>>    	.type = IIO_VOLTAGE,					\
>>>    	.indexed = 1,						\
>>>    	.channel = _index,					\
>>>    	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>>>    	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>>>    	.datasheet_name = _id,					\
>>> +	.scan_index = _index,					\
>>> +	.scan_type = {						\
>>> +		.sign = 'u',					\
>>> +		.realbits = _res,				\
>>> +		.storagebits = 16,				\
>>> +		.endianness = IIO_LE,				\
>>> +	},							\
>>>    }
>>>    
>>>    static const struct iio_chan_spec rockchip_saradc_iio_channels[] = {
>>> -	ADC_CHANNEL(0, "adc0"),
>>> -	ADC_CHANNEL(1, "adc1"),
>>> -	ADC_CHANNEL(2, "adc2"),
>>> +	ADC_CHANNEL(0, "adc0", 10),
>>> +	ADC_CHANNEL(1, "adc1", 10),
>>> +	ADC_CHANNEL(2, "adc2", 10),
>>>    };
>>>    
>>>    static const struct rockchip_saradc_data saradc_data = {
>>> -	.num_bits = 10,
>>>    	.channels = rockchip_saradc_iio_channels,
>>>    	.num_channels = ARRAY_SIZE(rockchip_saradc_iio_channels),
>>>    	.clk_rate = 1000000,
>>>    };
>>>    
>>>    static const struct iio_chan_spec rockchip_rk3066_tsadc_iio_channels[] = {
>>> -	ADC_CHANNEL(0, "adc0"),
>>> -	ADC_CHANNEL(1, "adc1"),
>>> +	ADC_CHANNEL(0, "adc0", 12),
>>> +	ADC_CHANNEL(1, "adc1", 12),
>>>    };
>>>    
>>>    static const struct rockchip_saradc_data rk3066_tsadc_data = {
>>> -	.num_bits = 12,
>>>    	.channels = rockchip_rk3066_tsadc_iio_channels,
>>>    	.num_channels = ARRAY_SIZE(rockchip_rk3066_tsadc_iio_channels),
>>>    	.clk_rate = 50000,
>>>    };
>>>    
>>>    static const struct iio_chan_spec rockchip_rk3399_saradc_iio_channels[] = {
>>> -	ADC_CHANNEL(0, "adc0"),
>>> -	ADC_CHANNEL(1, "adc1"),
>>> -	ADC_CHANNEL(2, "adc2"),
>>> -	ADC_CHANNEL(3, "adc3"),
>>> -	ADC_CHANNEL(4, "adc4"),
>>> -	ADC_CHANNEL(5, "adc5"),
>>> +	ADC_CHANNEL(0, "adc0", 10),
>>> +	ADC_CHANNEL(1, "adc1", 10),
>>> +	ADC_CHANNEL(2, "adc2", 10),
>>> +	ADC_CHANNEL(3, "adc3", 10),
>>> +	ADC_CHANNEL(4, "adc4", 10),
>>> +	ADC_CHANNEL(5, "adc5", 10),
>>>    };
>>>    
>>>    static const struct rockchip_saradc_data rk3399_saradc_data = {
>>> -	.num_bits = 10,
>>>    	.channels = rockchip_rk3399_saradc_iio_channels,
>>>    	.num_channels = ARRAY_SIZE(rockchip_rk3399_saradc_iio_channels),
>>>    	.clk_rate = 1000000,
>>> @@ -193,6 +216,39 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
>>>    	reset_control_deassert(reset);
>>>    }
>>>    
>>> +static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
>>> +{
>>> +	struct iio_poll_func *pf = p;
>>> +	struct iio_dev *i_dev = pf->indio_dev;
>>> +	struct rockchip_saradc *info = iio_priv(i_dev);
>>> +	u16 data[20];
>> How about this:
>> #define MAX_CHANNEL_NUM 16
> 
> Unfortunately this is a bit more complex than it seems.
> The buffer needs to be big enough for all the channels
> + a 8 byte aligned space to put the timestamp in.
> 
> You can construct that in a fashion suitable to use in a
> macro but it's a bit more fiddly than simply being the
> maximum number of channels.
> 
Make use of iio_dev->scan_bytes to alloc a buffer in 
iio_info->update_scan_mode callback for storing the
"data + timestamp" is another way
>> u16 data[MAX_CHANNEL_NUM];
>>> +	int ret;
>>> +	int i, j = 0;
>>> +
>>> +	mutex_lock(&i_dev->mlock);
>>> +
>>> +	for_each_set_bit(i, i_dev->active_scan_mask, i_dev->masklength) {
>>> +		const struct iio_chan_spec *chan = &i_dev->channels[i];
>>> +
>>> +		ret = rockchip_saradc_conversion(info, chan);
>>> +		if (ret) {
>>> +			rockchip_saradc_power_down(info);
>>> +			goto out;
>>> +		}
>>> +
>>> +		data[j] = info->last_val;
>>> +		j++;
>>> +	}
>>> +
>>> +	iio_push_to_buffers_with_timestamp(i_dev, data, iio_get_time_ns(i_dev));
>>> +out:
>>> +	mutex_unlock(&i_dev->mlock);
>>> +
>>> +	iio_trigger_notify_done(i_dev->trig);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>>    static int rockchip_saradc_probe(struct platform_device *pdev)
>>>    {
>>>    	struct rockchip_saradc *info = NULL;
>>> @@ -315,12 +371,19 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>>>    	indio_dev->channels = info->data->channels;
>>>    	indio_dev->num_channels = info->data->num_channels;
>>>    
>>> -	ret = iio_device_register(indio_dev);
>>> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +					 rockchip_saradc_trigger_handler, NULL);
>> devm_iio_triggered_buffer_setup seems better
>>>    	if (ret)
>>>    		goto err_clk;
>>>    
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret)
>>> +		goto err_buffer_cleanup;
>>> +
>>>    	return 0;
>>>    
>>> +err_buffer_cleanup:
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>>    err_clk:
>>>    	clk_disable_unprepare(info->clk);
>>>    err_pclk:
>>>    
>> xxm@rock-chips.com
>>
>>
> 
> 
> 
>