On Mon, 23 Apr 2018 00:04:42 +0200
David Veenstra <davidjulianveenstra@xxxxxxxxx> wrote:
> Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
> converter out of staging, into mainline iio subsystems.
>
> Signed-off-by: David Veenstra <davidjulianveenstra@xxxxxxxxx>
A few more minor bits and bobs + suggestions.
Some little things that I missed in earlier patches. If I haven't
taken the relevant patch, please roll the fixes in for V4. If I have
just do a little follow up patch.
Very nearly there!
thanks,
Jonathan
> ---
> Changes in v3:
> - Add mention of ad2s1205 in commit message.
>
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/resolver/Kconfig | 17 +++
> drivers/iio/resolver/Makefile | 5 +
> drivers/iio/resolver/ad2s1200.c | 201 ++++++++++++++++++++++++++++++++
> drivers/staging/iio/resolver/Kconfig | 12 --
> drivers/staging/iio/resolver/Makefile | 1 -
> drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
> 8 files changed, 225 insertions(+), 214 deletions(-)
> create mode 100644 drivers/iio/resolver/Kconfig
> create mode 100644 drivers/iio/resolver/Makefile
> create mode 100644 drivers/iio/resolver/ad2s1200.c
> delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index b3c8c6ef0dff..4bec3ccbf4a1 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -92,6 +92,7 @@ source "drivers/iio/potentiometer/Kconfig"
> source "drivers/iio/potentiostat/Kconfig"
> source "drivers/iio/pressure/Kconfig"
> source "drivers/iio/proximity/Kconfig"
> +source "drivers/iio/resolver/Kconfig"
> source "drivers/iio/temperature/Kconfig"
>
> endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index b16b2e9ddc40..1865361b8714 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -35,5 +35,6 @@ obj-y += potentiometer/
> obj-y += potentiostat/
> obj-y += pressure/
> obj-y += proximity/
> +obj-y += resolver/
> obj-y += temperature/
> obj-y += trigger/
> diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
> new file mode 100644
> index 000000000000..2ced9f22aa70
> --- /dev/null
> +++ b/drivers/iio/resolver/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# Resolver/Synchro drivers
> +#
> +menu "Resolver to digital converters"
> +
> +config AD2S1200
> + tristate "Analog Devices ad2s1200/ad2s1205 driver"
> + depends on SPI
> + depends on GPIOLIB || COMPILE_TEST
> + help
> + Say yes here to build support for Analog Devices spi resolver
> + to digital converters, ad2s1200 and ad2s1205, provides direct access
> + via sysfs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad2s1200.
> +endmenu
> diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
> new file mode 100644
> index 000000000000..4e1dccae07e7
> --- /dev/null
> +++ b/drivers/iio/resolver/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for Resolver/Synchro drivers
> +#
> +
> +obj-$(CONFIG_AD2S1200) += ad2s1200.o
> diff --git a/drivers/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
> new file mode 100644
> index 000000000000..d2b62308b31d
> --- /dev/null
> +++ b/drivers/iio/resolver/ad2s1200.c
> @@ -0,0 +1,201 @@
> +/*
> + * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
> + * AD2S1200/1205
> + *
> + * Copyright (c) 2010-2010 Analog Devices Inc.
I think you have done enough changes in here that if you want to add your
own copyright I think it would be justified.
> + *
> + * 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.
> + *
One of my pet hates if you want to clean it up :)
No point in having a blank line above here.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DRV_NAME "ad2s1200"
> +
> +/* input clock on serial interface */
> +#define AD2S1200_HZ 8192000
> +/* clock period in nano second */
> +#define AD2S1200_TSCLK (1000000000 / AD2S1200_HZ)
> +
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock: protects both the GPIO pins and the rx buffer, to prevent
> + * concurrent spi transactions.
> + * @sdev: spi device.
> + * @sample: GPIO pin SAMPLE.
> + * @rdvel: GPIO pin RDVEL.
> + * @rx: buffer for spi transfers.
> + */
> +struct ad2s1200_state {
> + struct mutex lock;
> + struct spi_device *sdev;
> + struct gpio_desc *sample;
> + struct gpio_desc *rdvel;
> + __be16 rx ____cacheline_aligned;
> +};
> +
> +static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long m)
> +{
> + struct ad2s1200_state *st = iio_priv(indio_dev);
> + int ret = 0;
We only return ret in paths that set it. So doesn't need
initializing any more. This might want to be pushed down
to the earlier patch changing this code.
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_ANGL:
> + /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> + *val = 0;
> + *val2 = 1534355;
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_ANGL_VEL:
> + /* 2 * Pi ~= 6.283185 */
> + *val = 6;
> + *val2 = 283185;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + gpiod_set_value(st->sample, 0);
> +
> + /* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
> + udelay(1);
> + gpiod_set_value(st->sample, 1);
> + gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
> +
> + ret = spi_read(st->sdev, &st->rx, 2);
> + if (ret < 0) {
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> +
> + switch (chan->type) {
> + case IIO_ANGL:
> + *val = be16_to_cpup(&st->rx) >> 4;
> + break;
> + case IIO_ANGL_VEL:
> + *val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> + break;
> + default:
> + mutex_unlock(&st->lock);
> + return -EINVAL;
> + }
> +
> + /* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> + udelay(1);
> + mutex_unlock(&st->lock);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_chan_spec ad2s1200_channels[] = {
> + {
> + .type = IIO_ANGL,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + }, {
> + .type = IIO_ANGL_VEL,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + }
> +};
> +
> +static const struct iio_info ad2s1200_info = {
> + .read_raw = ad2s1200_read_raw,
> +};
> +
> +static int ad2s1200_probe(struct spi_device *spi)
> +{
> + struct ad2s1200_state *st;
> + struct iio_dev *indio_dev;
> + int ret = 0;
I guess this is left from somewhere and should have been in
an earlier patch, but there is no need to set to 0 and
it is set in all paths.
Mind you I've suggested you get rid of the only user
anyway so that won't matter as this will go away anyway.
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, indio_dev);
> + st = iio_priv(indio_dev);
> + mutex_init(&st->lock);
> + st->sdev = spi;
> +
> + st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
> + if (IS_ERR(st->sample)) {
> + dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> + PTR_ERR(st->sample));
> + return PTR_ERR(st->sample);
> + }
> +
> + st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
> + if (IS_ERR(st->rdvel)) {
> + dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
> + PTR_ERR(st->rdvel));
> + return PTR_ERR(st->rdvel);
> + }
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->info = &ad2s1200_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = ad2s1200_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
> + indio_dev->name = spi_get_device_id(spi)->name;
> +
> + ret = devm_iio_device_register(&spi->dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + spi->max_speed_hz = AD2S1200_HZ;
> + spi->mode = SPI_MODE_3;
> + spi_setup(spi);
This ordering is racy. We could in theory have been
talking to the device after it's interfaces were exposed
in the devm_iio_device_register call.
Reorder this to put them before that then just do
return devm_iio_device_register
to finish up probe.
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id ad2s1200_id[] = {
> + { "ad2s1200" },
Worth putting in an explicit device tree table for matching
with the vendor ID. The intent long term is to remove
the nasty code that leads to fall back matching with the
spi_device_id table.
No reason this needs to happen before taking it out of staging.
> + { "ad2s1205" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, ad2s1200_id);
> +
> +static struct spi_driver ad2s1200_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + },
> + .probe = ad2s1200_probe,
> + .id_table = ad2s1200_id,
> +};
> +module_spi_driver(ad2s1200_driver);
> +
> +MODULE_AUTHOR("Graff Yang <graff.yang@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
> +MODULE_LICENSE("GPL v2");
...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
