Re: [[PATCH v2] 2/2] Altera Modular ADC driver support

[Jonathan Cameron]

On 01/09/15 22:49, Chee Nouk Phoo wrote:
> From: Chee Nouk Phoon <cnphoon@xxxxxxxxxx>
> 
> Altera Modular ADC is soft IP that wraps the hardened ADC block in a Max10
> device. It can be configured to dual ADC mode that supports two channel
> synchronous sampling or independent single ADCs. ADC sampled values will be
> written into memory slots in sequence determined by a user configurable
> sequencer block.
> 
> This patch adds Altera Modular ADC driver support
> 
> Signed-off-by: Chee Nouk Phoon <cnphoon@xxxxxxxxxx>
Sorry, I wrote a review of version 1 but for some reason seem not have
sent it.  Anyhow, will comment on this version now.

I'm feeling rather guilty about this as there are a number of elements
in this driver that are major issues and I meant to get back to you on
them over a week ago!  Oops.

Anyhow, big ones in here are unusual uses of the ABI.  One point to note
is that if you ever add any ABI not documented in Documentation/ABI/testing/*
then you MUST document it as part of your patch.  It saves a lot
of time whilst the reviewer figures out what the ABI being presented
actually is.

Dual ADCs should not have their two elements passed out through a single
sysfs attribute.  If you need to maintain their relationship it must
be done by using the buffered interface not the sysfs one.  Doing it
any other way leads to inconsistent and confusing userspace ABI which
would be a disaster.

Extended naming (tied up with the above) has very limited use cases and
must be documented.  It confuses userspace applications by encoding
information in a freeform way within the attribute name. As such the
form must be clearly documented and justified.  The use case here is
not justified.

Anyhow, whilst these sound major, the driver is actually in pretty good
shape and the other stuff is minor tidy ups / formatting suggestions.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/Kconfig           |   11 ++
>  drivers/iio/adc/Makefile          |    1 +
>  drivers/iio/adc/alt_modular_adc.c |  322 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 334 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 drivers/iio/adc/Kconfig
>  create mode 100755 drivers/iio/adc/alt_modular_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> old mode 100644
> new mode 100755
> index 50c103d..e1a67cb
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -118,6 +118,17 @@ config AD799X
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad799x.
>  
> +config ALT_MODULAR_ADC
> +	tristate "Altera Modular ADC driver"
> +	select ANON_INODES
> +
> +	help
> +	  Say yes here to have support for Altera Modular ADC. The driver
> +	  supports both Altera Modular ADC and Altera Modular Dual ADC.
> +
> +	  The driver can also be build as a module. If so the module will be
> +	  called alt_modular_adc.
> +
>  config AT91_ADC
>  	tristate "Atmel AT91 ADC"
>  	depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..d7f10e0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
>  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
>  xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
>  obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
> +obj-$(CONFIG_ALT_MODULAR_ADC) += alt_modular_adc.o
> diff --git a/drivers/iio/adc/alt_modular_adc.c b/drivers/iio/adc/alt_modular_adc.c
> new file mode 100755
> index 0000000..3d2d870
> --- /dev/null
> +++ b/drivers/iio/adc/alt_modular_adc.c
> @@ -0,0 +1,322 @@
> +/*
> + * Copyright (C) 2015 Altera Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Constant Definitions */
Please prefix all constants iwth ALT_MODULAR_ADC_ or similar
to avoid potential name clashes in the future.

> +#define MAX_SLOT		64
> +#define MAX_ADC			2
> +#define MAX_CHANNEL		18
> +#define MODE_SINGLE_ADC		1
> +#define MODE_DUAL_ADC		2
> +#define ADC_BITS		12
> +#define ADC_STORE_BITS		16
> +#define ADC_MAX_STR_SIZE        20
> +
> +/* Register Definitions */
> +#define ADC_CMD_REG		0x0
> +#define ADC_IER_REG		0x100
> +#define ADC_ISR_REG		0x104
> +
> +#define ADC_RUN_MSK		0x1
> +#define ADC_SINGLE_MKS		0x2
> +#define ADC_STOP_MSK		0x0
> +#define ADC_LOW_MSK		0xFFF
> +#define ADC_HIGH_MSK		0xFFF0000
> +
I'd like to see kernel-doc description fo this structure.
It's not immediately obvious what all the parts are!

