Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630251 > unrolled thread

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2017-04-25 09:40 +0200
    Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-25 11:30 +0200
    Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Jan Kiszka <jan.kiszka@siemens.com> - 2017-04-25 11:40 +0200
      Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-04-25 13:30 +0200
        Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-04-25 14:30 +0200
    Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Jan Kiszka <jan.kiszka@siemens.com> - 2017-04-26 08:00 +0200
      Re: [PATCH] iio: adc: Add support for TI ADC1x8s102 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-26 12:30 +0200

#1630251 — Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Multipart message — attachments visible in raw view] — view raw

> This is an upstream port of an IIO driver for the TI ADC108S102 and
> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
> included.

comments below

naming: don't use placeholders, name after one of the supported chips and 
list them in Kconfig and the driver file

what is the difference between this chip and those supported 
by ti-adc084s021 which was proposed by Mårten Lindahl on April 21?

I think board-specific stuff should not go into the driver -> DT?
 
> Original author: Bogdan Pricop <bogdan.pricop@emutex.com>
> Ported from Intel Galileo Gen2 BSP to Intel Yocto kernel:
> Todor Minchev <todor@minchev.co.uk>.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/iio/adc/Kconfig                  |  12 +
>  drivers/iio/adc/Makefile                 |   1 +
>  drivers/iio/adc/ti-adc1x8s102.c          | 408 +++++++++++++++++++++++++++++++
>  include/linux/platform_data/adc1x8s102.h |  28 +++
>  4 files changed, 449 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-adc1x8s102.c
>  create mode 100644 include/linux/platform_data/adc1x8s102.h
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7adbce9..edb7254a648c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -582,6 +582,18 @@ config TI_ADC128S052
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
>  
> +config TI_ADC1x8S102
> +	tristate "Texas Instruments ADC1x8S102 driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Texas Instruments ADC1x8S102 ADC.
> +	  Provides direct access via sysfs.

drop the 'direct access' statement

> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called ti-adc1x8s102

end with .

> +
>  config TI_ADC161S626
>  	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d0012620cd1c..d5d913bc1263 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> +obj-$(CONFIG_TI_ADC1x8S102) += ti-adc1x8s102.o
>  obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> diff --git a/drivers/iio/adc/ti-adc1x8s102.c b/drivers/iio/adc/ti-adc1x8s102.c
> new file mode 100644
> index 000000000000..4f94c371489d
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc1x8s102.c
> @@ -0,0 +1,408 @@
> +/*
> + * TI ADC1x8S102 SPI ADC driver
> + *
> + * Copyright (c) 2013-2015 Intel Corporation.
> + * Copyright (c) 2017 Siemens AG
> + *
> + * 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.
> + *
> + * This IIO device driver is is designed to work with the following

is is

> + * analog to digital converters from Texas Instruments:
> + *  ADC108S102
> + *  ADC128S102
> + * The communication with ADC chip is via the SPI bus (mode 3).
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/types.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/platform_data/adc1x8s102.h>

who needs platform data these days :)

> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/property.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/spi/pxa2xx_spi.h>

no, this shouldn't be here

> +
> +/*
> + * Defining the ADC resolution being 12 bits, we can use the same driver for
> + * both ADC108S102 (10 bits resolution) and ADC128S102 (12 bits resolution)
> + * chips. The ADC108S102 effectively returns a 12-bit result with the 2
> + * least-significant bits unset.
> + */
> +#define ADC1x8S102_BITS		12
> +#define ADC1x8S102_MAX_CHANNELS	8
> +
> +/* 16-bit SPI command format:
> + *   [15:14] Ignored
> + *   [13:11] 3-bit channel address
> + *   [10:0]  Ignored
> + */
> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))

no need to put parenthesis around 8 and 3
why not shift by 11?

> +
> +/*
> + * 16-bit SPI response format:
> + *   [15:12] Zeros
> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
> + */
> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 << ADC1x8S102_BITS) - 1))

could use GENMASK()

> +
> +#define ADC1x8S102_GALILEO2_CS	8

this board-specific detail shouldn't be here

