Re: [PATCH] iio: adc: Add Renesas GyroADC driver

[Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 12/30/2016 08:50 PM, Peter Meerwald-Stadler wrote:
> 
>> Add IIO driver for the Renesas RCar GyroADC block. This block is a
>> simple 4/8-channel ADC which samples 12/15/24 bits of data every
>> cycle from all channels.
> 
> comments below
>  
>> Signed-off-by: Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>> Cc: Simon Horman <horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
>> ---
>>  .../bindings/iio/adc/renesas,gyroadc.txt           |  38 +++
>>  MAINTAINERS                                        |   6 +
>>  drivers/iio/adc/Kconfig                            |  10 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/rcar_gyro_adc.c                    | 379 +++++++++++++++++++++
>>  5 files changed, 434 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>>  create mode 100644 drivers/iio/adc/rcar_gyro_adc.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>> new file mode 100644
>> index 0000000..3fd5f57
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt
>> @@ -0,0 +1,38 @@
>> +* Renesas RCar GyroADC device driver
>> +
>> +Required properties:
>> +- compatible:	Should be "renesas,rcar-gyroadc" for regular GyroADC or
>> +		"renesas,rcar-gyroadc-r8a7792" for GyroADC without interrupt
>> +		block found in R8A7792.
>> +- reg:		Address and length of the register set for the device
>> +- clocks:	References to all the clocks specified in the clock-names
>> +		property as specified in
>> +		Documentation/devicetree/bindings/clock/clock-bindings.txt.
>> +- clock-names:	Shall contain "fck" and "if". The "fck" are the GyroADC block
> 
> "fck" is...
> 
>> +		clock, the "if" are the interface clock.
> 
> "if" is ...
> 
>> +		power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
> 
> this is just an example and not appropriate here?

Oh, copy-paste error, thanks :)

>> +- power-domains: Must contain a reference to the PM domain, if available.
>> +- renesas,gyroadc-mode:	GyroADC mode of operation, must be either of:
>> +			1 - MB88101A mode, 12bit sampling, 4 channels
>> +			2 - ADCS7476 mode, 15bit sampling, 8 channels
>> +			3 - MAX1162 mode,  16bit sampling, 8 channels
>> +- renesas,gyroadc-vref:	Array of reference voltage values for each input to
>> +			the GyroADC, in uV. Array must have 4 elemenets for
> 
> elements

All spelling fixed.

[...]

>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 99c0514..4a4cac7 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -408,6 +408,16 @@ config QCOM_SPMI_VADC
>>  	  To compile this driver as a module, choose M here: the module will
>>  	  be called qcom-spmi-vadc.
>>  
>> +config RCAR_GYRO_ADC
>> +	tristate "Renesas RCAR GyroADC driver"
>> +	depends on ARCH_RCAR_GEN2 || (ARM && COMPILE_TEST)
>> +	help
>> +	  Say yes here to build support for the GyroADC found in Renesas
>> +	  RCar Gen2 SoCs.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called rcar-gyroadc.
> 
> called rcar_gyro_adc?

Why so ? The driver is really named rcar-gyroadc , I guess I should
rename either the file or the driver to keep things consistent. So
probably the file .

>> +
>>  config ROCKCHIP_SARADC
>>  	tristate "Rockchip SARADC driver"
>>  	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile

[...]

>> +
>> +#define RCAR_GYROADC_CLOCK_LENGTH		0x08
>> +#define RCAR_GYROADC_1_25MS_LENGTH		0x0c
>> +
>> +#define RCAR_GYROADC_REALTIME_DATA(ch)		(0x10 + ((ch) * 4))
>> +#define RCAR_GYROADC_100MS_ADDED_DATA(ch)	(0x30 + ((ch) * 4))
>> +#define RCAR_GYROADC_10MS_AVG_DATA(ch)		(0x50 + ((ch) * 4))
>> +
>> +#define RCAR_GYROADC_FIFO_STATUS		0x70
>> +#define RCAR_GYROADC_FIFO_STATUS_EMPTY(ch)	BIT(0 + (4 * (ch)))
> 
> FIFO_STATUS_... is not used (yet)

Is it a problem to have a complete register layout here ?