> +struct altera_adc {
> +	void __iomem		*seq_regs;
> +	void __iomem		*sample_regs;
> +
> +	unsigned int		mode;
> +	unsigned int		slot_count;
> +	unsigned int		slot_sequence[MAX_ADC][MAX_SLOT];
Without looking at the bindings doc, it's not obvious what adc_number.
I'd prefer enough local docs to make that clear!
> +	unsigned int		adc_number;
> +};
> +
> +static int alt_modular_adc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val,
> +				int *val2,
> +				long mask)
> +{
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +	u32 value;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	value = readl_relaxed(adc->sample_regs + (chan->address * 4));
> +
> +	if (adc->mode == MODE_SINGLE_ADC) {
> +		*val = (value & ADC_LOW_MSK);
> +		ret = IIO_VAL_INT;
> +	} else if (adc->mode == MODE_DUAL_ADC) {
> +		*val = (value & ADC_LOW_MSK);
> +		*val2 = ((value & ADC_HIGH_MSK) >> 16);

No.  This is not part of the ABI and is absolutely not the intent.
You can't pass two separate ADC readings from different channels through
the sysfs interface like this.  It's been requested a number of times
but the write way to do this is to implement the buffered interface
and push the out that way.  One sysfs attribute, one value is the rule!
The exceptions are for element that have no meaning whatsoever on their own
(such as quaternions) where any given element has no scale without the rest.
That is not true here.

> +		ret = IIO_VAL_INT_MULTIPLE;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct iio_info adc_iio_info = {
> +	.read_raw = &alt_modular_adc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int alt_modular_adc_channel_init(struct iio_dev *indio_dev)
> +{
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +	struct iio_chan_spec *chan_array;
> +	struct iio_chan_spec *chan;
> +	char **channel_name;
> +	int i;
> +
> +	chan_array = devm_kzalloc(&indio_dev->dev, (adc->slot_count *
> +		sizeof(*chan_array)), GFP_KERNEL);
> +	if (chan_array == NULL)
> +		return -ENOMEM;
> +
> +	channel_name = devm_kzalloc(&indio_dev->dev, adc->slot_count,
> +		GFP_KERNEL);
> +	if (channel_name == NULL)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < adc->slot_count; i++) {
> +		channel_name[i] = devm_kzalloc(&indio_dev->dev,
> +				ADC_MAX_STR_SIZE, GFP_KERNEL);
No need to do this here, use devm_kasprintf as suggested below.
If not I think you could easily have combined this loop with the one
below simplifying the code.

> +		if (channel_name == NULL)
> +			return -ENOMEM;
> +	}
> +
> +	chan = chan_array;
> +	for (i = 0; i < adc->slot_count; i++, chan++) {
> +		chan->type = IIO_VOLTAGE;
> +		chan->indexed = 1;
> +		chan->channel = i;
> +		chan->address = i;
No need to set address if it is always being set to the same
value as channel.  Channel is available everywhere address is so
just use that value.

> +
> +		/* Construct iio sysfs name*/
> +		if (adc->mode == MODE_SINGLE_ADC) {
Use kasprintf and you can skip the earlier allocation of channel_name[i]
plus allocate exactly what is needed.
> +			snprintf(channel_name[i], ADC_MAX_STR_SIZE,
> +				"adc%d-ch%d",
> +				adc->adc_number,
> +				adc->slot_sequence[0][i]);
> +		} else if (adc->mode == MODE_DUAL_ADC) {
> +			snprintf(channel_name[i], ADC_MAX_STR_SIZE,
> +				"adc1-ch%d_adc2-ch%d",
> +				adc->slot_sequence[0][i],
> +				adc->slot_sequence[1][i]);
> +		} else {
> +			return -EINVAL;
> +		}
> +
> +		chan->datasheet_name = channel_name[i];
> +		chan->extend_name = channel_name[i];
You are making use of extend_name in an unusual way.
1) This results in non standard ABI so should be documented under
Documentation/ABI/testing/sysfs-bus-iio*
2) The naming here makes me think you are abusing the abi and pushing two
channels through the same sysfs attribute.  Don't do that.  If you need
to clearly capture two channels together then implement the buffered
interface (chardev reading of 'scans' - groups of channels captured at
a similar time) rather than abusing the simpler sysfs route.
> +		chan->scan_index = i;
You aren't providing buffered access yet so don't bother filling in
scan_index or scan_type fo rhte channels (they are unusued).
> +		chan->scan_type.sign = 'u';
> +		chan->scan_type.realbits = ADC_BITS;
> +		chan->scan_type.storagebits = ADC_STORE_BITS;
> +		chan->scan_type.endianness = IIO_CPU;
> +		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	}
> +
> +	indio_dev->channels = chan_array;
Might as well just use indio_dev->channels directly rather than
having the local variable.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id alt_modular_adc_match[] = {
> +	{ .compatible = "altr,modular-adc-1.0" },
> +	{ .compatible = "altr,modular-dual-adc-1.0" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, alt_modular_adc_match);
> +
> +static int alt_modular_adc_parse_dt(struct iio_dev *indio_dev,
> +				struct device *dev)
> +{
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +	struct device_node *np = dev->of_node;
> +	u32 value;
> +	int ret, i, j;
> +	char str[50];
> +
> +	ret = of_property_read_u32(np, "altr,adc-mode", &value);
> +	if (ret < 0)
> +		return ret;
> +	if (value == 0 || value > MAX_ADC) {
> +		dev_err(dev, "Invalid ADC mode value");
> +		return -EINVAL;
> +	}
> +	adc->mode = value;
> +
> +	ret = of_property_read_u32(np, "altr,adc-slot-count", &value);
> +	if (ret < 0)
> +		return ret;
> +	if (value == 0 || value > MAX_SLOT) {
> +		dev_err(dev, "Invalid ADC slot count value");
> +		return -EINVAL;
> +	}
> +	adc->slot_count = value;
> +
> +	ret = of_property_read_u32(np, "altr,adc-number", &value);
> +	if (ret < 0)
> +		return ret;
> +	if (value == 0 || value > MAX_ADC) {
> +		dev_err(dev, "Invalid ADC number value");
> +		return -EINVAL;
> +	}
> +	adc->adc_number = value;
> +
> +	/* Device tree lookup for channels for each memory slots */
> +	for (j = 0; j < adc->mode; j++) {
> +		for (i = 0; i < adc->slot_count; i++) {
> +			str[0] = '\0';
> +
> +			snprintf(str, sizeof(str),
> +				"altr,adc%d-slot-sequence-%d",
> +				(j + 1), (i + 1));
> +
> +			ret = of_property_read_u32(np, str, &value);
> +			if (ret < 0)
> +				return ret;
> +			if (value > MAX_CHANNEL) {
> +				dev_err(dev, "Invalid ADC channel value");
> +				return -EINVAL;
> +			}
> +			adc->slot_sequence[j][i] = value;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int alt_modular_adc_probe(struct platform_device *pdev)
> +{
> +	struct altera_adc *adc;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	indio_dev = iio_device_alloc(sizeof(struct altera_adc));
Convention has become sizeof(*adc) as it makes the connection slightly
more explicit.

> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	adc = iio_priv(indio_dev);
> +
> +	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"sequencer_csr");
> +	adc->seq_regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(adc->seq_regs)) {
> +		ret = PTR_ERR(adc->seq_regs);
> +		goto err_iio;
> +	}
> +
> +	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"sample_store_csr");
> +	adc->sample_regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(adc->sample_regs)) {
> +		ret = PTR_ERR(adc->sample_regs);
> +		goto err_iio;
> +	}
> +
> +	ret = alt_modular_adc_parse_dt(indio_dev, &pdev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to parse device tree\n");
> +		goto err_iio;
> +	}
> +
> +	ret = alt_modular_adc_channel_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed initialize ADC channels\n");
> +		goto err_iio;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	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 = &adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = adc->slot_count;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err_iio;
> +
> +	/* Disable Interrupt */
> +	writel_relaxed(0, (adc->sample_regs + ADC_IER_REG));
Given we don't seem to handle interrupts anywhere I'd expect the device
to have come up with this disabled, but if it doesn't, this definitely
belongs a lot earlier than here.
> +
> +	/* Start Continuous Sampling */
> +	writel_relaxed((ADC_RUN_MSK), (adc->seq_regs + ADC_CMD_REG));

I'm not sure it makes sense to do either of these last two steps
after the userspace interface is started by the iio_device_register
call.

> +
> +	return 0;
> +
Only 1 blank line here please (pretty much always considered enough
in kernel coding style).
> +
> +err_iio:
> +	iio_device_free(indio_dev);
> +	return ret;
> +}
> +
> +static int alt_modular_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +

If you don't unregister the iio_device first there is a clear
race condition with userspace requests hitting after you've
stopped the ADC but before the interfaces have been removed / marked
as going away by that unregister call.

> +	/* Stop ADC */
> +	writel((ADC_STOP_MSK), (adc->seq_regs + ADC_CMD_REG));

Still  an excess of brackets.  You don't need them around a lot
of these arguments.  We can reasonably assume they don't contain
any malicious elements that could lead to it doing other than the
obvious.
	writel(ADC_STOP_MSK, adc->seq_regs + ADC_CMD_REG);
will be fine (similar cases elsewhere in the driver).

> +
> +	/* Unregister ADC */
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver altr_modular_adc_driver = {
> +	.probe		= alt_modular_adc_probe,
> +	.remove		= alt_modular_adc_remove,
> +	.driver		= {
> +		.name	= "alt-modular-adc",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = alt_modular_adc_match,
> +	},
> +};
> +
> +
> +module_platform_driver(altr_modular_adc_driver);
> +
> +MODULE_DESCRIPTION("Altera Modular ADC Driver");
> +MODULE_AUTHOR("Chee Nouk Phoon <cnphoon@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/