> +
> +struct adc1x8s102_state {
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	u16				ext_vin;

unit?

> +	/* SPI transfer used by triggered buffer handler*/
> +	struct spi_transfer		ring_xfer;
> +	/* SPI transfer used by direct scan */
> +	struct spi_transfer		scan_single_xfer;
> +	/* SPI message used by ring_xfer SPI transfer */
> +	struct spi_message		ring_msg;
> +	/* SPI message used by scan_single_xfer SPI transfer */
> +	struct spi_message		scan_single_msg;
> +
> +	/* SPI message buffers:

not a proper multi-line comment

> +	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
> +	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
> +	 *
> +	 *  tx_buf: 8 channel read commands, plus 1 dummy command
> +	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-bit timestamp
> +	 */
> +	__be16				rx_buf[13] ____cacheline_aligned;
> +	__be16				tx_buf[9];
> +};
> +
> +#define ADC1X8S102_V_CHAN(index)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = index,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			BIT(IIO_CHAN_INFO_SCALE),			\
> +		.address = index,					\
> +		.scan_index = index,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = ADC1x8S102_BITS,			\

this should be different for the 128 and 108 part, shift missing

most drivers do shifting and don't use _SCALE for that purpose

> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec adc1x8s102_channels[] = {
> +	ADC1X8S102_V_CHAN(0),
> +	ADC1X8S102_V_CHAN(1),
> +	ADC1X8S102_V_CHAN(2),
> +	ADC1X8S102_V_CHAN(3),
> +	ADC1X8S102_V_CHAN(4),
> +	ADC1X8S102_V_CHAN(5),
> +	ADC1X8S102_V_CHAN(6),
> +	ADC1X8S102_V_CHAN(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
> +		unsigned long const *active_scan_mask)
> +{
> +	struct adc1x8s102_state *st;
> +	int i, j;
> +
> +	st = iio_priv(indio_dev);
> +
> +	/* Fill in the first x shorts of tx_buf with the number of channels

not a multi-line comment

> +	 * enabled for sampling by the triggered buffer
> +	 */
> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
> +		if (test_bit(i, active_scan_mask)) {
> +			st->tx_buf[j] = cpu_to_be16(ADC1x8S102_CMD(i));
> +			j++;
> +		}
> +	}
> +	/* One dummy command added, to clock in the last response */
> +	st->tx_buf[j] = 0x00;
> +
> +	/* build SPI ring message */
> +	st->ring_xfer.tx_buf = &st->tx_buf[0];
> +	st->ring_xfer.rx_buf = &st->rx_buf[0];
> +	st->ring_xfer.len = (j + 1) * sizeof(__be16);
> +
> +	spi_message_init(&st->ring_msg);
> +	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t adc1x8s102_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev;
> +	struct adc1x8s102_state *st;
> +	s64 time_ns = 0;

no need to initialize

> +	int b_sent;
> +
> +	indio_dev = pf->indio_dev;
> +	st = iio_priv(indio_dev);
> +
> +	b_sent = spi_sync(st->spi, &st->ring_msg);
> +	if (b_sent == 0) {
> +		if (indio_dev->scan_timestamp) {
> +			time_ns = iio_get_time_ns(indio_dev);
> +			memcpy((u8 *)st->rx_buf + st->ring_xfer.len, &time_ns,
> +			       sizeof(time_ns));
> +		}
> +
> +		/* Skip the dummy response in the first slot */
> +		iio_push_to_buffers(indio_dev, (u8 *)&st->rx_buf[1]);

iio_push_to_buffers_with_timestamp()?

> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adc1x8s102_scan_direct(struct adc1x8s102_state *st, unsigned int ch)
> +{
> +	int ret;
> +
> +	st->tx_buf[0] = cpu_to_be16(ADC1x8S102_CMD(ch));
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		return ret;
> +
> +	/* Skip the dummy response in the first slot */
> +	return be16_to_cpu(st->rx_buf[1]);
> +}
> +
> +static int adc1x8s102_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	int ret;
> +	struct adc1x8s102_state *st;
> +
> +	st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:

iio_device_claim_direct_mode()?

> +		mutex_lock(&indio_dev->mlock);
> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +			ret = -EBUSY;
> +			dev_warn(&st->spi->dev,
> +			 "indio_dev->currentmode is INDIO_BUFFER_TRIGGERED\n");
> +		} else {
> +			ret = adc1x8s102_scan_direct(st, chan->address);
> +		}
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret < 0)
> +			return ret;
> +		*val = ADC1x8S102_RES_DATA(ret);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (st->reg)
> +				*val = regulator_get_voltage(st->reg) / 1000;
> +			else
> +				*val = st->ext_vin;
> +
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			dev_warn(&st->spi->dev,
> +				 "Invalid channel type %u for channel %d\n",
> +				 chan->type, chan->channel);