> 4*ch looks suspicious for ch==8??

Well yes, channel is in range 0..7 :)

>> +#define RCAR_GYROADC_FIFO_STATUS_FULL(ch)	BIT(1 + (4 * (ch)))
>> +#define RCAR_GYROADC_FIFO_STATUS_ERROR(ch)	BIT(2 + (4 * (ch)))
>> +
>> +#define RCAR_GYROADC_INTR			0x74
>> +#define RCAR_GYROADC_INTR_INT			BIT(0)
>> +
>> +#define RCAR_GYROADC_INTENR			0x78
>> +#define RCAR_GYROADC_INTENR_INTEN		BIT(0)
>> +
>> +#define RCAR_GYROADC_SAMPLE_RATE		800	/* Hz */

[...]

>> +#define RCAR_GYROADC_CHAN(_idx, _chan_type, _realbits) {	\
>> +	.type			= (_chan_type),			\
> 
> _chan_type is IIO_VOLTAGE always?

Yep, fixed.

>> +	.indexed		= 1,				\
>> +	.channel		= (_idx),			\
>> +	.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW),	\
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
>> +				    BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +	.scan_index		= (_idx),			\
> 
> no buffered mode yet, so strictly no need for a scan_index and scan_type

OK

>> +	.scan_type	= {					\
>> +		.sign		= 'u',				\
>> +		.realbits	= (_realbits),			\
>> +		.storagebits	= 16,				\
>> +	},							\
>> +}
>> +
>> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_1[] = {
>> +	RCAR_GYROADC_CHAN(0, IIO_VOLTAGE, 12),
>> +	RCAR_GYROADC_CHAN(1, IIO_VOLTAGE, 12),
>> +	RCAR_GYROADC_CHAN(2, IIO_VOLTAGE, 12),
>> +	RCAR_GYROADC_CHAN(3, IIO_VOLTAGE, 12),
>> +};
>> +
>> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_2[] = {
>> +	RCAR_GYROADC_CHAN(0, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(1, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(2, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(3, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(4, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(5, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(6, IIO_VOLTAGE, 15),
>> +	RCAR_GYROADC_CHAN(7, IIO_VOLTAGE, 15),
>> +};
>> +
>> +/*
>> + * NOTE: The data we receive in mode 3 from MAX1162 have MSByte = 0,
>> + *       therefore we only use 16bit realbits here instead of 24.
>> + */
>> +static const struct iio_chan_spec rcar_gyroadc_iio_channels_3[] = {
>> +	RCAR_GYROADC_CHAN(0, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(1, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(2, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(3, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(4, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(5, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(6, IIO_VOLTAGE, 16),
>> +	RCAR_GYROADC_CHAN(7, IIO_VOLTAGE, 16),
>> +};
>> +
>> +static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
>> +				 struct iio_chan_spec const *chan,
>> +				 int *val, int *val2, long mask)
>> +{
>> +	struct rcar_gyroadc *priv = iio_priv(indio_dev);
>> +	unsigned int datareg = RCAR_GYROADC_REALTIME_DATA(chan->channel);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (chan->type != IIO_VOLTAGE)
>> +			return -EINVAL;
>> +
>> +		if (iio_buffer_enabled(indio_dev))
>> +			return -EBUSY;
> 
> use iio_device_claim_direct_mode()

Why ? Do I really need the mutex locking here ?

>> +
>> +		*val = readl(priv->regs + datareg);
>> +		*val &= BIT(chan->scan_type.realbits) - 1;
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = 0;
>> +		*val2 = (priv->vref_uv[chan->channel] * 1000) / 0x10000;
>> +		return IIO_VAL_INT_PLUS_NANO;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = RCAR_GYROADC_SAMPLE_RATE;
>> +		*val2 = 0;
> 
> *val2 = 0 not needed

OK

[...]

>> +	indio_dev->name = dev_name(dev);
>> +	indio_dev->dev.parent = dev;
>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>> +	indio_dev->info = &rcar_gyroadc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	if (mode == 1) {
> 
> maybe do the mode differentiation only once, any with a switch?

Done

[...]

-- 
Best regards,
Marek Vasut