message necessary?

> +			return -EINVAL;
> +		}
> +	default:
> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO: %lu\n", m);

message necessary?

> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info adc1x8s102_info = {
> +	.read_raw		= &adc1x8s102_read_raw,
> +	.update_scan_mode	= &adc1x8s102_update_scan_mode,
> +	.driver_module		= THIS_MODULE,
> +};
> +
> +#ifdef CONFIG_ACPI
> +typedef int (*acpi_setup_handler)(struct spi_device *,
> +				  const struct adc1x8s102_platform_data **);
> +

no board specific stuff here please

> +static const struct adc1x8s102_platform_data int3495_platform_data = {
> +	.ext_vin = 5000,	/* 5 V */
> +};
> +
> +/* Galileo Gen 2 SPI setup */
> +static int
> +adc1x8s102_setup_int3495(struct spi_device *spi,
> +			 const struct adc1x8s102_platform_data **pdata)
> +{
> +	struct pxa2xx_spi_chip *chip_data;
> +
> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data), GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;
> +
> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
> +	spi->controller_data = chip_data;
> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
> +		 chip_data->gpio_cs);
> +	spi_setup(spi);
> +
> +	*pdata = &int3495_platform_data;
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
> +#endif
> +
> +static int adc1x8s102_probe(struct spi_device *spi)
> +{
> +	const struct adc1x8s102_platform_data *pdata = spi->dev.platform_data;
> +	struct adc1x8s102_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +#ifdef CONFIG_ACPI
> +	if (ACPI_COMPANION(&spi->dev)) {
> +		acpi_setup_handler setup_handler;
> +		const struct acpi_device_id *id;
> +
> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi->dev);
> +		if (!id)
> +			return -ENODEV;
> +
> +		setup_handler = (acpi_setup_handler)id->driver_data;
> +		if (setup_handler) {
> +			ret = setup_handler(spi, &pdata);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +#endif
> +
> +	if (!pdata) {
> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform data\n");
> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->ext_vin = pdata->ext_vin;
> +
> +	/* Use regulator, if available. */
> +	st->reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(st->reg)) {
> +		dev_err(&spi->dev, "Cannot get 'vref' regulator\n");
> +		return PTR_ERR(st->reg);
> +	}
> +	ret = regulator_enable(st->reg);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Cannot enable vref regulator\n");
> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	st->spi = spi;
> +
> +	indio_dev->name = spi->modalias;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = adc1x8s102_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adc1x8s102_channels);
> +	indio_dev->info = &adc1x8s102_info;
> +
> +	/* Setup default message */
> +	st->scan_single_xfer.tx_buf = st->tx_buf;
> +	st->scan_single_xfer.rx_buf = st->rx_buf;
> +	st->scan_single_xfer.len = 2 * sizeof(__be16);
> +	st->scan_single_xfer.cs_change = 0;
> +
> +	spi_message_init(&st->scan_single_msg);
> +	spi_message_add_tail(&st->scan_single_xfer, &st->scan_single_msg);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +			&adc1x8s102_trigger_handler, NULL);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev,
> +			"Failed to register IIO device\n");
> +		goto error_cleanup_ring;
> +	}
> +	return 0;
> +
> +error_cleanup_ring:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +error_disable_reg:
> +	regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +
> +static int adc1x8s102_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id adc1x8s102_id[] = {
> +	{ "adc1x8s102", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adc1x8s102_id);
> +
> +static struct spi_driver adc1x8s102_driver = {
> +	.driver = {
> +		.name   = "adc1x8s102",
> +		.owner	= THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(adc1x8s102_acpi_ids),
> +	},
> +	.probe		= adc1x8s102_probe,
> +	.remove		= adc1x8s102_remove,
> +	.id_table	= adc1x8s102_id,
> +};
> +module_spi_driver(adc1x8s102_driver);
> +
> +MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADC1x8S102 driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/adc1x8s102.h b/include/linux/platform_data/adc1x8s102.h
> new file mode 100644
> index 000000000000..6ad753c99823
> --- /dev/null
> +++ b/include/linux/platform_data/adc1x8s102.h
> @@ -0,0 +1,28 @@
> +/*
> + * ADC1x8S102 SPI ADC driver
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + *
> + * 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.
> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_ADC1x8S102_H__
> +#define __LINUX_PLATFORM_DATA_ADC1x8S102_H__
> +
> +/**
> + * struct adc1x8s102_platform_data - Platform data for the adc1x8s102 ADC driver
> + * @ext_vin: External input voltage range for all voltage input channels
> + *	This is the voltage level of pin VA in millivolts
> + **/
> +struct adc1x8s102_platform_data {
> +	u16  ext_vin;
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_ADC1x8S102_H__ */
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

[toc] | [next] | [standalone]

#1630330

On Tue, Apr 25, 2017 at 10:31 AM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>> included.

> I think board-specific stuff should not go into the driver -> DT?

World is not ARM/DT only -> Unified Device Properties, yes.

P.S. I agree with everything else, though it looks a bit overlapping
with my review, which is a good sign to me :-)

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1630340

On 2017-04-25 09:31, Peter Meerwald-Stadler wrote:
> 
>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>> included.
> 
> comments below
> 
> naming: don't use placeholders, name after one of the supported chips and 
> list them in Kconfig and the driver file

No problem.

> 
> what is the difference between this chip and those supported 
> by ti-adc084s021 which was proposed by Mårten Lindahl on April 21?

I'm not an expert in all those variants, I've "just" adopted the driver
where Intel and Yocto apparently dropped it. But let me try to find out
more.

> 
> I think board-specific stuff should not go into the driver -> DT?

Unfortunately, we only have half-baked ACPI on those boards.

>  
>> Original author: Bogdan Pricop <bogdan.pricop@emutex.com>
>> Ported from Intel Galileo Gen2 BSP to Intel Yocto kernel:
>> Todor Minchev <todor@minchev.co.uk>.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  drivers/iio/adc/Kconfig                  |  12 +
>>  drivers/iio/adc/Makefile                 |   1 +
>>  drivers/iio/adc/ti-adc1x8s102.c          | 408 +++++++++++++++++++++++++++++++
>>  include/linux/platform_data/adc1x8s102.h |  28 +++
>>  4 files changed, 449 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-adc1x8s102.c
>>  create mode 100644 include/linux/platform_data/adc1x8s102.h
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index dedae7adbce9..edb7254a648c 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -582,6 +582,18 @@ config TI_ADC128S052
>>  	  This driver can also be built as a module. If so, the module will be
>>  	  called ti-adc128s052.
>>  
>> +config TI_ADC1x8S102
>> +	tristate "Texas Instruments ADC1x8S102 driver"
>> +	depends on SPI
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>> +	help
>> +	  Say yes here to build support for Texas Instruments ADC1x8S102 ADC.
>> +	  Provides direct access via sysfs.
> 
> drop the 'direct access' statement
> 
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called ti-adc1x8s102
> 
> end with .
> 
>> +
>>  config TI_ADC161S626
>>  	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index d0012620cd1c..d5d913bc1263 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>> +obj-$(CONFIG_TI_ADC1x8S102) += ti-adc1x8s102.o
>>  obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
>> diff --git a/drivers/iio/adc/ti-adc1x8s102.c b/drivers/iio/adc/ti-adc1x8s102.c
>> new file mode 100644
>> index 000000000000..4f94c371489d
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-adc1x8s102.c
>> @@ -0,0 +1,408 @@
>> +/*
>> + * TI ADC1x8S102 SPI ADC driver
>> + *
>> + * Copyright (c) 2013-2015 Intel Corporation.
>> + * Copyright (c) 2017 Siemens AG
>> + *
>> + * 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.
>> + *
>> + * This IIO device driver is is designed to work with the following
> 
> is is
> 
>> + * analog to digital converters from Texas Instruments:
>> + *  ADC108S102
>> + *  ADC128S102
>> + * The communication with ADC chip is via the SPI bus (mode 3).
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/types.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/platform_data/adc1x8s102.h>
> 
> who needs platform data these days :)

Apparently, quite a few devices.

But the situation here is:
- Existing hardware has incomplete ACPI description that needs to be
  augmented by software.
- I'm not aware of a complete DT specification for this device. If
  there is any somewhere, I can happily include it.

> 
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/acpi.h>
>> +#include <linux/property.h>
>> +#include <linux/gpio.h>
>> +
>> +#include <linux/spi/pxa2xx_spi.h>
> 
> no, this shouldn't be here
> 
>> +
>> +/*
>> + * Defining the ADC resolution being 12 bits, we can use the same driver for
>> + * both ADC108S102 (10 bits resolution) and ADC128S102 (12 bits resolution)
>> + * chips. The ADC108S102 effectively returns a 12-bit result with the 2
>> + * least-significant bits unset.
>> + */
>> +#define ADC1x8S102_BITS		12
>> +#define ADC1x8S102_MAX_CHANNELS	8
>> +
>> +/* 16-bit SPI command format:
>> + *   [15:14] Ignored
>> + *   [13:11] 3-bit channel address
>> + *   [10:0]  Ignored
>> + */
>> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))
> 
> no need to put parenthesis around 8 and 3
> why not shift by 11?
> 
>> +
>> +/*
>> + * 16-bit SPI response format:
>> + *   [15:12] Zeros
>> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
>> + */
>> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 << ADC1x8S102_BITS) - 1))
> 
> could use GENMASK()
> 

Both already fixed locally (Andy noted that as well).

>> +
>> +#define ADC1x8S102_GALILEO2_CS	8
> 
> this board-specific detail shouldn't be here

Where should it go then?

> 
>> +
>> +struct adc1x8s102_state {
>> +	struct spi_device		*spi;
>> +	struct regulator		*reg;
>> +	u16				ext_vin;
> 
> unit?

ext_vin_mv?

> 
>> +	/* SPI transfer used by triggered buffer handler*/
>> +	struct spi_transfer		ring_xfer;
>> +	/* SPI transfer used by direct scan */
>> +	struct spi_transfer		scan_single_xfer;
>> +	/* SPI message used by ring_xfer SPI transfer */
>> +	struct spi_message		ring_msg;
>> +	/* SPI message used by scan_single_xfer SPI transfer */
>> +	struct spi_message		scan_single_msg;
>> +
>> +	/* SPI message buffers:
> 
> not a proper multi-line comment

Hmm, checkpatch didn't complain, but your are right.

> 
>> +	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
>> +	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
>> +	 *
>> +	 *  tx_buf: 8 channel read commands, plus 1 dummy command
>> +	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-bit timestamp
>> +	 */
>> +	__be16				rx_buf[13] ____cacheline_aligned;
>> +	__be16				tx_buf[9];
>> +};
>> +
>> +#define ADC1X8S102_V_CHAN(index)					\
>> +	{								\
>> +		.type = IIO_VOLTAGE,					\
>> +		.indexed = 1,						\
>> +		.channel = index,					\
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>> +			BIT(IIO_CHAN_INFO_SCALE),			\
>> +		.address = index,					\
>> +		.scan_index = index,					\
>> +		.scan_type = {						\
>> +			.sign = 'u',					\
>> +			.realbits = ADC1x8S102_BITS,			\
> 
> this should be different for the 128 and 108 part, shift missing
> 
> most drivers do shifting and don't use _SCALE for that purpose

Unfortunately, I cannot test the 128 as we only have the 108 built in. I
adjust blindly, of course.

> 
>> +			.storagebits = 16,				\
>> +			.endianness = IIO_BE,				\
>> +		},							\
>> +	}
>> +
>> +static const struct iio_chan_spec adc1x8s102_channels[] = {
>> +	ADC1X8S102_V_CHAN(0),
>> +	ADC1X8S102_V_CHAN(1),
>> +	ADC1X8S102_V_CHAN(2),
>> +	ADC1X8S102_V_CHAN(3),
>> +	ADC1X8S102_V_CHAN(4),
>> +	ADC1X8S102_V_CHAN(5),
>> +	ADC1X8S102_V_CHAN(6),
>> +	ADC1X8S102_V_CHAN(7),
>> +	IIO_CHAN_SOFT_TIMESTAMP(8),
>> +};
>> +
>> +static int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
>> +		unsigned long const *active_scan_mask)
>> +{
>> +	struct adc1x8s102_state *st;
>> +	int i, j;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	/* Fill in the first x shorts of tx_buf with the number of channels
> 
> not a multi-line comment
> 
>> +	 * enabled for sampling by the triggered buffer
>> +	 */
>> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
>> +		if (test_bit(i, active_scan_mask)) {
>> +			st->tx_buf[j] = cpu_to_be16(ADC1x8S102_CMD(i));
>> +			j++;
>> +		}
>> +	}
>> +	/* One dummy command added, to clock in the last response */
>> +	st->tx_buf[j] = 0x00;
>> +
>> +	/* build SPI ring message */
>> +	st->ring_xfer.tx_buf = &st->tx_buf[0];
>> +	st->ring_xfer.rx_buf = &st->rx_buf[0];
>> +	st->ring_xfer.len = (j + 1) * sizeof(__be16);
>> +
>> +	spi_message_init(&st->ring_msg);
>> +	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t adc1x8s102_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev;
>> +	struct adc1x8s102_state *st;
>> +	s64 time_ns = 0;
> 
> no need to initialize
> 
>> +	int b_sent;
>> +
>> +	indio_dev = pf->indio_dev;
>> +	st = iio_priv(indio_dev);
>> +
>> +	b_sent = spi_sync(st->spi, &st->ring_msg);
>> +	if (b_sent == 0) {
>> +		if (indio_dev->scan_timestamp) {
>> +			time_ns = iio_get_time_ns(indio_dev);
>> +			memcpy((u8 *)st->rx_buf + st->ring_xfer.len, &time_ns,
>> +			       sizeof(time_ns));
>> +		}
>> +
>> +		/* Skip the dummy response in the first slot */
>> +		iio_push_to_buffers(indio_dev, (u8 *)&st->rx_buf[1]);
> 
> iio_push_to_buffers_with_timestamp()?

No idea. If you tell me that this should work, I will put it in.

> 
>> +	}
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int adc1x8s102_scan_direct(struct adc1x8s102_state *st, unsigned int ch)
>> +{
>> +	int ret;
>> +
>> +	st->tx_buf[0] = cpu_to_be16(ADC1x8S102_CMD(ch));
>> +	ret = spi_sync(st->spi, &st->scan_single_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Skip the dummy response in the first slot */
>> +	return be16_to_cpu(st->rx_buf[1]);
>> +}
>> +
>> +static int adc1x8s102_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val,
>> +			   int *val2,
>> +			   long m)
>> +{
>> +	int ret;
>> +	struct adc1x8s102_state *st;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
> 
> iio_device_claim_direct_mode()?

Already found and fixed locally.

> 
>> +		mutex_lock(&indio_dev->mlock);
>> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
>> +			ret = -EBUSY;
>> +			dev_warn(&st->spi->dev,
>> +			 "indio_dev->currentmode is INDIO_BUFFER_TRIGGERED\n");
>> +		} else {
>> +			ret = adc1x8s102_scan_direct(st, chan->address);
>> +		}
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = ADC1x8S102_RES_DATA(ret);
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_VOLTAGE:
>> +			if (st->reg)
>> +				*val = regulator_get_voltage(st->reg) / 1000;
>> +			else
>> +				*val = st->ext_vin;
>> +
>> +			*val2 = chan->scan_type.realbits;
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		default:
>> +			dev_warn(&st->spi->dev,
>> +				 "Invalid channel type %u for channel %d\n",
>> +				 chan->type, chan->channel);
> 
> message necessary?

No idea. The original code contained some "defensive" checks that I
removed. If this case is prevented by the IIO core, I'll drop it.

> 
>> +			return -EINVAL;
>> +		}
>> +	default:
>> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO: %lu\n", m);
> 
> message necessary?

Same here.

> 
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct iio_info adc1x8s102_info = {
>> +	.read_raw		= &adc1x8s102_read_raw,
>> +	.update_scan_mode	= &adc1x8s102_update_scan_mode,
>> +	.driver_module		= THIS_MODULE,
>> +};
>> +
>> +#ifdef CONFIG_ACPI
>> +typedef int (*acpi_setup_handler)(struct spi_device *,
>> +				  const struct adc1x8s102_platform_data **);
>> +
> 
> no board specific stuff here please

Needed to make it work. If there is a better file to keep that, I'll
move it.

> 
>> +static const struct adc1x8s102_platform_data int3495_platform_data = {
>> +	.ext_vin = 5000,	/* 5 V */
>> +};
>> +
>> +/* Galileo Gen 2 SPI setup */
>> +static int
>> +adc1x8s102_setup_int3495(struct spi_device *spi,
>> +			 const struct adc1x8s102_platform_data **pdata)
>> +{
>> +	struct pxa2xx_spi_chip *chip_data;
>> +
>> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data), GFP_KERNEL);
>> +	if (!chip_data)
>> +		return -ENOMEM;
>> +
>> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>> +	spi->controller_data = chip_data;
>> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>> +		 chip_data->gpio_cs);
>> +	spi_setup(spi);
>> +
>> +	*pdata = &int3495_platform_data;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
>> +#endif
>> +
>> +static int adc1x8s102_probe(struct spi_device *spi)
>> +{
>> +	const struct adc1x8s102_platform_data *pdata = spi->dev.platform_data;
>> +	struct adc1x8s102_state *st;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +#ifdef CONFIG_ACPI
>> +	if (ACPI_COMPANION(&spi->dev)) {
>> +		acpi_setup_handler setup_handler;
>> +		const struct acpi_device_id *id;
>> +
>> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi->dev);
>> +		if (!id)
>> +			return -ENODEV;
>> +
>> +		setup_handler = (acpi_setup_handler)id->driver_data;
>> +		if (setup_handler) {
>> +			ret = setup_handler(spi, &pdata);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +#endif
>> +
>> +	if (!pdata) {
>> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform data\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +	st->ext_vin = pdata->ext_vin;
>> +
>> +	/* Use regulator, if available. */
>> +	st->reg = devm_regulator_get(&spi->dev, "vref");
>> +	if (IS_ERR(st->reg)) {
>> +		dev_err(&spi->dev, "Cannot get 'vref' regulator\n");
>> +		return PTR_ERR(st->reg);
>> +	}
>> +	ret = regulator_enable(st->reg);
>> +	if (ret < 0) {
>> +		dev_err(&spi->dev, "Cannot enable vref regulator\n");
>> +		return ret;
>> +	}
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +	st->spi = spi;
>> +
>> +	indio_dev->name = spi->modalias;
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = adc1x8s102_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(adc1x8s102_channels);
>> +	indio_dev->info = &adc1x8s102_info;
>> +
>> +	/* Setup default message */
>> +	st->scan_single_xfer.tx_buf = st->tx_buf;
>> +	st->scan_single_xfer.rx_buf = st->rx_buf;
>> +	st->scan_single_xfer.len = 2 * sizeof(__be16);
>> +	st->scan_single_xfer.cs_change = 0;
>> +
>> +	spi_message_init(&st->scan_single_msg);
>> +	spi_message_add_tail(&st->scan_single_xfer, &st->scan_single_msg);
>> +
>> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +			&adc1x8s102_trigger_handler, NULL);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret) {
>> +		dev_err(&spi->dev,
>> +			"Failed to register IIO device\n");
>> +		goto error_cleanup_ring;
>> +	}
>> +	return 0;
>> +
>> +error_cleanup_ring:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +error_disable_reg:
>> +	regulator_disable(st->reg);
>> +
>> +	return ret;
>> +}
>> +
>> +static int adc1x8s102_remove(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +	regulator_disable(st->reg);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id adc1x8s102_id[] = {
>> +	{ "adc1x8s102", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, adc1x8s102_id);
>> +
>> +static struct spi_driver adc1x8s102_driver = {
>> +	.driver = {
>> +		.name   = "adc1x8s102",
>> +		.owner	= THIS_MODULE,
>> +		.acpi_match_table = ACPI_PTR(adc1x8s102_acpi_ids),
>> +	},
>> +	.probe		= adc1x8s102_probe,
>> +	.remove		= adc1x8s102_remove,
>> +	.id_table	= adc1x8s102_id,
>> +};
>> +module_spi_driver(adc1x8s102_driver);
>> +
>> +MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
>> +MODULE_DESCRIPTION("Texas Instruments ADC1x8S102 driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/platform_data/adc1x8s102.h b/include/linux/platform_data/adc1x8s102.h
>> new file mode 100644
>> index 000000000000..6ad753c99823
>> --- /dev/null
>> +++ b/include/linux/platform_data/adc1x8s102.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * ADC1x8S102 SPI ADC driver
>> + *
>> + * Copyright(c) 2013 Intel Corporation.
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __LINUX_PLATFORM_DATA_ADC1x8S102_H__
>> +#define __LINUX_PLATFORM_DATA_ADC1x8S102_H__
>> +
>> +/**
>> + * struct adc1x8s102_platform_data - Platform data for the adc1x8s102 ADC driver
>> + * @ext_vin: External input voltage range for all voltage input channels
>> + *	This is the voltage level of pin VA in millivolts
>> + **/
>> +struct adc1x8s102_platform_data {
>> +	u16  ext_vin;
>> +};
>> +
>> +#endif /* __LINUX_PLATFORM_DATA_ADC1x8S102_H__ */
>>
> 

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1630422

On Tue, 2017-04-25 at 11:32 +0200, Jan Kiszka wrote:
> On 2017-04-25 09:31, Peter Meerwald-Stadler wrote:

+Cc: Mika.

> > I think board-specific stuff should not go into the driver -> DT?
> 
> Unfortunately, we only have half-baked ACPI on those boards.

That's the main problem and still no excuse for uglifying code.

> > > +
> > > +#include <linux/platform_data/adc1x8s102.h>
> > 
> > who needs platform data these days :)
> 
> Apparently, quite a few devices.

New drivers are not supposed to use platform data.

> But the situation here is:
> - Existing hardware has incomplete ACPI description that needs to be
>   augmented by software.

Yes, and this software is called DSDT table in BIOS. Linux kernel has a
support for properly formed table already for few releases.

> - I'm not aware of a complete DT specification for this device. If
>   there is any somewhere, I can happily include it.

Looking to proposed code there is only one property for it, if there is
an existing binding for the same property you may just simple re-use it.

> > > +
> > > +#define ADC1x8S102_GALILEO2_CS	8
> > 
> > this board-specific detail shouldn't be here
> 
> Where should it go then?

Obviously in (properly formed) ACPI.

> no board specific stuff here please
> 
> Needed to make it work. If there is a better file to keep that, I'll
> move it.

Ideally you need BIOS fixed for that.

Otherwise you may do a separate code which would provide CS GPIO look up
table.

Mika, what do you think about fixing this in the C code for existing
devices?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1630447

On Tue, Apr 25, 2017 at 02:23:48PM +0300, Andy Shevchenko wrote:
> > Needed to make it work. If there is a better file to keep that, I'll
> > move it.
> 
> Ideally you need BIOS fixed for that.
> 
> Otherwise you may do a separate code which would provide CS GPIO look up
> table.
> 
> Mika, what do you think about fixing this in the C code for existing
> devices?

If nothing else helps but I would first try to explore the SSDT overlay
stuff if it can be used here instead.

[toc] | [prev] | [next] | [standalone]

#1631164

On 2017-04-25 09:31, Peter Meerwald-Stadler wrote:
> 
>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>> included.
> 
> comments below
> 
> naming: don't use placeholders, name after one of the supported chips and 
> list them in Kconfig and the driver file
> 
> what is the difference between this chip and those supported 
> by ti-adc084s021 which was proposed by Mårten Lindahl on April 21?

I've cross-read that driver, and it looks fairly different to me.

> 
> I think board-specific stuff should not go into the driver -> DT?

Still looking for suggestions how to provide the external reference
voltage as parameter. Chip select is gone now.

Also, should I suggest a device tree binding even though I have no test
case for it? My current feeling is to better leave this exercise to the
first user on a DT platform.

[...]

>> +
>> +/*
>> + * Defining the ADC resolution being 12 bits, we can use the same driver for
>> + * both ADC108S102 (10 bits resolution) and ADC128S102 (12 bits resolution)
>> + * chips. The ADC108S102 effectively returns a 12-bit result with the 2
>> + * least-significant bits unset.
>> + */
>> +#define ADC1x8S102_BITS		12

[...]

>> +#define ADC1X8S102_V_CHAN(index)					\
>> +	{								\
>> +		.type = IIO_VOLTAGE,					\
>> +		.indexed = 1,						\
>> +		.channel = index,					\
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>> +			BIT(IIO_CHAN_INFO_SCALE),			\
>> +		.address = index,					\
>> +		.scan_index = index,					\
>> +		.scan_type = {						\
>> +			.sign = 'u',					\
>> +			.realbits = ADC1x8S102_BITS,			\
> 
> this should be different for the 128 and 108 part, shift missing
> 
> most drivers do shifting and don't use _SCALE for that purpose

What would be the difference when following your suggestion?

To my understanding, which is based on the comment above, the 108 simply
has its two least significant bits cleared, i.e. it provides a value
with the exact same scale, just with lower resolution.

> 
>> +			.storagebits = 16,				\
>> +			.endianness = IIO_BE,				\
>> +		},							\
>> +	}

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1631353

On Wed, Apr 26, 2017 at 8:37 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-04-25 09:31, Peter Meerwald-Stadler wrote:

>> I think board-specific stuff should not go into the driver -> DT?
>
> Still looking for suggestions how to provide the external reference
> voltage as parameter. Chip select is gone now.

Unified Device Properties API.

Just
ret = device_property_read_u16();
if (ret)
 ...use default...

> Also, should I suggest a device tree binding even though I have no test
> case for it? My current feeling is to better leave this exercise to the
> first user on a DT platform.

But I prefer this one. One problem at a time.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web