Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v4 01/12] iio: Add hardware consumer buffer support
From: Jonathan Cameron @ 2017-11-19 12:32 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-2-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:23 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> From: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> 
> Hardware consumer interface can be used when one IIO device has
> a direct connection to another device in hardware.
> 
> Signed-off-by: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

I think this is fine though it is something we will have to
keep an eye on to make sure it is appropriately used...

Jonathan

> ---
>  drivers/iio/buffer/Kconfig                    |  10 ++
>  drivers/iio/buffer/Makefile                   |   1 +
>  drivers/iio/buffer/industrialio-hw-consumer.c | 182 ++++++++++++++++++++++++++
>  include/linux/iio/hw-consumer.h               |  20 +++
>  4 files changed, 213 insertions(+)
>  create mode 100644 drivers/iio/buffer/industrialio-hw-consumer.c
>  create mode 100644 include/linux/iio/hw-consumer.h
> 
> diff --git a/drivers/iio/buffer/Kconfig b/drivers/iio/buffer/Kconfig
> index 4ffd3db..338774c 100644
> --- a/drivers/iio/buffer/Kconfig
> +++ b/drivers/iio/buffer/Kconfig
> @@ -29,6 +29,16 @@ config IIO_BUFFER_DMAENGINE
>  
>  	  Should be selected by drivers that want to use this functionality.
>  
> +config IIO_BUFFER_HW_CONSUMER
> +	tristate "Industrial I/O HW buffering"
> +	help
> +	  Provides a way to bonding when an IIO device has a direct connection
> +	  to another device in hardware. In this case buffers for data transfers
> +	  are handled by hardware.
> +
> +	  Should be selected by drivers that want to use the generic Hw consumer
> +	  interface.
> +
>  config IIO_KFIFO_BUF
>  	tristate "Industrial I/O buffering based on kfifo"
>  	help
> diff --git a/drivers/iio/buffer/Makefile b/drivers/iio/buffer/Makefile
> index 85beaae..324a36b 100644
> --- a/drivers/iio/buffer/Makefile
> +++ b/drivers/iio/buffer/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_IIO_BUFFER_CB) += industrialio-buffer-cb.o
>  obj-$(CONFIG_IIO_BUFFER_DMA) += industrialio-buffer-dma.o
>  obj-$(CONFIG_IIO_BUFFER_DMAENGINE) += industrialio-buffer-dmaengine.o
> +obj-$(CONFIG_IIO_BUFFER_HW_CONSUMER) += industrialio-hw-consumer.o
>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
> diff --git a/drivers/iio/buffer/industrialio-hw-consumer.c b/drivers/iio/buffer/industrialio-hw-consumer.c
> new file mode 100644
> index 0000000..7d4d800
> --- /dev/null
> +++ b/drivers/iio/buffer/industrialio-hw-consumer.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2017 Analog Devices Inc.
> + *  Author: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/hw-consumer.h>
> +#include <linux/iio/buffer_impl.h>
> +
> +/**
> + * struct iio_hw_consumer - IIO hw consumer block
> + * @buffers: hardware buffers list head.
> + * @channels: IIO provider channels.
> + */
> +struct iio_hw_consumer {
> +	struct list_head buffers;
> +	struct iio_channel *channels;
> +};
> +
> +struct hw_consumer_buffer {
> +	struct list_head head;
> +	struct iio_dev *indio_dev;
> +	struct iio_buffer buffer;
> +	long scan_mask[];
> +};
> +
> +static struct hw_consumer_buffer *iio_buffer_to_hw_consumer_buffer(
> +	struct iio_buffer *buffer)
> +{
> +	return container_of(buffer, struct hw_consumer_buffer, buffer);
> +}
> +
> +static void iio_hw_buf_release(struct iio_buffer *buffer)
> +{
> +	struct hw_consumer_buffer *hw_buf =
> +		iio_buffer_to_hw_consumer_buffer(buffer);
> +	kfree(hw_buf);
> +}
> +
> +static const struct iio_buffer_access_funcs iio_hw_buf_access = {
> +	.release = &iio_hw_buf_release,
> +	.modes = INDIO_BUFFER_HARDWARE,
> +};
> +
> +static struct hw_consumer_buffer *iio_hw_consumer_get_buffer(
> +	struct iio_hw_consumer *hwc, struct iio_dev *indio_dev)
> +{
> +	size_t mask_size = BITS_TO_LONGS(indio_dev->masklength) * sizeof(long);
> +	struct hw_consumer_buffer *buf;
> +
> +	list_for_each_entry(buf, &hwc->buffers, head) {
> +		if (buf->indio_dev == indio_dev)
> +			return buf;
> +	}
> +
> +	buf = kzalloc(sizeof(*buf) + mask_size, GFP_KERNEL);
> +	if (!buf)
> +		return NULL;
> +
> +	buf->buffer.access = &iio_hw_buf_access;
> +	buf->indio_dev = indio_dev;
> +	buf->buffer.scan_mask = buf->scan_mask;
> +
> +	iio_buffer_init(&buf->buffer);
> +	list_add_tail(&buf->head, &hwc->buffers);
> +
> +	return buf;
> +}
> +
> +/**
> + * iio_hw_consumer_alloc() - Allocate IIO hardware consumer
> + * @dev: Pointer to consumer device.
> + *
> + * Returns a valid iio_hw_consumer on success or a ERR_PTR() on failure.
> + */
> +struct iio_hw_consumer *iio_hw_consumer_alloc(struct device *dev)
> +{
> +	struct hw_consumer_buffer *buf;
> +	struct iio_hw_consumer *hwc;
> +	struct iio_channel *chan;
> +	int ret;
> +
> +	hwc = kzalloc(sizeof(*hwc), GFP_KERNEL);
> +	if (!hwc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&hwc->buffers);
> +
> +	hwc->channels = iio_channel_get_all(dev);
> +	if (IS_ERR(hwc->channels)) {
> +		ret = PTR_ERR(hwc->channels);
> +		goto err_free_hwc;
> +	}
> +
> +	chan = &hwc->channels[0];
> +	while (chan->indio_dev) {
> +		buf = iio_hw_consumer_get_buffer(hwc, chan->indio_dev);
> +		if (!buf) {
> +			ret = -ENOMEM;
> +			goto err_put_buffers;
> +		}
> +		set_bit(chan->channel->scan_index, buf->buffer.scan_mask);
> +		chan++;
> +	}
> +
> +	return hwc;
> +
> +err_put_buffers:
> +	list_for_each_entry(buf, &hwc->buffers, head)
> +		iio_buffer_put(&buf->buffer);
> +	iio_channel_release_all(hwc->channels);
> +err_free_hwc:
> +	kfree(hwc);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(iio_hw_consumer_alloc);
> +
> +/**
> + * iio_hw_consumer_free() - Free IIO hardware consumer
> + * @hwc: hw consumer to free.
> + */
> +void iio_hw_consumer_free(struct iio_hw_consumer *hwc)
> +{
> +	struct hw_consumer_buffer *buf;
> +
> +	iio_channel_release_all(hwc->channels);
> +	list_for_each_entry(buf, &hwc->buffers, head)
> +		iio_buffer_put(&buf->buffer);
> +	kfree(hwc);
> +}
> +EXPORT_SYMBOL_GPL(iio_hw_consumer_free);
> +
> +/**
> + * iio_hw_consumer_enable() - Enable IIO hardware consumer
> + * @hwc: iio_hw_consumer to enable.
> + *
> + * Returns 0 on success.
> + */
> +int iio_hw_consumer_enable(struct iio_hw_consumer *hwc)
> +{
> +	struct hw_consumer_buffer *buf;
> +	int ret;
> +
> +	list_for_each_entry(buf, &hwc->buffers, head) {
> +		ret = iio_update_buffers(buf->indio_dev, &buf->buffer, NULL);
> +		if (ret)
> +			goto err_disable_buffers;
> +	}
> +
> +	return 0;
> +
> +err_disable_buffers:
> +	list_for_each_entry_continue_reverse(buf, &hwc->buffers, head)
> +		iio_update_buffers(buf->indio_dev, NULL, &buf->buffer);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_hw_consumer_enable);
> +
> +/**
> + * iio_hw_consumer_disable() - Disable IIO hardware consumer
> + * @hwc: iio_hw_consumer to disable.
> + */
> +void iio_hw_consumer_disable(struct iio_hw_consumer *hwc)
> +{
> +	struct hw_consumer_buffer *buf;
> +
> +	list_for_each_entry(buf, &hwc->buffers, head)
> +		iio_update_buffers(buf->indio_dev, NULL, &buf->buffer);
> +}
> +EXPORT_SYMBOL_GPL(iio_hw_consumer_disable);
> +
> +MODULE_AUTHOR("Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>");
> +MODULE_DESCRIPTION("Hardware consumer buffer the IIO framework");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/iio/hw-consumer.h b/include/linux/iio/hw-consumer.h
> new file mode 100644
> index 0000000..f16791b
> --- /dev/null
> +++ b/include/linux/iio/hw-consumer.h
> @@ -0,0 +1,20 @@
> +/*
> + * Industrial I/O in kernel hardware consumer interface
> + *
> + * Copyright 2017 Analog Devices Inc.
> + *  Author: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#ifndef LINUX_IIO_HW_CONSUMER_H
> +#define LINUX_IIO_HW_CONSUMER_H
> +
> +struct iio_hw_consumer;
> +
> +struct iio_hw_consumer *iio_hw_consumer_alloc(struct device *dev);
> +void iio_hw_consumer_free(struct iio_hw_consumer *hwc);
> +int iio_hw_consumer_enable(struct iio_hw_consumer *hwc);
> +void iio_hw_consumer_disable(struct iio_hw_consumer *hwc);
> +
> +#endif

^ permalink raw reply

* Re: [PATCH v4 03/12] IIO: hw_consumer: add devm_iio_hw_consumer_alloc
From: Jonathan Cameron @ 2017-11-19 12:34 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-4-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:25 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> Add devm_iio_hw_consumer_alloc function that calls iio_hw_consumer_free
> when the device is unbound from the bus.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>
Hmm.. I normally don't like devm for what is a single use case in
a driver (for now) but I guess this is generic enough it will have
additional users reasonably soon.  Hence fine.

Jonathan
> ---
>  drivers/iio/buffer/industrialio-hw-consumer.c | 70 ++++++++++++++++++++++++++-
>  include/linux/iio/hw-consumer.h               |  2 +
>  2 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-hw-consumer.c b/drivers/iio/buffer/industrialio-hw-consumer.c
> index 7d4d800..e980a79 100644
> --- a/drivers/iio/buffer/industrialio-hw-consumer.c
> +++ b/drivers/iio/buffer/industrialio-hw-consumer.c
> @@ -129,15 +129,81 @@ EXPORT_SYMBOL_GPL(iio_hw_consumer_alloc);
>   */
>  void iio_hw_consumer_free(struct iio_hw_consumer *hwc)
>  {
> -	struct hw_consumer_buffer *buf;
> +	struct hw_consumer_buffer *buf, *n;
>  
>  	iio_channel_release_all(hwc->channels);
> -	list_for_each_entry(buf, &hwc->buffers, head)
> +	list_for_each_entry_safe(buf, n, &hwc->buffers, head)
>  		iio_buffer_put(&buf->buffer);

This looks like an unrelated fix.  Push back into the
original patch?

>  	kfree(hwc);
>  }
>  EXPORT_SYMBOL_GPL(iio_hw_consumer_free);
>  
> +static void devm_iio_hw_consumer_release(struct device *dev, void *res)
> +{
> +	iio_hw_consumer_free(*(struct iio_hw_consumer **)res);
> +}
> +
> +static int devm_iio_hw_consumer_match(struct device *dev, void *res, void *data)
> +{
> +	struct iio_hw_consumer **r = res;
> +
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}
> +	return *r == data;
> +}
> +
> +/**
> + * devm_iio_hw_consumer_alloc - Resource-managed iio_hw_consumer_alloc()
> + * @dev: Pointer to consumer device.
> + *
> + * Managed iio_hw_consumer_alloc. iio_hw_consumer allocated with this function
> + * is automatically freed on driver detach.
> + *
> + * If an iio_hw_consumer allocated with this function needs to be freed
> + * separately, devm_iio_hw_consumer_free() must be used.
> + *
> + * returns pointer to allocated iio_hw_consumer on success, NULL on failure.
> + */
> +struct iio_hw_consumer *devm_iio_hw_consumer_alloc(struct device *dev)
> +{
> +	struct iio_hw_consumer **ptr, *iio_hwc;
> +
> +	ptr = devres_alloc(devm_iio_hw_consumer_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	iio_hwc = iio_hw_consumer_alloc(dev);
> +	if (IS_ERR(iio_hwc)) {
> +		devres_free(ptr);
> +	} else {
> +		*ptr = iio_hwc;
> +		devres_add(dev, ptr);
> +	}
> +
> +	return iio_hwc;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_hw_consumer_alloc);
> +
> +/**
> + * devm_iio_hw_consumer_free - Resource-managed iio_hw_consumer_free()
> + * @dev: Pointer to consumer device.
> + * @hwc: iio_hw_consumer to free.
> + *
> + * Free iio_hw_consumer allocated with devm_iio_hw_consumer_alloc().
> + */
> +void devm_iio_hw_consumer_free(struct device *dev, struct iio_hw_consumer *hwc)
> +{
> +	int rc;
> +
> +	rc = devres_release(dev, devm_iio_hw_consumer_release,
> +			    devm_iio_hw_consumer_match, hwc);
> +	WARN_ON(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_hw_consumer_free);
> +
>  /**
>   * iio_hw_consumer_enable() - Enable IIO hardware consumer
>   * @hwc: iio_hw_consumer to enable.
> diff --git a/include/linux/iio/hw-consumer.h b/include/linux/iio/hw-consumer.h
> index f16791b..90ecfce 100644
> --- a/include/linux/iio/hw-consumer.h
> +++ b/include/linux/iio/hw-consumer.h
> @@ -14,6 +14,8 @@ struct iio_hw_consumer;
>  
>  struct iio_hw_consumer *iio_hw_consumer_alloc(struct device *dev);
>  void iio_hw_consumer_free(struct iio_hw_consumer *hwc);
> +struct iio_hw_consumer *devm_iio_hw_consumer_alloc(struct device *dev);
> +void devm_iio_hw_consumer_free(struct device *dev, struct iio_hw_consumer *hwc);
>  int iio_hw_consumer_enable(struct iio_hw_consumer *hwc);
>  void iio_hw_consumer_disable(struct iio_hw_consumer *hwc);
>  

^ permalink raw reply

* Re: [PATCH v4 05/12] IIO: ADC: add sigma delta modulator support
From: Jonathan Cameron @ 2017-11-19 12:44 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-6-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:27 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> Add generic driver to support sigma delta modulators.
A trivial comment inline..

> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>
> ---
>  drivers/iio/adc/Kconfig            | 12 ++++++
>  drivers/iio/adc/Makefile           |  1 +
>  drivers/iio/adc/sd_adc_modulator.c | 82 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+)
>  create mode 100644 drivers/iio/adc/sd_adc_modulator.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5762565..c5db62f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -626,6 +626,18 @@ config SPEAR_ADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called spear_adc.
>  
> +config SD_ADC_MODULATOR
> +	tristate "Generic sigma delta modulator"
> +	depends on OF
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Select this option to enables sigma delta modulator. This driver can
> +	  support generic sigma delta modulators.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called sd_adc_modulator.
> +
>  config STM32_ADC_CORE
>  	tristate "STMicroelectronics STM32 adc core"
>  	depends on ARCH_STM32 || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 9874e05..d800325 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -81,3 +81,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_SD_ADC_MODULATOR) += sd_adc_modulator.o
> diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c
> new file mode 100644
> index 0000000..ff2504a
> --- /dev/null
> +++ b/drivers/iio/adc/sd_adc_modulator.c
> @@ -0,0 +1,82 @@
> +/*
> + * Generic sigma delta modulator driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>.
> + *
> + * License type: GPLv2
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that 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/iio/iio.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +static const struct iio_info iio_sd_mod_iio_info;
> +
> +static const struct iio_chan_spec iio_sd_mod_ch = {
> +	.type = IIO_VOLTAGE,
> +	.indexed = 1,
> +	.scan_index = 0,

Don't specify a scan index until you add buffered support.
Actually scan type isn't usually supplied until then either, but
here it kind of acts as documentation of the odd nature of this
device.

> +	.scan_type = {
> +		.sign = 'u',
> +		.realbits = 1,
> +		.shift = 0,
> +	},
> +};
> +
> +static int iio_sd_mod_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *iio;
> +
> +	iio = devm_iio_device_alloc(dev, 0);
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	iio->dev.parent = dev;
> +	iio->dev.of_node = dev->of_node;
> +	iio->name = dev_name(dev);
> +	iio->info = &iio_sd_mod_iio_info;
> +	iio->modes = INDIO_BUFFER_HARDWARE;
> +
> +	iio->num_channels = 1;
> +	iio->channels = &iio_sd_mod_ch;
> +
> +	platform_set_drvdata(pdev, iio);
> +
> +	return devm_iio_device_register(&pdev->dev, iio);
> +}
> +
> +static const struct of_device_id sd_adc_of_match[] = {
> +	{ .compatible = "sd-modulator" },
> +	{ .compatible = "ads1201" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sd_adc_of_match);
> +
> +static struct platform_driver iio_sd_mod_adc = {
> +	.driver = {
> +		.name = "iio_sd_adc_mod",
> +		.of_match_table = of_match_ptr(sd_adc_of_match),
> +	},
> +	.probe = iio_sd_mod_probe,
> +};
> +
> +module_platform_driver(iio_sd_mod_adc);
> +
> +MODULE_DESCRIPTION("Basic sigma delta modulator");
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 07/12] IIO: ADC: add stm32 DFSDM core support
From: Jonathan Cameron @ 2017-11-19 12:54 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-8-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:29 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> Add driver for stm32 DFSDM pheripheral. Its converts a sigma delta
> stream in n bit samples through a low pass filter and an integrator.
> stm32-dfsdm-core driver is the core part supporting the filter
> instances dedicated to sigma-delta ADC or audio PDM microphone purpose.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>
Looks good to me..  
Reviewed-by: Jonathan Cameron <jonathan.cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

> ---
> V3 -> V4 changes:
> - Patch is split into 2 parts, one dedicated to the core part, another dedicated to
> the ADC part.
> - Filter and channel functions migrated to ADC driver.
> 
>  drivers/iio/adc/Kconfig            |  12 ++
>  drivers/iio/adc/Makefile           |   1 +
>  drivers/iio/adc/stm32-dfsdm-core.c | 318 ++++++++++++++++++++++++++++++++++++
>  drivers/iio/adc/stm32-dfsdm.h      | 319 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 650 insertions(+)
>  create mode 100644 drivers/iio/adc/stm32-dfsdm-core.c
>  create mode 100644 drivers/iio/adc/stm32-dfsdm.h
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index c5db62f..b729ae0 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -665,6 +665,18 @@ config STM32_ADC
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called stm32-adc.
>  
> +config STM32_DFSDM_CORE
> +	tristate "STMicroelectronics STM32 DFSDM core"
> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> +	select REGMAP
> +	select REGMAP_MMIO
> +	help
> +	  Select this option to enable the  driver for STMicroelectronics
> +	  STM32 digital filter for sigma delta converter.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called stm32-dfsdm-core.
> +
>  config STX104
>  	tristate "Apex Embedded Systems STX104 driver"
>  	depends on PC104 && X86 && ISA_BUS_API
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d800325..b52d0a0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_STX104) += stx104.o
>  obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>  obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
>  obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> +obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
> new file mode 100644
> index 0000000..0be5155
> --- /dev/null
> +++ b/drivers/iio/adc/stm32-dfsdm-core.c
> @@ -0,0 +1,318 @@
> +/*
> + * This file is part the core part STM32 DFSDM driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> for STMicroelectronics.
> + *
> + * License terms: GPL V2.0.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include "stm32-dfsdm.h"
> +
> +struct stm32_dfsdm_dev_data {
> +	unsigned int num_filters;
> +	unsigned int num_channels;
> +	const struct regmap_config *regmap_cfg;
> +};
> +
> +#define STM32H7_DFSDM_NUM_FILTERS	4
> +#define STM32H7_DFSDM_NUM_CHANNELS	8
> +
> +static bool stm32_dfsdm_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg < DFSDM_FILTER_BASE_ADR)
> +		return false;
> +
> +	/*
> +	 * Mask is done on register to avoid to list registers of all
> +	 * filter instances.
> +	 */
> +	switch (reg & DFSDM_FILTER_REG_MASK) {
> +	case DFSDM_CR1(0) & DFSDM_FILTER_REG_MASK:
> +	case DFSDM_ISR(0) & DFSDM_FILTER_REG_MASK:
> +	case DFSDM_JDATAR(0) & DFSDM_FILTER_REG_MASK:
> +	case DFSDM_RDATAR(0) & DFSDM_FILTER_REG_MASK:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static const struct regmap_config stm32h7_dfsdm_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = sizeof(u32),
> +	.max_register = 0x2B8,
> +	.volatile_reg = stm32_dfsdm_volatile_reg,
> +	.fast_io = true,
> +};
> +
> +static const struct stm32_dfsdm_dev_data stm32h7_dfsdm_data = {
> +	.num_filters = STM32H7_DFSDM_NUM_FILTERS,
> +	.num_channels = STM32H7_DFSDM_NUM_CHANNELS,
> +	.regmap_cfg = &stm32h7_dfsdm_regmap_cfg,
> +};
> +
> +struct dfsdm_priv {
> +	struct platform_device *pdev; /* platform device */
> +
> +	struct stm32_dfsdm dfsdm; /* common data exported for all instances */
> +
> +	unsigned int spi_clk_out_div; /* SPI clkout divider value */
> +	atomic_t n_active_ch;	/* number of current active channels */
> +
> +	struct clk *clk; /* DFSDM clock */
> +	struct clk *aclk; /* audio clock */
> +};
> +
> +/**
> + * stm32_dfsdm_start_dfsdm - start global dfsdm interface.
> + *
> + * Enable interface if n_active_ch is not null.
> + * @dfsdm: Handle used to retrieve dfsdm context.
> + */
> +int stm32_dfsdm_start_dfsdm(struct stm32_dfsdm *dfsdm)
> +{
> +	struct dfsdm_priv *priv = container_of(dfsdm, struct dfsdm_priv, dfsdm);
> +	struct device *dev = &priv->pdev->dev;
> +	unsigned int clk_div = priv->spi_clk_out_div;
> +	int ret;
> +
> +	if (atomic_inc_return(&priv->n_active_ch) == 1) {
> +		ret = clk_prepare_enable(priv->clk);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to start clock\n");
> +			goto error_ret;
> +		}
> +		if (priv->aclk) {
> +			ret = clk_prepare_enable(priv->aclk);
> +			if (ret < 0) {
> +				dev_err(dev, "Failed to start audio clock\n");
> +				goto disable_clk;
> +			}
> +		}
> +
> +		/* Output the SPI CLKOUT (if clk_div == 0 clock if OFF) */
> +		ret = regmap_update_bits(dfsdm->regmap, DFSDM_CHCFGR1(0),
> +					 DFSDM_CHCFGR1_CKOUTDIV_MASK,
> +					 DFSDM_CHCFGR1_CKOUTDIV(clk_div));
> +		if (ret < 0)
> +			goto disable_aclk;
> +
> +		/* Global enable of DFSDM interface */
> +		ret = regmap_update_bits(dfsdm->regmap, DFSDM_CHCFGR1(0),
> +					 DFSDM_CHCFGR1_DFSDMEN_MASK,
> +					 DFSDM_CHCFGR1_DFSDMEN(1));
> +		if (ret < 0)
> +			goto disable_aclk;
> +	}
> +
> +	dev_dbg(dev, "%s: n_active_ch %d\n", __func__,
> +		atomic_read(&priv->n_active_ch));
> +
> +	return 0;
> +
> +disable_aclk:
> +	clk_disable_unprepare(priv->aclk);
> +disable_clk:
> +	clk_disable_unprepare(priv->clk);
> +
> +error_ret:
> +	atomic_dec(&priv->n_active_ch);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(stm32_dfsdm_start_dfsdm);
> +
> +/**
> + * stm32_dfsdm_stop_dfsdm - stop global DFSDM interface.
> + *
> + * Disable interface if n_active_ch is null
> + * @dfsdm: Handle used to retrieve dfsdm context.
> + */
> +int stm32_dfsdm_stop_dfsdm(struct stm32_dfsdm *dfsdm)
> +{
> +	struct dfsdm_priv *priv = container_of(dfsdm, struct dfsdm_priv, dfsdm);
> +	int ret;
> +
> +	if (atomic_dec_and_test(&priv->n_active_ch)) {
> +		/* Global disable of DFSDM interface */
> +		ret = regmap_update_bits(dfsdm->regmap, DFSDM_CHCFGR1(0),
> +					 DFSDM_CHCFGR1_DFSDMEN_MASK,
> +					 DFSDM_CHCFGR1_DFSDMEN(0));
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Stop SPI CLKOUT */
> +		ret = regmap_update_bits(dfsdm->regmap, DFSDM_CHCFGR1(0),
> +					 DFSDM_CHCFGR1_CKOUTDIV_MASK,
> +					 DFSDM_CHCFGR1_CKOUTDIV(0));
> +		if (ret < 0)
> +			return ret;
> +
> +		clk_disable_unprepare(priv->clk);
> +		if (priv->aclk)
> +			clk_disable_unprepare(priv->aclk);
> +	}
> +	dev_dbg(&priv->pdev->dev, "%s: n_active_ch %d\n", __func__,
> +		atomic_read(&priv->n_active_ch));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(stm32_dfsdm_stop_dfsdm);
> +
> +static int stm32_dfsdm_parse_of(struct platform_device *pdev,
> +				struct dfsdm_priv *priv)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct resource *res;
> +	unsigned long clk_freq;
> +	unsigned int spi_freq, rem;
> +	int ret;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get memory resource\n");
> +		return -ENODEV;
> +	}
> +	priv->dfsdm.phys_base = res->start;
> +	priv->dfsdm.base = devm_ioremap_resource(&pdev->dev, res);
> +
> +	/*
> +	 * "dfsdm" clock is mandatory for DFSDM peripheral clocking.
> +	 * "dfsdm" or "audio" clocks can be used as source clock for
> +	 * the SPI clock out signal and internal processing, depending
> +	 * on use case.
> +	 */
> +	priv->clk = devm_clk_get(&pdev->dev, "dfsdm");
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(&pdev->dev, "No stm32_dfsdm_clk clock found\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->aclk = devm_clk_get(&pdev->dev, "audio");
> +	if (IS_ERR(priv->aclk))
> +		priv->aclk = NULL;
> +
> +	if (priv->aclk)
> +		clk_freq = clk_get_rate(priv->aclk);
> +	else
> +		clk_freq = clk_get_rate(priv->clk);
> +
> +	/* SPI clock out frequency */
> +	ret = of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
> +				   &spi_freq);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to get spi-max-frequency\n");
> +		return ret;
> +	}
> +
> +	priv->spi_clk_out_div = div_u64_rem(clk_freq, spi_freq, &rem) - 1;
> +	priv->dfsdm.spi_master_freq = spi_freq;
> +
> +	if (rem) {
> +		dev_warn(&pdev->dev, "SPI clock not accurate\n");
> +		dev_warn(&pdev->dev, "%ld = %d * %d + %d\n",
> +			 clk_freq, spi_freq, priv->spi_clk_out_div + 1, rem);
> +	}
> +
> +	return 0;
> +};
> +
> +static const struct of_device_id stm32_dfsdm_of_match[] = {
> +	{
> +		.compatible = "st,stm32h7-dfsdm",
> +		.data = &stm32h7_dfsdm_data,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dfsdm_of_match);
> +
> +static int stm32_dfsdm_probe(struct platform_device *pdev)
> +{
> +	struct dfsdm_priv *priv;
> +	struct device_node *pnode = pdev->dev.of_node;
> +	const struct of_device_id *of_id;
> +	const struct stm32_dfsdm_dev_data *dev_data;
> +	struct stm32_dfsdm *dfsdm;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->pdev = pdev;
> +
> +	of_id = of_match_node(stm32_dfsdm_of_match, pnode);
> +	if (!of_id->data) {
> +		dev_err(&pdev->dev, "Data associated to device is missing\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_data = (const struct stm32_dfsdm_dev_data *)of_id->data;
> +	dfsdm = &priv->dfsdm;
> +	dfsdm->fl_list = devm_kcalloc(&pdev->dev, dev_data->num_filters,
> +				      sizeof(*dfsdm->fl_list), GFP_KERNEL);
> +	if (!dfsdm->fl_list)
> +		return -ENOMEM;
> +
> +	dfsdm->num_fls = dev_data->num_filters;
> +	dfsdm->ch_list = devm_kcalloc(&pdev->dev, dev_data->num_channels,
> +				      sizeof(*dfsdm->ch_list),
> +				      GFP_KERNEL);
> +	if (!dfsdm->ch_list)
> +		return -ENOMEM;
> +	dfsdm->num_chs = dev_data->num_channels;
> +
> +	ret = stm32_dfsdm_parse_of(pdev, priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	dfsdm->regmap = devm_regmap_init_mmio(&pdev->dev, dfsdm->base,
> +					      &stm32h7_dfsdm_regmap_cfg);
> +	if (IS_ERR(dfsdm->regmap)) {
> +		ret = PTR_ERR(dfsdm->regmap);
> +		dev_err(&pdev->dev, "%s: Failed to allocate regmap: %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, dfsdm);
> +
> +	return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static struct platform_driver stm32_dfsdm_driver = {
> +	.probe = stm32_dfsdm_probe,
> +	.driver = {
> +		.name = "stm32-dfsdm",
> +		.of_match_table = stm32_dfsdm_of_match,
> +	},
> +};
> +
> +module_platform_driver(stm32_dfsdm_driver);
> +
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 dfsdm driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/adc/stm32-dfsdm.h b/drivers/iio/adc/stm32-dfsdm.h
> new file mode 100644
> index 0000000..9990e8b
> --- /dev/null
> +++ b/drivers/iio/adc/stm32-dfsdm.h
> @@ -0,0 +1,319 @@
> +/*
> + * This file is part of STM32 DFSDM driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>.
> + *
> + * License terms: GPL V2.0.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that 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 MDF_STM32_DFSDM__H
> +#define MDF_STM32_DFSDM__H
> +
> +#include <linux/bitfield.h>
> +
> +/*
> + * STM32 DFSDM - global register map
> + * ________________________________________________________
> + * | Offset |                 Registers block             |
> + * --------------------------------------------------------
> + * | 0x000  |      CHANNEL 0 + COMMON CHANNEL FIELDS      |
> + * --------------------------------------------------------
> + * | 0x020  |                CHANNEL 1                    |
> + * --------------------------------------------------------
> + * | ...    |                .....                        |
> + * --------------------------------------------------------
> + * | 0x0E0  |                CHANNEL 7                    |
> + * --------------------------------------------------------
> + * | 0x100  |      FILTER  0 + COMMON  FILTER FIELDs      |
> + * --------------------------------------------------------
> + * | 0x200  |                FILTER  1                    |
> + * --------------------------------------------------------
> + * | 0x300  |                FILTER  2                    |
> + * --------------------------------------------------------
> + * | 0x400  |                FILTER  3                    |
> + * --------------------------------------------------------
> + */
> +
> +/*
> + * Channels register definitions
> + */
> +#define DFSDM_CHCFGR1(y)  ((y) * 0x20 + 0x00)
> +#define DFSDM_CHCFGR2(y)  ((y) * 0x20 + 0x04)
> +#define DFSDM_AWSCDR(y)   ((y) * 0x20 + 0x08)
> +#define DFSDM_CHWDATR(y)  ((y) * 0x20 + 0x0C)
> +#define DFSDM_CHDATINR(y) ((y) * 0x20 + 0x10)
> +
> +/* CHCFGR1: Channel configuration register 1 */
> +#define DFSDM_CHCFGR1_SITP_MASK     GENMASK(1, 0)
> +#define DFSDM_CHCFGR1_SITP(v)       FIELD_PREP(DFSDM_CHCFGR1_SITP_MASK, v)
> +#define DFSDM_CHCFGR1_SPICKSEL_MASK GENMASK(3, 2)
> +#define DFSDM_CHCFGR1_SPICKSEL(v)   FIELD_PREP(DFSDM_CHCFGR1_SPICKSEL_MASK, v)
> +#define DFSDM_CHCFGR1_SCDEN_MASK    BIT(5)
> +#define DFSDM_CHCFGR1_SCDEN(v)      FIELD_PREP(DFSDM_CHCFGR1_SCDEN_MASK, v)
> +#define DFSDM_CHCFGR1_CKABEN_MASK   BIT(6)
> +#define DFSDM_CHCFGR1_CKABEN(v)     FIELD_PREP(DFSDM_CHCFGR1_CKABEN_MASK, v)
> +#define DFSDM_CHCFGR1_CHEN_MASK     BIT(7)
> +#define DFSDM_CHCFGR1_CHEN(v)       FIELD_PREP(DFSDM_CHCFGR1_CHEN_MASK, v)
> +#define DFSDM_CHCFGR1_CHINSEL_MASK  BIT(8)
> +#define DFSDM_CHCFGR1_CHINSEL(v)    FIELD_PREP(DFSDM_CHCFGR1_CHINSEL_MASK, v)
> +#define DFSDM_CHCFGR1_DATMPX_MASK   GENMASK(13, 12)
> +#define DFSDM_CHCFGR1_DATMPX(v)     FIELD_PREP(DFSDM_CHCFGR1_DATMPX_MASK, v)
> +#define DFSDM_CHCFGR1_DATPACK_MASK  GENMASK(15, 14)
> +#define DFSDM_CHCFGR1_DATPACK(v)    FIELD_PREP(DFSDM_CHCFGR1_DATPACK_MASK, v)
> +#define DFSDM_CHCFGR1_CKOUTDIV_MASK GENMASK(23, 16)
> +#define DFSDM_CHCFGR1_CKOUTDIV(v)   FIELD_PREP(DFSDM_CHCFGR1_CKOUTDIV_MASK, v)
> +#define DFSDM_CHCFGR1_CKOUTSRC_MASK BIT(30)
> +#define DFSDM_CHCFGR1_CKOUTSRC(v)   FIELD_PREP(DFSDM_CHCFGR1_CKOUTSRC_MASK, v)
> +#define DFSDM_CHCFGR1_DFSDMEN_MASK  BIT(31)
> +#define DFSDM_CHCFGR1_DFSDMEN(v)    FIELD_PREP(DFSDM_CHCFGR1_DFSDMEN_MASK, v)
> +
> +/* CHCFGR2: Channel configuration register 2 */
> +#define DFSDM_CHCFGR2_DTRBS_MASK    GENMASK(7, 3)
> +#define DFSDM_CHCFGR2_DTRBS(v)      FIELD_PREP(DFSDM_CHCFGR2_DTRBS_MASK, v)
> +#define DFSDM_CHCFGR2_OFFSET_MASK   GENMASK(31, 8)
> +#define DFSDM_CHCFGR2_OFFSET(v)     FIELD_PREP(DFSDM_CHCFGR2_OFFSET_MASK, v)
> +
> +/* AWSCDR: Channel analog watchdog and short circuit detector */
> +#define DFSDM_AWSCDR_SCDT_MASK    GENMASK(7, 0)
> +#define DFSDM_AWSCDR_SCDT(v)      FIELD_PREP(DFSDM_AWSCDR_SCDT_MASK, v)
> +#define DFSDM_AWSCDR_BKSCD_MASK   GENMASK(15, 12)
> +#define DFSDM_AWSCDR_BKSCD(v)	  FIELD_PREP(DFSDM_AWSCDR_BKSCD_MASK, v)
> +#define DFSDM_AWSCDR_AWFOSR_MASK  GENMASK(20, 16)
> +#define DFSDM_AWSCDR_AWFOSR(v)    FIELD_PREP(DFSDM_AWSCDR_AWFOSR_MASK, v)
> +#define DFSDM_AWSCDR_AWFORD_MASK  GENMASK(23, 22)
> +#define DFSDM_AWSCDR_AWFORD(v)    FIELD_PREP(DFSDM_AWSCDR_AWFORD_MASK, v)
> +
> +/*
> + * Filters register definitions
> + */
> +#define DFSDM_FILTER_BASE_ADR		0x100
> +#define DFSDM_FILTER_REG_MASK		0x7F
> +#define DFSDM_FILTER_X_BASE_ADR(x)	((x) * 0x80 + DFSDM_FILTER_BASE_ADR)
> +
> +#define DFSDM_CR1(x)     (DFSDM_FILTER_X_BASE_ADR(x)  + 0x00)
> +#define DFSDM_CR2(x)     (DFSDM_FILTER_X_BASE_ADR(x)  + 0x04)
> +#define DFSDM_ISR(x)     (DFSDM_FILTER_X_BASE_ADR(x)  + 0x08)
> +#define DFSDM_ICR(x)     (DFSDM_FILTER_X_BASE_ADR(x)  + 0x0C)
> +#define DFSDM_JCHGR(x)   (DFSDM_FILTER_X_BASE_ADR(x)  + 0x10)
> +#define DFSDM_FCR(x)     (DFSDM_FILTER_X_BASE_ADR(x)  + 0x14)
> +#define DFSDM_JDATAR(x)  (DFSDM_FILTER_X_BASE_ADR(x)  + 0x18)
> +#define DFSDM_RDATAR(x)  (DFSDM_FILTER_X_BASE_ADR(x)  + 0x1C)
> +#define DFSDM_AWHTR(x)   (DFSDM_FILTER_X_BASE_ADR(x)  + 0x20)
> +#define DFSDM_AWLTR(x)   (DFSDM_FILTER_X_BASE_ADR(x)  + 0x24)
> +#define DFSDM_AWSR(x)    (DFSDM_FILTER_X_BASE_ADR(x)  + 0x28)
> +#define DFSDM_AWCFR(x)   (DFSDM_FILTER_X_BASE_ADR(x)  + 0x2C)
> +#define DFSDM_EXMAX(x)   (DFSDM_FILTER_X_BASE_ADR(x)  + 0x30)
> +#define DFSDM_EXMIN(x)   (DFSDM_FILTER_X_BASE_ADR(x)  + 0x34)
> +#define DFSDM_CNVTIMR(x) (DFSDM_FILTER_X_BASE_ADR(x)  + 0x38)
> +
> +/* CR1 Control register 1 */
> +#define DFSDM_CR1_DFEN_MASK	BIT(0)
> +#define DFSDM_CR1_DFEN(v)	FIELD_PREP(DFSDM_CR1_DFEN_MASK, v)
> +#define DFSDM_CR1_JSWSTART_MASK	BIT(1)
> +#define DFSDM_CR1_JSWSTART(v)	FIELD_PREP(DFSDM_CR1_JSWSTART_MASK, v)
> +#define DFSDM_CR1_JSYNC_MASK	BIT(3)
> +#define DFSDM_CR1_JSYNC(v)	FIELD_PREP(DFSDM_CR1_JSYNC_MASK, v)
> +#define DFSDM_CR1_JSCAN_MASK	BIT(4)
> +#define DFSDM_CR1_JSCAN(v)	FIELD_PREP(DFSDM_CR1_JSCAN_MASK, v)
> +#define DFSDM_CR1_JDMAEN_MASK	BIT(5)
> +#define DFSDM_CR1_JDMAEN(v)	FIELD_PREP(DFSDM_CR1_JDMAEN_MASK, v)
> +#define DFSDM_CR1_JEXTSEL_MASK	GENMASK(12, 8)
> +#define DFSDM_CR1_JEXTSEL(v)	FIELD_PREP(DFSDM_CR1_JEXTSEL_MASK, v)
> +#define DFSDM_CR1_JEXTEN_MASK	GENMASK(14, 13)
> +#define DFSDM_CR1_JEXTEN(v)	FIELD_PREP(DFSDM_CR1_JEXTEN_MASK, v)
> +#define DFSDM_CR1_RSWSTART_MASK	BIT(17)
> +#define DFSDM_CR1_RSWSTART(v)	FIELD_PREP(DFSDM_CR1_RSWSTART_MASK, v)
> +#define DFSDM_CR1_RCONT_MASK	BIT(18)
> +#define DFSDM_CR1_RCONT(v)	FIELD_PREP(DFSDM_CR1_RCONT_MASK, v)
> +#define DFSDM_CR1_RSYNC_MASK	BIT(19)
> +#define DFSDM_CR1_RSYNC(v)	FIELD_PREP(DFSDM_CR1_RSYNC_MASK, v)
> +#define DFSDM_CR1_RDMAEN_MASK	BIT(21)
> +#define DFSDM_CR1_RDMAEN(v)	FIELD_PREP(DFSDM_CR1_RDMAEN_MASK, v)
> +#define DFSDM_CR1_RCH_MASK	GENMASK(26, 24)
> +#define DFSDM_CR1_RCH(v)	FIELD_PREP(DFSDM_CR1_RCH_MASK, v)
> +#define DFSDM_CR1_FAST_MASK	BIT(29)
> +#define DFSDM_CR1_FAST(v)	FIELD_PREP(DFSDM_CR1_FAST_MASK, v)
> +#define DFSDM_CR1_AWFSEL_MASK	BIT(30)
> +#define DFSDM_CR1_AWFSEL(v)	FIELD_PREP(DFSDM_CR1_AWFSEL_MASK, v)
> +
> +/* CR2: Control register 2 */
> +#define DFSDM_CR2_IE_MASK	GENMASK(6, 0)
> +#define DFSDM_CR2_IE(v)		FIELD_PREP(DFSDM_CR2_IE_MASK, v)
> +#define DFSDM_CR2_JEOCIE_MASK	BIT(0)
> +#define DFSDM_CR2_JEOCIE(v)	FIELD_PREP(DFSDM_CR2_JEOCIE_MASK, v)
> +#define DFSDM_CR2_REOCIE_MASK	BIT(1)
> +#define DFSDM_CR2_REOCIE(v)	FIELD_PREP(DFSDM_CR2_REOCIE_MASK, v)
> +#define DFSDM_CR2_JOVRIE_MASK	BIT(2)
> +#define DFSDM_CR2_JOVRIE(v)	FIELD_PREP(DFSDM_CR2_JOVRIE_MASK, v)
> +#define DFSDM_CR2_ROVRIE_MASK	BIT(3)
> +#define DFSDM_CR2_ROVRIE(v)	FIELD_PREP(DFSDM_CR2_ROVRIE_MASK, v)
> +#define DFSDM_CR2_AWDIE_MASK	BIT(4)
> +#define DFSDM_CR2_AWDIE(v)	FIELD_PREP(DFSDM_CR2_AWDIE_MASK, v)
> +#define DFSDM_CR2_SCDIE_MASK	BIT(5)
> +#define DFSDM_CR2_SCDIE(v)	FIELD_PREP(DFSDM_CR2_SCDIE_MASK, v)
> +#define DFSDM_CR2_CKABIE_MASK	BIT(6)
> +#define DFSDM_CR2_CKABIE(v)	FIELD_PREP(DFSDM_CR2_CKABIE_MASK, v)
> +#define DFSDM_CR2_EXCH_MASK	GENMASK(15, 8)
> +#define DFSDM_CR2_EXCH(v)	FIELD_PREP(DFSDM_CR2_EXCH_MASK, v)
> +#define DFSDM_CR2_AWDCH_MASK	GENMASK(23, 16)
> +#define DFSDM_CR2_AWDCH(v)	FIELD_PREP(DFSDM_CR2_AWDCH_MASK, v)
> +
> +/* ISR: Interrupt status register */
> +#define DFSDM_ISR_JEOCF_MASK	BIT(0)
> +#define DFSDM_ISR_JEOCF(v)	FIELD_PREP(DFSDM_ISR_JEOCF_MASK, v)
> +#define DFSDM_ISR_REOCF_MASK	BIT(1)
> +#define DFSDM_ISR_REOCF(v)	FIELD_PREP(DFSDM_ISR_REOCF_MASK, v)
> +#define DFSDM_ISR_JOVRF_MASK	BIT(2)
> +#define DFSDM_ISR_JOVRF(v)	FIELD_PREP(DFSDM_ISR_JOVRF_MASK, v)
> +#define DFSDM_ISR_ROVRF_MASK	BIT(3)
> +#define DFSDM_ISR_ROVRF(v)	FIELD_PREP(DFSDM_ISR_ROVRF_MASK, v)
> +#define DFSDM_ISR_AWDF_MASK	BIT(4)
> +#define DFSDM_ISR_AWDF(v)	FIELD_PREP(DFSDM_ISR_AWDF_MASK, v)
> +#define DFSDM_ISR_JCIP_MASK	BIT(13)
> +#define DFSDM_ISR_JCIP(v)	FIELD_PREP(DFSDM_ISR_JCIP_MASK, v)
> +#define DFSDM_ISR_RCIP_MASK	BIT(14)
> +#define DFSDM_ISR_RCIP(v)	FIELD_PREP(DFSDM_ISR_RCIP, v)
> +#define DFSDM_ISR_CKABF_MASK	GENMASK(23, 16)
> +#define DFSDM_ISR_CKABF(v)	FIELD_PREP(DFSDM_ISR_CKABF_MASK, v)
> +#define DFSDM_ISR_SCDF_MASK	GENMASK(31, 24)
> +#define DFSDM_ISR_SCDF(v)	FIELD_PREP(DFSDM_ISR_SCDF_MASK, v)
> +
> +/* ICR: Interrupt flag clear register */
> +#define DFSDM_ICR_CLRJOVRF_MASK	      BIT(2)
> +#define DFSDM_ICR_CLRJOVRF(v)	      FIELD_PREP(DFSDM_ICR_CLRJOVRF_MASK, v)
> +#define DFSDM_ICR_CLRROVRF_MASK	      BIT(3)
> +#define DFSDM_ICR_CLRROVRF(v)	      FIELD_PREP(DFSDM_ICR_CLRROVRF_MASK, v)
> +#define DFSDM_ICR_CLRCKABF_MASK	      GENMASK(23, 16)
> +#define DFSDM_ICR_CLRCKABF(v)	      FIELD_PREP(DFSDM_ICR_CLRCKABF_MASK, v)
> +#define DFSDM_ICR_CLRCKABF_CH_MASK(y) BIT(16 + (y))
> +#define DFSDM_ICR_CLRCKABF_CH(v, y)   \
> +			   (((v) << (16 + (y))) & DFSDM_ICR_CLRCKABF_CH_MASK(y))
> +#define DFSDM_ICR_CLRSCDF_MASK	      GENMASK(31, 24)
> +#define DFSDM_ICR_CLRSCDF(v)	      FIELD_PREP(DFSDM_ICR_CLRSCDF_MASK, v)
> +#define DFSDM_ICR_CLRSCDF_CH_MASK(y)  BIT(24 + (y))
> +#define DFSDM_ICR_CLRSCDF_CH(v, y)    \
> +			       (((v) << (24 + (y))) & DFSDM_ICR_CLRSCDF_MASK(y))
> +
> +/* FCR: Filter control register */
> +#define DFSDM_FCR_IOSR_MASK	GENMASK(7, 0)
> +#define DFSDM_FCR_IOSR(v)	FIELD_PREP(DFSDM_FCR_IOSR_MASK, v)
> +#define DFSDM_FCR_FOSR_MASK	GENMASK(25, 16)
> +#define DFSDM_FCR_FOSR(v)	FIELD_PREP(DFSDM_FCR_FOSR_MASK, v)
> +#define DFSDM_FCR_FORD_MASK	GENMASK(31, 29)
> +#define DFSDM_FCR_FORD(v)	FIELD_PREP(DFSDM_FCR_FORD_MASK, v)
> +
> +/* RDATAR: Filter data register for regular channel */
> +#define DFSDM_DATAR_CH_MASK	GENMASK(2, 0)
> +#define DFSDM_DATAR_DATA_OFFSET 8
> +#define DFSDM_DATAR_DATA_MASK	GENMASK(31, DFSDM_DATAR_DATA_OFFSET)
> +
> +/* AWLTR: Filter analog watchdog low threshold register */
> +#define DFSDM_AWLTR_BKAWL_MASK	GENMASK(3, 0)
> +#define DFSDM_AWLTR_BKAWL(v)	FIELD_PREP(DFSDM_AWLTR_BKAWL_MASK, v)
> +#define DFSDM_AWLTR_AWLT_MASK	GENMASK(31, 8)
> +#define DFSDM_AWLTR_AWLT(v)	FIELD_PREP(DFSDM_AWLTR_AWLT_MASK, v)
> +
> +/* AWHTR: Filter analog watchdog low threshold register */
> +#define DFSDM_AWHTR_BKAWH_MASK	GENMASK(3, 0)
> +#define DFSDM_AWHTR_BKAWH(v)	FIELD_PREP(DFSDM_AWHTR_BKAWH_MASK, v)
> +#define DFSDM_AWHTR_AWHT_MASK	GENMASK(31, 8)
> +#define DFSDM_AWHTR_AWHT(v)	FIELD_PREP(DFSDM_AWHTR_AWHT_MASK, v)
> +
> +/* AWSR: Filter watchdog status register */
> +#define DFSDM_AWSR_AWLTF_MASK	GENMASK(7, 0)
> +#define DFSDM_AWSR_AWLTF(v)	FIELD_PREP(DFSDM_AWSR_AWLTF_MASK, v)
> +#define DFSDM_AWSR_AWHTF_MASK	GENMASK(15, 8)
> +#define DFSDM_AWSR_AWHTF(v)	FIELD_PREP(DFSDM_AWSR_AWHTF_MASK, v)
> +
> +/* AWCFR: Filter watchdog status register */
> +#define DFSDM_AWCFR_AWLTF_MASK	GENMASK(7, 0)
> +#define DFSDM_AWCFR_AWLTF(v)	FIELD_PREP(DFSDM_AWCFR_AWLTF_MASK, v)
> +#define DFSDM_AWCFR_AWHTF_MASK	GENMASK(15, 8)
> +#define DFSDM_AWCFR_AWHTF(v)	FIELD_PREP(DFSDM_AWCFR_AWHTF_MASK, v)
> +
> +/* DFSDM filter order  */
> +enum stm32_dfsdm_sinc_order {
> +	DFSDM_FASTSINC_ORDER, /* FastSinc filter type */
> +	DFSDM_SINC1_ORDER,    /* Sinc 1 filter type */
> +	DFSDM_SINC2_ORDER,    /* Sinc 2 filter type */
> +	DFSDM_SINC3_ORDER,    /* Sinc 3 filter type */
> +	DFSDM_SINC4_ORDER,    /* Sinc 4 filter type (N.A. for watchdog) */
> +	DFSDM_SINC5_ORDER,    /* Sinc 5 filter type (N.A. for watchdog) */
> +	DFSDM_NB_SINC_ORDER,
> +};
> +
> +/**
> + * struct stm32_dfsdm_filter - structure relative to stm32 FDSDM filter
> + * @iosr: integrator oversampling
> + * @fosr: filter oversampling
> + * @ford: filter order
> + * @res: output sample resolution
> + * @sync_mode: filter synchronized with filter 0
> + * @fast: filter fast mode
> + */
> +struct stm32_dfsdm_filter {
> +	unsigned int iosr;
> +	unsigned int fosr;
> +	enum stm32_dfsdm_sinc_order ford;
> +	u64 res;
> +	unsigned int sync_mode;
> +	unsigned int fast;
> +};
> +
> +/**
> + * struct stm32_dfsdm_channel - structure relative to stm32 FDSDM channel
> + * @id: id of the channel
> + * @type: interface type linked to stm32_dfsdm_chan_type
> + * @src: interface type linked to stm32_dfsdm_chan_src
> + * @alt_si: alternative serial input interface
> + */
> +struct stm32_dfsdm_channel {
> +	unsigned int id;
> +	unsigned int type;
> +	unsigned int src;
> +	unsigned int alt_si;
> +};
> +
> +/**
> + * struct stm32_dfsdm - stm32 FDSDM driver common data (for all instances)
> + * @base:	control registers base cpu addr
> + * @phys_base:	DFSDM IP register physical address
> + * @regmap:	regmap for register read/write
> + * @fl_list:	filter resources list
> + * @num_fls:	number of filter resources available
> + * @ch_list:	channel resources list
> + * @num_chs:	number of channel resources available
> + * @spi_master_freq: SPI clock out frequency
> + */
> +struct stm32_dfsdm {
> +	void __iomem	*base;
> +	phys_addr_t	phys_base;
> +	struct regmap *regmap;
> +	struct stm32_dfsdm_filter *fl_list;
> +	unsigned int num_fls;
> +	struct stm32_dfsdm_channel *ch_list;
> +	unsigned int num_chs;
> +	unsigned int spi_master_freq;
> +};
> +
> +/* DFSDM channel serial spi clock source */
> +enum stm32_dfsdm_spi_clk_src {
> +	DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL,
> +	DFSDM_CHANNEL_SPI_CLOCK_INTERNAL,
> +	DFSDM_CHANNEL_SPI_CLOCK_INTERNAL_DIV2_FALLING,
> +	DFSDM_CHANNEL_SPI_CLOCK_INTERNAL_DIV2_RISING
> +};
> +
> +int stm32_dfsdm_start_dfsdm(struct stm32_dfsdm *dfsdm);
> +int stm32_dfsdm_stop_dfsdm(struct stm32_dfsdm *dfsdm);
> +
> +#endif

^ permalink raw reply

* Re: [PATCH v4 09/12] IIO: ADC: add stm32 DFSDM support for PDM microphone
From: Jonathan Cameron @ 2017-11-19 14:18 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-10-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:31 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> This code offers a way to handle PDM audio microphones in
> ASOC framework. Audio driver should use consumer API.
> A specific management is implemented for DMA, with a
> callback, to allows to handle audio buffers efficiently.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

A few minor points inline.  I'm not sure I really 'like' the
solution we've ended up with currently but if it works it will
do just fine for now :)

Jonathan


> ---
> V3 -> V4 changes:
>  - Merge audio implementation in stm32-dfsdm-adc.c instead of creating separate file
>  - Add sysfs document for exported attributes
> 
>  .../ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32      |  22 +
>  drivers/iio/adc/stm32-dfsdm-adc.c                  | 517 ++++++++++++++++++++-
>  include/linux/iio/adc/stm32-dfsdm-adc.h            |  27 ++
>  3 files changed, 562 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32
>  create mode 100644 include/linux/iio/adc/stm32-dfsdm-adc.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32
> new file mode 100644
> index 0000000..0ce5508
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32
> @@ -0,0 +1,22 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_audio_sampling_rate
> +KernelVersion:	4.14
> +Contact:	arnaud.pouliquen-qxv4g6HH51o@public.gmane.org
> +Description:
> +		For audio purpose only.
> +		Used by audio driver to set/get the audio sampling rate.
> +		Reading returns current audio sample frequency.
> +		Writing value before starting conversions.

I would like to see a note here on why sampling_frequency can't be used for
this purpose.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_spi_clk_freq
> +KernelVersion:	4.14
> +Contact:	arnaud.pouliquen-qxv4g6HH51o@public.gmane.org
> +Description:
> +		For audio purpose only.
> +		Used by audio driver to set/get the spi input frequency
> +		when DFSDM SPI input channel is in slave mode.
> +		if DFSDM input is SPI master
> +			Reading  0,
> +			error on writing
> +		If DFSDM input is SPI Slave:
> +		Reading returns value previously set.
> +		Writing value before starting conversions.

I'd like a brief note here on why we might change this at runtime.

> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index f9419ab..f0952e26 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -19,11 +19,15 @@
>   * this program. If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/hw-consumer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -32,6 +36,8 @@
>  
>  #include "stm32-dfsdm.h"
>  
> +#define DFSDM_DMA_BUFFER_SIZE (4 * PAGE_SIZE)
> +
>  /* Conversion timeout */
>  #define DFSDM_TIMEOUT_US 100000
>  #define DFSDM_TIMEOUT (msecs_to_jiffies(DFSDM_TIMEOUT_US / 1000))
> @@ -71,6 +77,18 @@ struct stm32_dfsdm_adc {
>  	struct completion completion;
>  	u32 *buffer;
>  
> +	/* Audio specific */
> +	unsigned int spi_freq;  /* SPI bus clock frequency */
> +	unsigned int sample_freq; /* Sample frequency after filter decimation */
> +	int (*cb)(const void *data, size_t size, void *cb_priv);
> +	void *cb_priv;
> +
> +	/* DMA */
> +	u8 *rx_buf;
> +	unsigned int bufi; /* Buffer current position */
> +	unsigned int buf_sz; /* Buffer size */
> +	struct dma_chan	*dma_chan;
> +	dma_addr_t dma_buf;
>  };
>  
>  struct stm32_dfsdm_str2field {
> @@ -364,10 +382,110 @@ int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
>  	return 0;
>  }
>  
> +static ssize_t dfsdm_adc_audio_get_rate(struct iio_dev *indio_dev,
> +					uintptr_t priv,
> +					const struct iio_chan_spec *chan,
> +					char *buf)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", adc->sample_freq);
> +}
> +
> +static ssize_t dfsdm_adc_audio_set_rate(struct iio_dev *indio_dev,
> +					uintptr_t priv,
> +					const struct iio_chan_spec *chan,
> +					const char *buf, size_t len)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> +	struct stm32_dfsdm_channel *ch = &adc->dfsdm->ch_list[adc->ch_id];
> +	unsigned int spi_freq = adc->spi_freq;
> +	unsigned int sample_freq;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 0, &sample_freq);
> +	if (ret)
> +		return ret;
> +	dev_dbg(&indio_dev->dev, "Requested sample_freq :%d\n", sample_freq);
> +
> +	if (!sample_freq)
> +		return -EINVAL;
> +
> +	if (ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
> +		spi_freq = adc->dfsdm->spi_master_freq;
> +
> +	if (spi_freq % sample_freq)
> +		dev_warn(&indio_dev->dev, "Sampling rate not accurate (%d)\n",
> +			 spi_freq / (spi_freq / sample_freq));
> +
> +	ret = stm32_dfsdm_set_osrs(fl, 0, (spi_freq / sample_freq));
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev,
> +			"Not able to find filter parameter that match!\n");
> +		return ret;
> +	}
> +	adc->sample_freq = sample_freq;
> +
> +	return len;
> +}
> +
> +static ssize_t dfsdm_adc_audio_get_spiclk(struct iio_dev *indio_dev,
> +					  uintptr_t priv,
> +					  const struct iio_chan_spec *chan,
> +					  char *buf)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", adc->spi_freq);
> +}
> +
> +static ssize_t dfsdm_adc_audio_set_spiclk(struct iio_dev *indio_dev,
> +					  uintptr_t priv,
> +					  const struct iio_chan_spec *chan,
> +					  const char *buf, size_t len)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> +	struct stm32_dfsdm_channel *ch = &adc->dfsdm->ch_list[adc->ch_id];
> +	unsigned int sample_freq = adc->sample_freq;
> +	unsigned int spi_freq;
> +	int ret;
> +
> +	dev_err(&indio_dev->dev, "enter %s\n", __func__);
> +	/* If DFSDM is master on SPI, SPI freq can not be updated */
> +	if (ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
> +		return -EPERM;
> +
> +	ret = kstrtoint(buf, 0, &spi_freq);
> +	if (ret)
> +		return ret;
> +
> +	if (!spi_freq)
> +		return -EINVAL;
> +
> +	if (sample_freq) {
> +		if (spi_freq % sample_freq)
> +			dev_warn(&indio_dev->dev,
> +				 "Sampling rate not accurate (%d)\n",
> +				 spi_freq / (spi_freq / sample_freq));
> +
> +		ret = stm32_dfsdm_set_osrs(fl, 0, (spi_freq / sample_freq));
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev,
> +				"No filter parameters that match!\n");
> +			return ret;
> +		}
> +	}
> +	adc->spi_freq = spi_freq;
> +
> +	return len;
> +}
blank line here please.

>  static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc, bool dma)
>  {
>  	struct regmap *regmap = adc->dfsdm->regmap;
>  	int ret;
> +	unsigned int dma_en = 0, cont_en = 0;
>  
>  	ret = stm32_dfsdm_start_channel(adc->dfsdm, adc->ch_id);
>  	if (ret < 0)
> @@ -378,6 +496,24 @@ static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc, bool dma)
>  	if (ret < 0)
>  		goto stop_channels;
>  
> +	if (dma) {
> +		/* Enable DMA transfer*/
> +		dma_en =  DFSDM_CR1_RDMAEN(1);
> +		/* Enable conversion triggered by SPI clock*/
> +		cont_en = DFSDM_CR1_RCONT(1);
> +	}
> +	/* Enable DMA transfer*/
> +	ret = regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> +				 DFSDM_CR1_RDMAEN_MASK, dma_en);
> +	if (ret < 0)
> +		goto stop_channels;
> +
> +	/* Enable conversion triggered by SPI clock*/
> +	ret = regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> +				 DFSDM_CR1_RCONT_MASK, cont_en);
> +	if (ret < 0)
> +		goto stop_channels;
> +
>  	ret = stm32_dfsdm_start_filter(adc->dfsdm, adc->fl_id);
>  	if (ret < 0)
>  		goto stop_channels;
> @@ -411,6 +547,241 @@ static void stm32_dfsdm_stop_conv(struct stm32_dfsdm_adc *adc)
>  	stm32_dfsdm_stop_channel(adc->dfsdm, adc->ch_id);
>  }
>  
> +static int stm32_dfsdm_set_watermark(struct iio_dev *indio_dev,
> +				     unsigned int val)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	unsigned int watermark = DFSDM_DMA_BUFFER_SIZE / 2;
> +
> +	/*
> +	 * DMA cyclic transfers are used, buffer is split into two periods.
> +	 * There should be :
> +	 * - always one buffer (period) DMA is working on
> +	 * - one buffer (period) driver pushed to ASoC side.
> +	 */
> +	watermark = min(watermark, val * (unsigned int)(sizeof(u32)));
> +	adc->buf_sz = watermark * 2;
> +
> +	return 0;
> +}
> +
> +static const struct iio_info stm32_dfsdm_info_audio = {
> +	.hwfifo_set_watermark = stm32_dfsdm_set_watermark,
> +	.driver_module = THIS_MODULE,

This has gone - now handled by macro magic...

> +};
> +
> +static unsigned int stm32_dfsdm_adc_dma_residue(struct stm32_dfsdm_adc *adc)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +
> +	status = dmaengine_tx_status(adc->dma_chan,
> +				     adc->dma_chan->cookie,
> +				     &state);
> +	if (status == DMA_IN_PROGRESS) {
> +		/* Residue is size in bytes from end of buffer */
> +		unsigned int i = adc->buf_sz - state.residue;
> +		unsigned int size;
> +
> +		/* Return available bytes */
> +		if (i >= adc->bufi)
> +			size = i - adc->bufi;
> +		else
> +			size = adc->buf_sz + i - adc->bufi;
> +
> +		return size;
> +	}
> +
> +	return 0;
> +}
> +
> +static void stm32_dfsdm_audio_dma_buffer_done(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int available = stm32_dfsdm_adc_dma_residue(adc);
> +	size_t old_pos;
> +
> +	/*
> +	 * FIXME: Buffer interface does not support cyclic DMA buffer,and offers
> +	 * only an interface to push data samples per samples.
> +	 * For this reason IIO buffer interface is not used and interface is
> +	 * bypassed using a private callback registered by ASoC.
> +	 * This should be a temporary solution waiting a cyclic DMA engine
> +	 * support in IIO.
More specifically the in kernel interfaces don't support dma cyclic
buffers.  We can already do them to userspace.  Doesn't really matter
though!

> +	 */
> +
> +	dev_dbg(&indio_dev->dev, "%s: pos = %d, available = %d\n", __func__,
> +		adc->bufi, available);
> +	old_pos = adc->bufi;
> +
> +	while (available >= indio_dev->scan_bytes) {
> +		u32 *buffer = (u32 *)&adc->rx_buf[adc->bufi];
> +
> +		/* Mask 8 LSB that contains the channel ID */
> +		*buffer = (*buffer & 0xFFFFFF00) << 8;
> +		available -= indio_dev->scan_bytes;
> +		adc->bufi += indio_dev->scan_bytes;
> +		if (adc->bufi >= adc->buf_sz) {
> +			if (adc->cb)
> +				adc->cb(&adc->rx_buf[old_pos],
> +					 adc->buf_sz - old_pos, adc->cb_priv);
> +			adc->bufi = 0;
> +			old_pos = 0;
> +		}
> +	}
> +	if (adc->cb)
> +		adc->cb(&adc->rx_buf[old_pos], adc->bufi - old_pos,
> +				adc->cb_priv);
> +}
> +
> +static int stm32_dfsdm_adc_dma_start(struct iio_dev *indio_dev)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct dma_async_tx_descriptor *desc;
> +	dma_cookie_t cookie;
> +	int ret;
> +
> +	if (!adc->dma_chan)
> +		return -EINVAL;
> +
> +	dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__,
> +		adc->buf_sz, adc->buf_sz / 2);
> +
> +	/* Prepare a DMA cyclic transaction */
> +	desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
> +					 adc->dma_buf,
> +					 adc->buf_sz, adc->buf_sz / 2,
> +					 DMA_DEV_TO_MEM,
> +					 DMA_PREP_INTERRUPT);
> +	if (!desc)
> +		return -EBUSY;
> +
> +	desc->callback = stm32_dfsdm_audio_dma_buffer_done;
> +	desc->callback_param = indio_dev;
> +
> +	cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(cookie);
> +	if (ret) {
> +		dmaengine_terminate_all(adc->dma_chan);
> +		return ret;
> +	}
> +
> +	/* Issue pending DMA requests */
> +	dma_async_issue_pending(adc->dma_chan);
> +
> +	return 0;
> +}
> +
> +static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Reset adc buffer index */
> +	adc->bufi = 0;
> +
> +	ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = stm32_dfsdm_start_conv(adc, true);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Can't start conversion\n");
> +		goto stop_dfsdm;
> +	}
> +
> +	if (adc->dma_chan) {
> +		ret = stm32_dfsdm_adc_dma_start(indio_dev);
> +		if (ret) {
> +			dev_err(&indio_dev->dev, "Can't start DMA\n");
> +			goto err_stop_conv;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_stop_conv:
> +	stm32_dfsdm_stop_conv(adc);
> +stop_dfsdm:
> +	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
> +
> +	return ret;
> +}
> +
> +static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +
> +	if (adc->dma_chan)
> +		dmaengine_terminate_all(adc->dma_chan);
> +
> +	stm32_dfsdm_stop_conv(adc);
> +
> +	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops stm32_dfsdm_buffer_setup_ops = {
> +	.postenable = &stm32_dfsdm_postenable,
> +	.predisable = &stm32_dfsdm_predisable,
> +};
> +
> +/**
> + * stm32_dfsdm_get_buff_cb - register a callback
> + *	that will be called when DMA transfer period is achieved.
Please run kernel-doc over this file. I'm fairly sure this isn't
quite meeting the spec...

> + *
> + * @iio_dev: Handle to IIO device.
> + * @cb: pointer to callback function.
> + *	@data: pointer to data buffer
> + *	@size: size in byte of the data buffer
> + *	@private: pointer to consumer private structure
> + * @private: pointer to consumer private structure
> + */
> +int stm32_dfsdm_get_buff_cb(struct iio_dev *iio_dev,
> +			    int (*cb)(const void *data, size_t size,
> +				      void *private),
> +			    void *private)
> +{
> +	struct stm32_dfsdm_adc *adc;
> +
> +	if (!iio_dev)
> +		return -EINVAL;
> +	adc = iio_priv(iio_dev);
> +
> +	if (iio_dev !=  iio_priv_to_dev(adc))
> +		return -EINVAL;
Same as for the case below..

> +
> +	adc->cb = cb;
> +	adc->cb_priv = private;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(stm32_dfsdm_get_buff_cb);
> +
> +/**
> + * stm32_dfsdm_release_buff_cb - unregister buffer callback
> + *
> + * @iio_dev: Handle to IIO device.
> + */
> +int stm32_dfsdm_release_buff_cb(struct iio_dev *iio_dev)
> +{
> +	struct stm32_dfsdm_adc *adc;
> +
> +	if (!iio_dev)
> +		return -EINVAL;
> +	adc = iio_priv(iio_dev);
> +
> +	if (iio_dev !=  iio_priv_to_dev(adc))
> +		return -EINVAL;
I can't immediately spot why this could ever fail so we definitely
need a comment here saying what it is protecting against!

> +	adc->cb = NULL;
> +	adc->cb_priv = NULL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(stm32_dfsdm_release_buff_cb);
> +
>  static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
>  				   const struct iio_chan_spec *chan, int *res)
>  {
> @@ -544,6 +915,67 @@ static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * Define external info for SPI Frequency and audio sampling rate that can be
> + * configured by ASoC driver through consumer.h API
> + */
> +static const struct iio_chan_spec_ext_info dfsdm_adc_audio_ext_info[] = {
> +	/* filter oversampling: Post filter oversampling ratio */
> +	{
> +		.name = "audio_sampling_rate",
> +		.shared = IIO_SHARED_BY_TYPE,
> +		.read = dfsdm_adc_audio_get_rate,
> +		.write = dfsdm_adc_audio_set_rate,
> +	},
> +	/* data_right_bit_shift : Filter output data shifting */
> +	{
> +		.name = "spi_clk_freq",
> +		.shared = IIO_SHARED_BY_TYPE,
> +		.read = dfsdm_adc_audio_get_spiclk,
> +		.write = dfsdm_adc_audio_set_spiclk,
> +	},
> +	{},
> +};
> +
> +static int stm32_dfsdm_dma_request(struct iio_dev *indio_dev)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct dma_slave_config config;
> +	int ret;
> +
> +	adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
> +	if (!adc->dma_chan)
> +		return -EINVAL;
> +
> +	adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
> +					 DFSDM_DMA_BUFFER_SIZE,
> +					 &adc->dma_buf, GFP_KERNEL);
> +	if (!adc->rx_buf) {
> +		ret = -ENOMEM;
> +		goto err_release;
> +	}
> +
> +	/* Configure DMA channel to read data register */
> +	memset(&config, 0, sizeof(config));
> +	config.src_addr = (dma_addr_t)adc->dfsdm->phys_base;
> +	config.src_addr += DFSDM_RDATAR(adc->fl_id);
> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> +	ret = dmaengine_slave_config(adc->dma_chan, &config);
> +	if (ret)
> +		goto err_free;
> +
> +	return 0;
> +
> +err_free:
> +	dma_free_coherent(adc->dma_chan->device->dev, DFSDM_DMA_BUFFER_SIZE,
> +			  adc->rx_buf, adc->dma_buf);
> +err_release:
> +	dma_release_channel(adc->dma_chan);
> +
> +	return ret;
> +}
> +
>  static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>  					 struct iio_chan_spec *ch)
>  {
> @@ -564,7 +996,12 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>  	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
>  
> -	ch->scan_type.sign = 'u';
> +	if (adc->dev_data->type == DFSDM_AUDIO) {
> +		ch->scan_type.sign = 's';
> +		ch->ext_info = dfsdm_adc_audio_ext_info;
> +	} else {
> +		ch->scan_type.sign = 'u';
> +	}
>  	ch->scan_type.realbits = 24;
>  	ch->scan_type.storagebits = 32;
>  	adc->ch_id = ch->channel;
> @@ -573,6 +1010,58 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
>  					  &adc->dfsdm->ch_list[ch->channel]);
>  }
>  
> +static int stm32_dfsdm_audio_init(struct iio_dev *indio_dev)
> +{
> +	struct iio_chan_spec *ch;
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = stm32_dfsdm_dma_request(indio_dev);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "DMA request failed\n");
> +		return ret;
> +	}
> +
> +	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> +
> +	ret = iio_triggered_buffer_setup(indio_dev,
> +					 &iio_pollfunc_store_time,
> +					 NULL,
> +					 &stm32_dfsdm_buffer_setup_ops);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Buffer setup failed\n");
> +		goto err_dma_disable;
> +	}
> +
> +	ch = devm_kzalloc(&indio_dev->dev, sizeof(*ch), GFP_KERNEL);
> +	if (!ch)
> +		return -ENOMEM;
> +
> +	ch->scan_index = 0;
> +	ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "channels init failed\n");
> +		goto err_buffer_cleanup;
> +	}
> +
> +	indio_dev->num_channels = 1;
> +	indio_dev->channels = ch;
> +
> +	return 0;
> +
> +err_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +err_dma_disable:
> +	if (adc->dma_chan) {
> +		dma_free_coherent(adc->dma_chan->device->dev,
> +				  DFSDM_DMA_BUFFER_SIZE,
> +				  adc->rx_buf, adc->dma_buf);
> +		dma_release_channel(adc->dma_chan);
> +	}
> +	return ret;
> +}
> +
>  static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
>  {
>  	struct iio_chan_spec *ch;
> @@ -625,10 +1114,18 @@ static const struct stm32_dfsdm_dev_data stm32h7_dfsdm_adc_data = {
>  	.init = stm32_dfsdm_adc_init,
>  };
>  
> +static const struct stm32_dfsdm_dev_data stm32h7_dfsdm_audio_data = {
> +	.type = DFSDM_AUDIO,
> +	.init = stm32_dfsdm_audio_init,
> +};
> +
>  static const struct of_device_id stm32_dfsdm_adc_match[] = {
>  	{ .compatible = "st,stm32-dfsdm-adc",
>  		.data = &stm32h7_dfsdm_adc_data,
>  	},
> +	{ .compatible = "st,stm32-dfsdm-dmic",
> +		.data = &stm32h7_dfsdm_audio_data,
> +	},
>  	{}
>  };
>  
> @@ -679,8 +1176,13 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
>  	name = devm_kzalloc(dev, sizeof("dfsdm-adc0"), GFP_KERNEL);
>  	if (!name)
>  		return -ENOMEM;
> -	iio->info = &stm32_dfsdm_info_adc;
> -	snprintf(name, sizeof("dfsdm-adc0"), "dfsdm-adc%d", adc->fl_id);
> +	if (dev_data->type == DFSDM_AUDIO) {
> +		iio->info = &stm32_dfsdm_info_audio;
> +		snprintf(name, sizeof("dfsdm-pdm0"), "dfsdm-pdm%d", adc->fl_id);
> +	} else {
> +		iio->info = &stm32_dfsdm_info_adc;
> +		snprintf(name, sizeof("dfsdm-adc0"), "dfsdm-adc%d", adc->fl_id);
> +	}
>  	iio->name = name;
>  
>  	/*
> @@ -721,7 +1223,14 @@ static int stm32_dfsdm_adc_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
>  
>  	iio_device_unregister(indio_dev);
> -
> +	if (indio_dev->pollfunc)
> +		iio_triggered_buffer_cleanup(indio_dev);
> +	if (adc->dma_chan) {
> +		dma_free_coherent(adc->dma_chan->device->dev,
> +				  DFSDM_DMA_BUFFER_SIZE,
> +				  adc->rx_buf, adc->dma_buf);
> +		dma_release_channel(adc->dma_chan);
> +	}
>  	return 0;
>  }
>  
> diff --git a/include/linux/iio/adc/stm32-dfsdm-adc.h b/include/linux/iio/adc/stm32-dfsdm-adc.h
> new file mode 100644
> index 0000000..6885645
> --- /dev/null
> +++ b/include/linux/iio/adc/stm32-dfsdm-adc.h
> @@ -0,0 +1,27 @@
> +/*
> + * This file discribe the STM32 DFSDM IIO driver API for audio part
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>.
> + *
> + * License terms: GPL V2.0.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that 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 STM32_DFSDM_ADC_H
> +#define STM32_DFSDM_ADC_H
> +
> +int stm32_dfsdm_get_buff_cb(struct iio_dev *iio_dev,
> +			    int (*cb)(const void *data, size_t size,
> +				      void *private),
> +			    void *private);
> +int stm32_dfsdm_release_buff_cb(struct iio_dev *iio_dev);
> +
> +#endif

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 10/12] IIO: consumer: allow to set buffer sizes
From: Jonathan Cameron @ 2017-11-19 14:19 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-11-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:32 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> Add iio consumer API to set buffer size and watermark according
> to sysfs API.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> V3 -> V4 changes:
>  - Set only the Watermark not the buffer lenght
>  - Rename functions to replace "params" with "watermark"
> 
>  drivers/iio/buffer/industrialio-buffer-cb.c | 11 +++++++++++
>  include/linux/iio/consumer.h                | 11 +++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
> index 4847534..ea63c83 100644
> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> @@ -104,6 +104,17 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_get_all_cb);
>  
> +int iio_channel_cb_set_buffer_watermark(struct iio_cb_buffer *cb_buff,
> +					size_t watermark)
> +{
> +	if (!watermark)
> +		return -EINVAL;
> +	cb_buff->buffer.watermark = watermark;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_channel_cb_set_buffer_watermark);
> +
>  int iio_channel_start_all_cb(struct iio_cb_buffer *cb_buff)
>  {
>  	return iio_update_buffers(cb_buff->indio_dev, &cb_buff->buffer,
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 5e347a9..0d94557 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -134,6 +134,17 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  						       void *private),
>  					     void *private);
>  /**
> + * iio_channel_cb_set_buffer_watermark() - set the buffer watermark.
> + * @cb_buffer:		The callback buffer from whom we want the channel
> + *			information.
> + * @watermark: buffer watermark in bytes.
> + *
> + * This function allows to configure the buffer watermark.
> + */
> +int iio_channel_cb_set_buffer_watermark(struct iio_cb_buffer *cb_buffer,
> +					size_t watermark);
> +
> +/**
>   * iio_channel_release_all_cb() - release and unregister the callback.
>   * @cb_buffer:		The callback buffer that was allocated.
>   */

^ permalink raw reply

* Re: [PATCH v4 08/12] IIO: ADC: add STM32 DFSDM sigma delta ADC support
From: Jonathan Cameron @ 2017-11-19 14:29 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Rob Herring, Mark Rutland, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jaroslav Kysela, Takashi Iwai,
	Liam Girdwood, Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Maxime Coquelin,
	Alexandre Torgue
In-Reply-To: <1510222354-15290-9-git-send-email-arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>

On Thu, 9 Nov 2017 11:12:30 +0100
Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org> wrote:

> Add DFSDM driver to handle sigma delta ADC.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>
> ---
>  drivers/iio/adc/Kconfig           |  13 +
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/stm32-dfsdm-adc.c | 741 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 755 insertions(+)
>  create mode 100644 drivers/iio/adc/stm32-dfsdm-adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index b729ae0..98ca30b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -677,6 +677,19 @@ config STM32_DFSDM_CORE
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called stm32-dfsdm-core.
>  
> +config STM32_DFSDM_ADC
> +	tristate "STMicroelectronics STM32 dfsdm adc"
> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> +	select STM32_DFSDM_CORE
> +	select REGMAP_MMIO
> +	select IIO_BUFFER_HW_CONSUMER
> +	help
> +	  Select this option to support ADCSigma delta modulator for
> +	  STMicroelectronics STM32 digital filter for sigma delta converter.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called stm32-dfsdm-adc.
> +
>  config STX104
>  	tristate "Apex Embedded Systems STX104 driver"
>  	depends on PC104 && X86 && ISA_BUS_API
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index b52d0a0..c4f5d15 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
>  obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
>  obj-$(CONFIG_STM32_ADC) += stm32-adc.o
>  obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o
> +obj-$(CONFIG_STM32_DFSDM_ADC) += stm32-dfsdm-adc.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> new file mode 100644
> index 0000000..f9419ab
> --- /dev/null
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -0,0 +1,741 @@
> +/*
> + * This file is the ADC part of the STM32 DFSDM driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author: Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>.
> + *
> + * License type: GPL V2.0.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that 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/interrupt.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/hw-consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include "stm32-dfsdm.h"
> +
> +/* Conversion timeout */
> +#define DFSDM_TIMEOUT_US 100000
> +#define DFSDM_TIMEOUT (msecs_to_jiffies(DFSDM_TIMEOUT_US / 1000))
> +
> +/* Oversampling attribute default */
> +#define DFSDM_DEFAULT_OVERSAMPLING  100
> +
> +/* Oversampling max values */
> +#define DFSDM_MAX_INT_OVERSAMPLING 256
> +#define DFSDM_MAX_FL_OVERSAMPLING 1024
> +
> +/* Max sample resolutions */
> +#define DFSDM_MAX_RES BIT(31)
> +#define DFSDM_DATA_RES BIT(23)
> +
> +enum sd_converter_type {
> +	DFSDM_AUDIO,
> +	DFSDM_IIO,
> +};
> +
> +struct stm32_dfsdm_dev_data {
> +	int type;
> +	int (*init)(struct iio_dev *indio_dev);
> +	unsigned int num_channels;
> +	const struct regmap_config *regmap_cfg;
> +};
> +
> +struct stm32_dfsdm_adc {
> +	struct stm32_dfsdm *dfsdm;
> +	const struct stm32_dfsdm_dev_data *dev_data;
> +	unsigned int fl_id;
> +	unsigned int ch_id;
> +
> +	/* ADC specific */
> +	unsigned int oversamp;
> +	struct iio_hw_consumer *hwc;
> +	struct completion completion;
> +	u32 *buffer;
> +
> +};
> +
> +struct stm32_dfsdm_str2field {
> +	const char	*name;
> +	unsigned int	val;
> +};
> +
> +/* DFSDM channel serial interface type */
> +static const struct stm32_dfsdm_str2field stm32_dfsdm_chan_type[] = {
> +	{ "SPI_R", 0 }, /* SPI with data on rising edge */
> +	{ "SPI_F", 1 }, /* SPI with data on falling edge */
> +	{ "MANCH_R", 2 }, /* Manchester codec, rising edge = logic 0 */
> +	{ "MANCH_F", 3 }, /* Manchester codec, falling edge = logic 1 */
> +	{},
> +};
> +
> +/* DFSDM channel clock source */
> +static const struct stm32_dfsdm_str2field stm32_dfsdm_chan_src[] = {
> +	/* External SPI clock (CLKIN x) */
> +	{ "CLKIN", DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL },
> +	/* Internal SPI clock (CLKOUT) */
> +	{ "CLKOUT", DFSDM_CHANNEL_SPI_CLOCK_INTERNAL },
> +	/* Internal SPI clock divided by 2 (falling edge) */
> +	{ "CLKOUT_F", DFSDM_CHANNEL_SPI_CLOCK_INTERNAL_DIV2_FALLING },
> +	/* Internal SPI clock divided by 2 (falling edge) */
> +	{ "CLKOUT_R", DFSDM_CHANNEL_SPI_CLOCK_INTERNAL_DIV2_RISING },
> +	{},
> +};
> +
> +static int stm32_dfsdm_str2val(const char *str,
> +			       const struct stm32_dfsdm_str2field *list)
> +{
> +	const struct stm32_dfsdm_str2field *p = list;
> +
> +	for (p = list; p && p->name; p++)
> +		if (!strcmp(p->name, str))
> +			return p->val;
> +
> +	return -EINVAL;
> +}
> +
> +static int stm32_dfsdm_set_osrs(struct stm32_dfsdm_filter *fl,
> +				unsigned int fast, unsigned int oversamp)
> +{
> +	unsigned int i, d, fosr, iosr;
> +	u64 res;
> +	s64 delta;
> +	unsigned int m = 1;	/* multiplication factor */
> +	unsigned int p = fl->ford;	/* filter order (ford) */
> +
> +	pr_debug("%s: Requested oversampling: %d\n",  __func__, oversamp);
> +	/*
> +	 * This function tries to compute filter oversampling and integrator
> +	 * oversampling, base on oversampling ratio requested by user.
> +	 *
> +	 * Decimation d depends on the filter order and the oversampling ratios.
> +	 * ford: filter order
> +	 * fosr: filter over sampling ratio
> +	 * iosr: integrator over sampling ratio
> +	 */
> +	if (fl->ford == DFSDM_FASTSINC_ORDER) {
> +		m = 2;
> +		p = 2;
> +	}
> +
> +	/*
> +	 * Look for filter and integrator oversampling ratios which allows
> +	 * to reach 24 bits data output resolution.
> +	 * Leave as soon as if exact resolution if reached.
> +	 * Otherwise the higher resolution below 32 bits is kept.
> +	 */
> +	for (fosr = 1; fosr <= DFSDM_MAX_FL_OVERSAMPLING; fosr++) {
> +		for (iosr = 1; iosr <= DFSDM_MAX_INT_OVERSAMPLING; iosr++) {
> +			if (fast)
> +				d = fosr * iosr;
> +			else if (fl->ford == DFSDM_FASTSINC_ORDER)
> +				d = fosr * (iosr + 3) + 2;
> +			else
> +				d = fosr * (iosr - 1 + p) + p;
> +
> +			if (d > oversamp)
> +				break;
> +			else if (d != oversamp)
> +				continue;
> +			/*
> +			 * Check resolution (limited to signed 32 bits)
> +			 *   res <= 2^31
> +			 * Sincx filters:
> +			 *   res = m * fosr^p x iosr (with m=1, p=ford)
> +			 * FastSinc filter
> +			 *   res = m * fosr^p x iosr (with m=2, p=2)
> +			 */
> +			res = fosr;
> +			for (i = p - 1; i > 0; i--) {
> +				res = res * (u64)fosr;
> +				if (res > DFSDM_MAX_RES)
> +					break;
> +			}
> +			if (res > DFSDM_MAX_RES)
> +				continue;
> +			res = res * (u64)m * (u64)iosr;
> +			if (res > DFSDM_MAX_RES)
> +				continue;
> +
> +			delta = res - DFSDM_DATA_RES;
> +
> +			if (res >= fl->res) {
> +				fl->res = res;
> +				fl->fosr = fosr;
> +				fl->iosr = iosr;
> +				fl->fast = fast;
> +				pr_debug("%s: fosr = %d, iosr = %d\n",
> +					 __func__, fl->fosr, fl->iosr);
> +			}
> +
> +			if (!delta)
> +				return 0;
> +		}
> +	}
> +
> +	if (!fl->fosr)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int stm32_dfsdm_start_channel(struct stm32_dfsdm *dfsdm,
> +				     unsigned int ch_id)
> +{
> +	return regmap_update_bits(dfsdm->regmap, DFSDM_CHCFGR1(ch_id),
> +				  DFSDM_CHCFGR1_CHEN_MASK,
> +				  DFSDM_CHCFGR1_CHEN(1));
> +}
> +
> +static void stm32_dfsdm_stop_channel(struct stm32_dfsdm *dfsdm,
> +				     unsigned int ch_id)
> +{
> +	regmap_update_bits(dfsdm->regmap, DFSDM_CHCFGR1(ch_id),
> +			   DFSDM_CHCFGR1_CHEN_MASK, DFSDM_CHCFGR1_CHEN(0));
> +}
> +
> +static int stm32_dfsdm_chan_configure(struct stm32_dfsdm *dfsdm,
> +				      struct stm32_dfsdm_channel *ch)
> +{
> +	unsigned int id = ch->id;
> +	struct regmap *regmap = dfsdm->regmap;
> +	int ret;
> +
> +	ret = regmap_update_bits(regmap, DFSDM_CHCFGR1(id),
> +				 DFSDM_CHCFGR1_SITP_MASK,
> +				 DFSDM_CHCFGR1_SITP(ch->type));
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_update_bits(regmap, DFSDM_CHCFGR1(id),
> +				 DFSDM_CHCFGR1_SPICKSEL_MASK,
> +				 DFSDM_CHCFGR1_SPICKSEL(ch->src));
> +	if (ret < 0)
> +		return ret;
> +	return regmap_update_bits(regmap, DFSDM_CHCFGR1(id),
> +				  DFSDM_CHCFGR1_CHINSEL_MASK,
> +				  DFSDM_CHCFGR1_CHINSEL(ch->alt_si));
> +}
> +
> +static int stm32_dfsdm_start_filter(struct stm32_dfsdm *dfsdm,
> +				    unsigned int fl_id)
> +{
> +	int ret;
> +
> +	/* Enable filter */
> +	ret = regmap_update_bits(dfsdm->regmap, DFSDM_CR1(fl_id),
> +				 DFSDM_CR1_DFEN_MASK, DFSDM_CR1_DFEN(1));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Start conversion */
> +	return regmap_update_bits(dfsdm->regmap, DFSDM_CR1(fl_id),
> +				  DFSDM_CR1_RSWSTART_MASK,
> +				  DFSDM_CR1_RSWSTART(1));
> +}
> +
> +void stm32_dfsdm_stop_filter(struct stm32_dfsdm *dfsdm, unsigned int fl_id)
> +{
> +	/* Disable conversion */
> +	regmap_update_bits(dfsdm->regmap, DFSDM_CR1(fl_id),
> +			   DFSDM_CR1_DFEN_MASK, DFSDM_CR1_DFEN(0));
> +}
> +
> +static int stm32_dfsdm_filter_configure(struct stm32_dfsdm *dfsdm,
> +					unsigned int fl_id, unsigned int ch_id)
> +{
> +	struct regmap *regmap = dfsdm->regmap;
> +	struct stm32_dfsdm_filter *fl = &dfsdm->fl_list[fl_id];
> +	int ret;
> +
> +	/* Average integrator oversampling */
> +	ret = regmap_update_bits(regmap, DFSDM_FCR(fl_id), DFSDM_FCR_IOSR_MASK,
> +				 DFSDM_FCR_IOSR(fl->iosr - 1));
> +	if (ret)
> +		return ret;
> +
> +	/* Filter order and Oversampling */
> +	ret = regmap_update_bits(regmap, DFSDM_FCR(fl_id), DFSDM_FCR_FOSR_MASK,
> +				 DFSDM_FCR_FOSR(fl->fosr - 1));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(regmap, DFSDM_FCR(fl_id), DFSDM_FCR_FORD_MASK,
> +				 DFSDM_FCR_FORD(fl->ford));
> +	if (ret)
> +		return ret;
> +
> +	/* No scan mode supported for the moment */
> +	ret = regmap_update_bits(regmap, DFSDM_CR1(fl_id), DFSDM_CR1_RCH_MASK,
> +				 DFSDM_CR1_RCH(ch_id));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(regmap, DFSDM_CR1(fl_id),
> +				  DFSDM_CR1_RSYNC_MASK,
> +				  DFSDM_CR1_RSYNC(fl->sync_mode));
> +}
> +
> +int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
> +				 struct iio_dev *indio_dev,
> +				 struct iio_chan_spec *ch)
> +{
> +	struct stm32_dfsdm_channel *df_ch;
> +	const char *of_str;
> +	int chan_idx = ch->scan_index;
> +	int ret, val;
> +
> +	ret = of_property_read_u32_index(indio_dev->dev.of_node,
> +					 "st,adc-channels", chan_idx,
> +					 &ch->channel);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev,
> +			" Error parsing 'st,adc-channels' for idx %d\n",
> +			chan_idx);
> +		return ret;
> +	}
> +	if (ch->channel >= dfsdm->num_chs) {
> +		dev_err(&indio_dev->dev,
> +			" Error bad channel number %d (max = %d)\n",
> +			ch->channel, dfsdm->num_chs);
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_string_index(indio_dev->dev.of_node,
> +					    "st,adc-channel-names", chan_idx,
> +					    &ch->datasheet_name);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev,
> +			" Error parsing 'st,adc-channel-names' for idx %d\n",
> +			chan_idx);
> +		return ret;
> +	}
> +
> +	df_ch =  &dfsdm->ch_list[ch->channel];
> +	df_ch->id = ch->channel;
> +
> +	ret = of_property_read_string_index(indio_dev->dev.of_node,
> +					    "st,adc-channel-types", chan_idx,
> +					    &of_str);
> +	if (!ret) {
> +		val  = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_type);
> +		if (val < 0)
> +			return val;
> +	} else {
> +		val = 0;
> +	}
> +	df_ch->type = val;
> +
> +	ret = of_property_read_string_index(indio_dev->dev.of_node,
> +					    "st,adc-channel-clk-src", chan_idx,
> +					    &of_str);
> +	if (!ret) {
> +		val  = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_src);
> +		if (val < 0)
> +			return val;
> +	} else {
> +		val = 0;
> +	}
> +	df_ch->src = val;
> +
> +	ret = of_property_read_u32_index(indio_dev->dev.of_node,
> +					 "st,adc-alt-channel", chan_idx,
> +					 &df_ch->alt_si);
> +	if (ret < 0)
> +		df_ch->alt_si = 0;
> +
> +	return 0;
> +}
> +
> +static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc, bool dma)
> +{
> +	struct regmap *regmap = adc->dfsdm->regmap;
> +	int ret;
> +
> +	ret = stm32_dfsdm_start_channel(adc->dfsdm, adc->ch_id);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = stm32_dfsdm_filter_configure(adc->dfsdm, adc->fl_id,
> +					   adc->ch_id);
> +	if (ret < 0)
> +		goto stop_channels;
> +
> +	ret = stm32_dfsdm_start_filter(adc->dfsdm, adc->fl_id);
> +	if (ret < 0)
> +		goto stop_channels;
> +
> +	return 0;
> +
> +stop_channels:
> +	regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> +			   DFSDM_CR1_RDMAEN_MASK, 0);
> +
> +	regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> +			   DFSDM_CR1_RCONT_MASK, 0);
> +	stm32_dfsdm_stop_channel(adc->dfsdm, adc->fl_id);
> +
> +	return ret;
> +}
> +
> +static void stm32_dfsdm_stop_conv(struct stm32_dfsdm_adc *adc)
> +{
> +	struct regmap *regmap = adc->dfsdm->regmap;
> +
> +	stm32_dfsdm_stop_filter(adc->dfsdm, adc->fl_id);
> +
> +	/* Clean conversion options */
> +	regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> +			   DFSDM_CR1_RDMAEN_MASK, 0);
> +
> +	regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> +			   DFSDM_CR1_RCONT_MASK, 0);
> +
> +	stm32_dfsdm_stop_channel(adc->dfsdm, adc->ch_id);
> +}
> +
> +static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan, int *res)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	long timeout;
> +	int ret;
> +
> +	reinit_completion(&adc->completion);
> +
> +	adc->buffer = res;
> +
> +	ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(adc->dfsdm->regmap, DFSDM_CR2(adc->fl_id),
> +				 DFSDM_CR2_REOCIE_MASK, DFSDM_CR2_REOCIE(1));
> +	if (ret < 0)
> +		goto stop_dfsdm;
> +
> +	ret = stm32_dfsdm_start_conv(adc, false);
> +	if (ret < 0) {
> +		regmap_update_bits(adc->dfsdm->regmap, DFSDM_CR2(adc->fl_id),
> +				   DFSDM_CR2_REOCIE_MASK, DFSDM_CR2_REOCIE(0));
> +		goto stop_dfsdm;
> +	}
> +
> +	timeout = wait_for_completion_interruptible_timeout(&adc->completion,
> +							    DFSDM_TIMEOUT);
> +
> +	/* Mask IRQ for regular conversion achievement*/
> +	regmap_update_bits(adc->dfsdm->regmap, DFSDM_CR2(adc->fl_id),
> +			   DFSDM_CR2_REOCIE_MASK, DFSDM_CR2_REOCIE(0));
> +
> +	if (timeout == 0)
> +		ret = -ETIMEDOUT;
> +	else if (timeout < 0)
> +		ret = timeout;
> +	else
> +		ret = IIO_VAL_INT;
> +
> +	stm32_dfsdm_stop_conv(adc);
> +
> +stop_dfsdm:
> +	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
> +
> +	return ret;
> +}
> +
> +static int stm32_dfsdm_write_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> +	int ret = -EINVAL;
> +
> +	if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
> +		ret = stm32_dfsdm_set_osrs(fl, 0, val);
> +		if (!ret)
> +			adc->oversamp = val;
> +	}
> +
> +	return ret;
> +}
> +
> +static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan, int *val,
> +				int *val2, long mask)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_hw_consumer_enable(adc->hwc);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev,
> +				"%s: IIO enable failed (channel %d)\n",
> +				__func__, chan->channel);
> +			return ret;
> +		}
> +		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
> +		iio_hw_consumer_disable(adc->hwc);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev,
> +				"%s: Conversion failed (channel %d)\n",
> +				__func__, chan->channel);
> +			return ret;
> +		}
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = adc->oversamp;
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info stm32_dfsdm_info_adc = {
> +	.read_raw = stm32_dfsdm_read_raw,
> +	.write_raw = stm32_dfsdm_write_raw,
> +};
> +
> +static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
> +{
> +	struct stm32_dfsdm_adc *adc = arg;
> +	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +	struct regmap *regmap = adc->dfsdm->regmap;
> +	unsigned int status, int_en;
> +
> +	regmap_read(regmap, DFSDM_ISR(adc->fl_id), &status);
> +	regmap_read(regmap, DFSDM_CR2(adc->fl_id), &int_en);
> +
> +	if (status & DFSDM_ISR_REOCF_MASK) {
> +		/* Read the data register clean the IRQ status */
> +		regmap_read(regmap, DFSDM_RDATAR(adc->fl_id), adc->buffer);
> +		complete(&adc->completion);
> +	}
> +
> +	if (status & DFSDM_ISR_ROVRF_MASK) {
> +		if (int_en & DFSDM_CR2_ROVRIE_MASK)
> +			dev_warn(&indio_dev->dev, "Overrun detected\n");
> +		regmap_update_bits(regmap, DFSDM_ICR(adc->fl_id),
> +				   DFSDM_ICR_CLRROVRF_MASK,
> +				   DFSDM_ICR_CLRROVRF_MASK);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec *ch)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
> +	if (ret < 0)
> +		return ret;
> +
> +	ch->type = IIO_VOLTAGE;
> +	ch->indexed = 1;
> +
> +	/*
> +	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
> +	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
> +	 */
> +	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> +
> +	ch->scan_type.sign = 'u';
> +	ch->scan_type.realbits = 24;
> +	ch->scan_type.storagebits = 32;
> +	adc->ch_id = ch->channel;
> +
> +	return stm32_dfsdm_chan_configure(adc->dfsdm,
> +					  &adc->dfsdm->ch_list[ch->channel]);
> +}
> +
> +static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> +{
> +	struct iio_chan_spec *ch;
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int num_ch;
> +	int ret, chan_idx;
> +
> +	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
> +	ret = stm32_dfsdm_set_osrs(&adc->dfsdm->fl_list[adc->fl_id], 0,
> +				   adc->oversamp);
> +	if (ret < 0)
> +		return ret;
> +
> +	num_ch = of_property_count_u32_elems(indio_dev->dev.of_node,
> +					     "st,adc-channels");
> +	if (num_ch < 0 || num_ch > adc->dfsdm->num_chs) {
> +		dev_err(&indio_dev->dev, "Bad st,adc-channels\n");
> +		return num_ch < 0 ? num_ch : -EINVAL;
> +	}
> +
> +	/* Bind to SD modulator IIO device */
> +	adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
> +	if (IS_ERR(adc->hwc))
> +		return -EPROBE_DEFER;
> +
> +	ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
> +			  GFP_KERNEL);
> +	if (!ch)
> +		return -ENOMEM;
> +
> +	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> +		ch->scan_index = chan_idx;
> +		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Channels init failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	indio_dev->num_channels = num_ch;
> +	indio_dev->channels = ch;
> +
> +	init_completion(&adc->completion);
> +
> +	return 0;
> +}
> +
> +static const struct stm32_dfsdm_dev_data stm32h7_dfsdm_adc_data = {
> +	.type = DFSDM_IIO,
> +	.init = stm32_dfsdm_adc_init,
> +};
> +
> +static const struct of_device_id stm32_dfsdm_adc_match[] = {
> +	{ .compatible = "st,stm32-dfsdm-adc",
> +		.data = &stm32h7_dfsdm_adc_data,
> +	},
> +	{}
> +};
> +
> +static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_dfsdm_adc *adc;
> +	struct device_node *np = dev->of_node;
> +	const struct stm32_dfsdm_dev_data *dev_data;
> +	struct iio_dev *iio;
> +	const struct of_device_id *of_id;
> +	char *name;
> +	int ret, irq, val;
> +
> +	of_id = of_match_node(stm32_dfsdm_adc_match, np);
> +	if (!of_id->data) {
> +		dev_err(&pdev->dev, "Data associated to device is missing\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_data = (const struct stm32_dfsdm_dev_data *)of_id->data;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*adc));
> +	if (IS_ERR(iio)) {
> +		dev_err(dev, "%s: Failed to allocate IIO\n", __func__);
> +		return PTR_ERR(iio);
> +	}
> +
> +	adc = iio_priv(iio);
> +	if (IS_ERR(adc)) {
> +		dev_err(dev, "%s: Failed to allocate ADC\n", __func__);
> +		return PTR_ERR(adc);
> +	}
> +	adc->dfsdm = dev_get_drvdata(dev->parent);
> +
> +	iio->dev.parent = dev;
> +	iio->dev.of_node = np;
> +	iio->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +
> +	platform_set_drvdata(pdev, adc);
> +
> +	ret = of_property_read_u32(dev->of_node, "reg", &adc->fl_id);
> +	if (ret != 0) {
> +		dev_err(dev, "Missing reg property\n");
> +		return -EINVAL;
> +	}
> +
> +	name = devm_kzalloc(dev, sizeof("dfsdm-adc0"), GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +	iio->info = &stm32_dfsdm_info_adc;
> +	snprintf(name, sizeof("dfsdm-adc0"), "dfsdm-adc%d", adc->fl_id);
> +	iio->name = name;
> +
> +	/*
> +	 * In a first step IRQs generated for channels are not treated.
> +	 * So IRQ associated to filter instance 0 is dedicated to the Filter 0.
> +	 */
> +	irq = platform_get_irq(pdev, 0);
> +	ret = devm_request_irq(dev, irq, stm32_dfsdm_irq,
> +			       0, pdev->name, adc);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to request IRQ\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "st,filter-order", &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set filter order\n");
> +		return ret;
> +	}
> +
> +	adc->dfsdm->fl_list[adc->fl_id].ford = val;
> +
> +	ret = of_property_read_u32(dev->of_node, "st,filter0-sync", &val);
> +	if (!ret)
> +		adc->dfsdm->fl_list[adc->fl_id].sync_mode = val;
> +
> +	adc->dev_data = dev_data;
> +	ret = dev_data->init(iio);
> +	if (ret < 0)
> +		return ret;
> +
> +	return iio_device_register(iio);
> +}
> +
> +static int stm32_dfsdm_adc_remove(struct platform_device *pdev)
> +{
> +	struct stm32_dfsdm_adc *adc = platform_get_drvdata(pdev);
> +	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +
> +	iio_device_unregister(indio_dev);
Given that all we have here is device unregister, you could
use the devm_iio_device_register call and drop the remove entirely.
(of course you may add stuff here in later patches - I haven't
looked yet ;)
> +
> +	return 0;
> +}
> +
> +static struct platform_driver stm32_dfsdm_adc_driver = {
> +	.driver = {
> +		.name = "stm32-dfsdm-adc",
> +		.of_match_table = stm32_dfsdm_adc_match,
> +	},
> +	.probe = stm32_dfsdm_adc_probe,
> +	.remove = stm32_dfsdm_adc_remove,
> +
> +};
> +module_platform_driver(stm32_dfsdm_adc_driver);
> +
> +MODULE_DESCRIPTION("STM32 sigma delta ADC");
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen-qxv4g6HH51o@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");

^ permalink raw reply

* Re: [PATCH v2] iio: adc: aspeed: Deassert reset in probe
From: Jonathan Cameron @ 2017-11-19 16:03 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Joel Stanley, Philipp Zabel, Rick Altherr, Rob Herring,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-iio-u79uwXL29TY76Z2rM5mHXA, devicetree, LKML
In-Reply-To: <20171102144932.00001e17-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

On Thu, 2 Nov 2017 14:49:32 +0000
Jonathan Cameron <Jonathan.Cameron-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:

> On Wed, 1 Nov 2017 10:15:32 +1030
> Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org> wrote:
> 
> > On Tue, Oct 31, 2017 at 10:28 PM, Philipp Zabel
> > <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:  
> > > On Tue, Oct 31, 2017 at 3:12 AM, Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> > > wrote:    
> > >> The ASPEED SoC must deassert a reset in order to use the ADC
> > >> peripheral.
> > >>
> > >> The device tree bindings are updated to document the resets
> > >> phandle, and the example is updated to match what is expected for
> > >> both the reset and clock phandle. Note that the bindings should
> > >> have always had the reset controller, as the hardware is unusable
> > >> without it.
> > >>
> > >> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>    
> > >
> > > It is unfortunate that this has to break DT (theoretical) backwards
> > > compatibility, but given that the old bindings never worked,
> > > this is better than to pretend a required reset is optional.
> > >
> > > Reviewed-by: Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>    
> > 
> > Thanks. I agree that it's unfortunate; this has been my first time
> > working on an ARM SoC and there were few things we could have done
> > better in hindsight.
> > 
> > I've got similar patches for the ASPEED hwmon pwm/tach driver, and the
> > i2c driver that I'll send out now.
> > 
> > Thanks for the review.
> > 
> > Cheers,
> > 
> > Joel  
> Hi Joel,
> 
> IIO is closed for this cycle anyway now.
> Otherwise, series looks good.
> 
> Will pick up when back with my main PC as traveling for this week and
> next.
Forgot to ask, do you want me to pick this up as a fix?
Also does it make sense to tag it for stable?

If not I can pick it up for the coming cycle. Given the code changes
are small and well isolated I'm happy to do any of the 3 options,
it really depends on whether the rest of the platform works well enough
to be worth rushing these through?

Jonathan
> 
> Thanks,
> 
> Jonathan
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio"
> > in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/2] iio: light: add support to UVIS25 sensor
From: Jonathan Cameron @ 2017-11-19 16:44 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Lorenzo BIANCONI
In-Reply-To: <CAA2SeN+WGorb6t52euEnVYOp+R8Mxa0_Y9eeM0qo6=-MsnZ3EQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Fri, 27 Oct 2017 15:26:09 +0200
Lorenzo Bianconi <lorenzo.bianconi83-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> > On Wed, 25 Oct 2017 20:16:08 +0200
> > Lorenzo Bianconi <lorenzo.bianconi83-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > Really minor English comment to start.
> > add support 'for' UVIS25 sensor
> >  
> 
> Hi Jonathan,
> 
> I added regmap support, thanks for the hint, the code is much better :)
> Just wondering how to manage drdy issue in more elegant way.
> As you outlined, the problem is due to the fact the trigger interrupt
> is currently masked
> but st_uvis25_set_enable() in st_uvis25_read_oneshot() actually
> enables interrupt generation
> so during the msleep() we get tons of interrupts if we configured the
> line as IRQ_TYPE_LEVEL_HIGH.
Can you clear it with a single read at the start of the read_oneshot?

> One possible solution could be to drop trigger support and push data
> to iio buffer directly in main irq thread handler (but in this way we
> are force to use just this kind of trigger).
> What do you think?

Not ideal... It's another case of hardware doing something unhelpful
for software.  We'll have to paper over it as usual ;)

Go with which ever route you think works out nicest.

Sorry for the delay on this reply - I missed your reply on my first
pass through my emails in amongst the other parts of the thread.

> 
> Reagrds,
> Lorenzo
> 
<snip>
> 
> >> +
> >> +     err = hw->tf->read(hw->dev, addr, sizeof(data), &data);
> >> +     if (err < 0) {
> >> +             dev_err(hw->dev, "failed to read %02x register\n", addr);
> >> +             goto unlock;
> >> +     }
> >> +
> >> +     data = (data & ~mask) | ((val << __ffs(mask)) & mask);
> >> +
> >> +     err = hw->tf->write(hw->dev, addr, sizeof(data), &data);
> >> +     if (err < 0)
> >> +             dev_err(hw->dev, "failed to write %02x register\n", addr);
> >> +
> >> +unlock:
> >> +     mutex_unlock(&hw->lock);
> >> +
> >> +     return err < 0 ? err : 0;
> >> +}
> >> +
> >> +int st_uvis25_set_enable(struct st_uvis25_hw *hw, bool enable)
> >> +{
> >> +     int err;
> >> +
> >> +     err = st_uvis25_write_with_mask(hw, ST_UVIS25_REG_CTRL1_ADDR,
> >> +                                     ST_UVIS25_REG_ODR_MASK, enable);
> >> +     if (err < 0)
> >> +             return err;
> >> +
> >> +     hw->enabled = enable;
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int st_uvis25_read_oneshot(struct st_uvis25_hw *hw, u8 addr, int *val)
> >> +{
> >> +     u8 data;
> >> +     int err;
> >> +
> >> +     err = st_uvis25_set_enable(hw, true);
> >> +     if (err < 0)
> >> +             return err;
> >> +
> >> +     msleep(1500);  
> >
> > Could drive this off the interrupt rather than disabling the interrupt?
> > Would that be a little neater (simple completion here).
> >  
> 
> What do you mean? I did not get you.
If the interrupt 'stuck' case wasn't occuring you could basically
let the buffered path run once in order to get the oneshot read.
Thus you could use an interrupt instead of a sleep.
Doesn't really matter though.

> 
> >> +
> >> +     err = hw->tf->read(hw->dev, addr, sizeof(data), &data);
> >> +     if (err < 0)
> >> +             return err;  
> >
> > Is there a potential race here if for some reason we managed to
> > got to sleep for another conversion?  I think to be completely
> > safe you need force an additional read after the disable (or
> > will that fail to clear the data ready?
> >  
> 
> We can move the read access after the st_uvis25_set_enable(hw, false)
> in order to avoid an unnecessary read, do you agree?
You could perhaps get a later reading if you did that?
I don't suppose it really matters.
> 
> >> +
> >> +     st_uvis25_set_enable(hw, false);
> >> +
> >> +     *val = data;
> >> +
> >> +     return IIO_VAL_INT;
> >> +}
> >> +
> >> +static int st_uvis25_read_raw(struct iio_dev *iio_dev,
> >> +                           struct iio_chan_spec const *ch,
> >> +                           int *val, int *val2, long mask)
> >> +{
> >> +     int ret;
> >> +
> >> +     ret = iio_device_claim_direct_mode(iio_dev);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     switch (mask) {
> >> +     case IIO_CHAN_INFO_PROCESSED: {
> >> +             struct st_uvis25_hw *hw = iio_priv(iio_dev);
> >> +
> >> +             /*
> >> +              * mask irq line during oneshot read since the sensor
> >> +              * does not export the capability to disable data-ready line
> >> +              * in the register map and it is enabled by default.
> >> +              * If the line is unmasked during read_raw() it will be set
> >> +              * active and never reset since the trigger is disabled
> >> +              */  
> >
> > Nasty but well documented...
> >
> > I wonder if there is a nicer way to handle this... If we leave it on
> > the issues is that we end up with the status being checked by the interrupt
> > handler for the trigger (harmless if a waste of time) then the trigger
> > being fired (with nothing associated with it).  No consumers are attached
> > so we call notify done for all of them and finally that will result in
> > a try reenable. You could supply one of those that results in a read
> > to the device.  It think that would always deal with your case of
> > the data ready getting stuck high..
> >
> > (Not totally sure though as it's been a while since I dealt with a
> > sensor with this particular issue).
> >  
> >> +             if (hw->irq > 0)
> >> +                     disable_irq(hw->irq);
> >> +             ret = st_uvis25_read_oneshot(hw, ch->address, val);
> >> +             if (hw->irq > 0)
> >> +                     enable_irq(hw->irq);
> >> +             break;
> >> +     }
> >> +     default:
> >> +             ret = -EINVAL;
> >> +             break;
> >> +     }

^ permalink raw reply

* Re: [PATCH v2 1/3] dt-bindings: iio: adc: stm32: add support for diff channels
From: Jonathan Cameron @ 2017-11-19 16:48 UTC (permalink / raw)
  To: Rob Herring
  Cc: mark.rutland, devicetree, benjamin.gaignard, lars,
	alexandre.torgue, linux-iio, pmeerw, linux, linux-kernel,
	mcoquelin.stm32, knaack.h, Fabrice Gasnier, linux-arm-kernel,
	benjamin.gaignard
In-Reply-To: <20171027143729.7oqcps43qvieko4r@rob-hp-laptop>

On Fri, 27 Oct 2017 09:37:29 -0500
Rob Herring <robh@kernel.org> wrote:

> On Wed, Oct 25, 2017 at 11:27:43AM +0200, Fabrice Gasnier wrote:
> > STM32H7 ADC channels may be configured either as single-ended or
> > differential.
> > Add 'st,adc-diff-channels' property to support differential channels.
> > Differential channels are defined as a pair of positive and negative
> > inputs: vinp & vinn.
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > ---
> > Changes in v2:
> > - Add differential channels example
> > - Add a note on mixed single-ended / differential channels
> > ---
> >  .../devicetree/bindings/iio/adc/st,stm32-adc.txt   | 24 ++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)  
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
Thanks.  Applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 2/3] iio: adc: stm32: remove const channel names definition
From: Jonathan Cameron @ 2017-11-19 16:48 UTC (permalink / raw)
  To: Fabrice Gasnier
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	mark.rutland-5wv7dgnIgG8, mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
	alexandre.torgue-qxv4g6HH51o, lars-Qo5EllUWu/uELgA04lAiVw,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A,
	benjamin.gaignard-qxv4g6HH51o
In-Reply-To: <20171026184522.576fbd8d@archlinux>

On Thu, 26 Oct 2017 18:45:22 +0100
Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Wed, 25 Oct 2017 11:27:44 +0200
> Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org> wrote:
> 
> > Remove const array that defines channels. Build channels definition
> > at probe time, when initializing channels (only for requested ones).
> > This will ease adding differential channels support.
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>  
> 
> Looks good to me.  Only held on Rob finding some time for a final glance
> at the bindings..
Applied.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > Changes in v2:
> > - Improve val variable names in stm32_adc_chan_init_one()
> >   definition as suggested by Jonathan.
> > ---
> >  drivers/iio/adc/stm32-adc.c | 66 ++++++++++-----------------------------------
> >  1 file changed, 14 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 4df32cf..417a894 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -153,6 +153,8 @@ enum stm32h7_adc_dmngt {
> >  /* BOOST bit must be set on STM32H7 when ADC clock is above 20MHz */
> >  #define STM32H7_BOOST_CLKRATE		20000000UL
> >  
> > +#define STM32_ADC_CH_MAX		20	/* max number of channels */
> > +#define STM32_ADC_CH_SZ			5	/* max channel name size */
> >  #define STM32_ADC_MAX_SQ		16	/* SQ1..SQ16 */
> >  #define STM32_ADC_MAX_SMP		7	/* SMPx range is [0..7] */
> >  #define STM32_ADC_TIMEOUT_US		100000
> > @@ -300,6 +302,7 @@ struct stm32_adc_cfg {
> >   * @pcsel		bitmask to preselect channels on some devices
> >   * @smpr_val:		sampling time settings (e.g. smpr1 / smpr2)
> >   * @cal:		optional calibration data on some devices
> > + * @chan_name:		channel name array
> >   */
> >  struct stm32_adc {
> >  	struct stm32_adc_common	*common;
> > @@ -321,69 +324,28 @@ struct stm32_adc {
> >  	u32			pcsel;
> >  	u32			smpr_val[2];
> >  	struct stm32_adc_calib	cal;
> > -};
> > -
> > -/**
> > - * struct stm32_adc_chan_spec - specification of stm32 adc channel
> > - * @type:	IIO channel type
> > - * @channel:	channel number (single ended)
> > - * @name:	channel name (single ended)
> > - */
> > -struct stm32_adc_chan_spec {
> > -	enum iio_chan_type	type;
> > -	int			channel;
> > -	const char		*name;
> > +	char			chan_name[STM32_ADC_CH_MAX][STM32_ADC_CH_SZ];
> >  };
> >  
> >  /**
> >   * struct stm32_adc_info - stm32 ADC, per instance config data
> > - * @channels:		Reference to stm32 channels spec
> >   * @max_channels:	Number of channels
> >   * @resolutions:	available resolutions
> >   * @num_res:		number of available resolutions
> >   */
> >  struct stm32_adc_info {
> > -	const struct stm32_adc_chan_spec *channels;
> >  	int max_channels;
> >  	const unsigned int *resolutions;
> >  	const unsigned int num_res;
> >  };
> >  
> > -/*
> > - * Input definitions common for all instances:
> > - * stm32f4 can have up to 16 channels
> > - * stm32h7 can have up to 20 channels
> > - */
> > -static const struct stm32_adc_chan_spec stm32_adc_channels[] = {
> > -	{ IIO_VOLTAGE, 0, "in0" },
> > -	{ IIO_VOLTAGE, 1, "in1" },
> > -	{ IIO_VOLTAGE, 2, "in2" },
> > -	{ IIO_VOLTAGE, 3, "in3" },
> > -	{ IIO_VOLTAGE, 4, "in4" },
> > -	{ IIO_VOLTAGE, 5, "in5" },
> > -	{ IIO_VOLTAGE, 6, "in6" },
> > -	{ IIO_VOLTAGE, 7, "in7" },
> > -	{ IIO_VOLTAGE, 8, "in8" },
> > -	{ IIO_VOLTAGE, 9, "in9" },
> > -	{ IIO_VOLTAGE, 10, "in10" },
> > -	{ IIO_VOLTAGE, 11, "in11" },
> > -	{ IIO_VOLTAGE, 12, "in12" },
> > -	{ IIO_VOLTAGE, 13, "in13" },
> > -	{ IIO_VOLTAGE, 14, "in14" },
> > -	{ IIO_VOLTAGE, 15, "in15" },
> > -	{ IIO_VOLTAGE, 16, "in16" },
> > -	{ IIO_VOLTAGE, 17, "in17" },
> > -	{ IIO_VOLTAGE, 18, "in18" },
> > -	{ IIO_VOLTAGE, 19, "in19" },
> > -};
> > -
> >  static const unsigned int stm32f4_adc_resolutions[] = {
> >  	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
> >  	12, 10, 8, 6,
> >  };
> >  
> > +/* stm32f4 can have up to 16 channels */
> >  static const struct stm32_adc_info stm32f4_adc_info = {
> > -	.channels = stm32_adc_channels,
> >  	.max_channels = 16,
> >  	.resolutions = stm32f4_adc_resolutions,
> >  	.num_res = ARRAY_SIZE(stm32f4_adc_resolutions),
> > @@ -394,9 +356,9 @@ struct stm32_adc_info {
> >  	16, 14, 12, 10, 8,
> >  };
> >  
> > +/* stm32h7 can have up to 20 channels */
> >  static const struct stm32_adc_info stm32h7_adc_info = {
> > -	.channels = stm32_adc_channels,
> > -	.max_channels = 20,
> > +	.max_channels = STM32_ADC_CH_MAX,
> >  	.resolutions = stm32h7_adc_resolutions,
> >  	.num_res = ARRAY_SIZE(stm32h7_adc_resolutions),
> >  };
> > @@ -1628,15 +1590,16 @@ static void stm32_adc_smpr_init(struct stm32_adc *adc, int channel, u32 smp_ns)
> >  }
> >  
> >  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
> > -				    struct iio_chan_spec *chan,
> > -				    const struct stm32_adc_chan_spec *channel,
> > +				    struct iio_chan_spec *chan, u32 vinp,
> >  				    int scan_index, u32 smp)
> >  {
> >  	struct stm32_adc *adc = iio_priv(indio_dev);
> > +	char *name = adc->chan_name[vinp];
> >  
> > -	chan->type = channel->type;
> > -	chan->channel = channel->channel;
> > -	chan->datasheet_name = channel->name;
> > +	chan->type = IIO_VOLTAGE;
> > +	chan->channel = vinp;
> > +	snprintf(name, STM32_ADC_CH_SZ, "in%d", vinp);
> > +	chan->datasheet_name = name;
> >  	chan->scan_index = scan_index;
> >  	chan->indexed = 1;
> >  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > @@ -1699,8 +1662,7 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >  					   scan_index, &smp);
> >  
> >  		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
> > -					&adc_info->channels[val],
> > -					scan_index, smp);
> > +					val, scan_index, smp);
> >  		scan_index++;
> >  	}
> >    
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 3/3] iio: adc: stm32: add support for differential channels
From: Jonathan Cameron @ 2017-11-19 16:49 UTC (permalink / raw)
  To: Fabrice Gasnier
  Cc: robh+dt, linux, mark.rutland, mcoquelin.stm32, alexandre.torgue,
	lars, knaack.h, pmeerw, linux-iio, devicetree, linux-arm-kernel,
	linux-kernel, benjamin.gaignard, benjamin.gaignard
In-Reply-To: <20171026184750.06a35299@archlinux>

On Thu, 26 Oct 2017 18:47:50 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 25 Oct 2017 11:27:45 +0200
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 
> > STM32H7 ADC channels can be configured either as single ended or
> > differential with 'st,adc-channels' or 'st,adc-diff-channels'
> > (positive and negative input pair: <vinp vinn>, ...).
> > 
> > Differential channels have different offset and scale, from spec:
> > raw value = (full_scale / 2) * (1 + (vinp - vinn) / vref).
> > Add offset attribute.
> > 
> > Differential channels are selected by DIFSEL register. Negative
> > inputs must be added to pre-selected channels as well (PCSEL).
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>  
> 
> Looks good. Again, DT review needed before I take the whole series.
> 
> Jonathan
> 
Applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> > ---
> > Changes in v2:
> > - Improve val & val2 variable names in stm32_adc_chan_init_one()
> >   definition as suggested by Jonathan.
> > ---
> >  drivers/iio/adc/stm32-adc.c | 123 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 103 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 417a894..fd50067 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -92,6 +92,7 @@
> >  #define STM32H7_ADC_SQR3		0x38
> >  #define STM32H7_ADC_SQR4		0x3C
> >  #define STM32H7_ADC_DR			0x40
> > +#define STM32H7_ADC_DIFSEL		0xC0
> >  #define STM32H7_ADC_CALFACT		0xC4
> >  #define STM32H7_ADC_CALFACT2		0xC8
> >  
> > @@ -154,7 +155,7 @@ enum stm32h7_adc_dmngt {
> >  #define STM32H7_BOOST_CLKRATE		20000000UL
> >  
> >  #define STM32_ADC_CH_MAX		20	/* max number of channels */
> > -#define STM32_ADC_CH_SZ			5	/* max channel name size */
> > +#define STM32_ADC_CH_SZ			10	/* max channel name size */
> >  #define STM32_ADC_MAX_SQ		16	/* SQ1..SQ16 */
> >  #define STM32_ADC_MAX_SMP		7	/* SMPx range is [0..7] */
> >  #define STM32_ADC_TIMEOUT_US		100000
> > @@ -299,6 +300,7 @@ struct stm32_adc_cfg {
> >   * @rx_buf:		dma rx buffer cpu address
> >   * @rx_dma_buf:		dma rx buffer bus address
> >   * @rx_buf_sz:		dma rx buffer size
> > + * @difsel		bitmask to set single-ended/differential channel
> >   * @pcsel		bitmask to preselect channels on some devices
> >   * @smpr_val:		sampling time settings (e.g. smpr1 / smpr2)
> >   * @cal:		optional calibration data on some devices
> > @@ -321,12 +323,18 @@ struct stm32_adc {
> >  	u8			*rx_buf;
> >  	dma_addr_t		rx_dma_buf;
> >  	unsigned int		rx_buf_sz;
> > +	u32			difsel;
> >  	u32			pcsel;
> >  	u32			smpr_val[2];
> >  	struct stm32_adc_calib	cal;
> >  	char			chan_name[STM32_ADC_CH_MAX][STM32_ADC_CH_SZ];
> >  };
> >  
> > +struct stm32_adc_diff_channel {
> > +	u32 vinp;
> > +	u32 vinn;
> > +};
> > +
> >  /**
> >   * struct stm32_adc_info - stm32 ADC, per instance config data
> >   * @max_channels:	Number of channels
> > @@ -944,15 +952,19 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> >   * stm32h7_adc_prepare() - Leave power down mode to enable ADC.
> >   * @adc: stm32 adc instance
> >   * Leave power down mode.
> > + * Configure channels as single ended or differential before enabling ADC.
> >   * Enable ADC.
> >   * Restore calibration data.
> > - * Pre-select channels that may be used in PCSEL (required by input MUX / IO).
> > + * Pre-select channels that may be used in PCSEL (required by input MUX / IO):
> > + * - Only one input is selected for single ended (e.g. 'vinp')
> > + * - Two inputs are selected for differential channels (e.g. 'vinp' & 'vinn')
> >   */
> >  static int stm32h7_adc_prepare(struct stm32_adc *adc)
> >  {
> >  	int ret;
> >  
> >  	stm32h7_adc_exit_pwr_down(adc);
> > +	stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
> >  
> >  	ret = stm32h7_adc_enable(adc);
> >  	if (ret)
> > @@ -1224,10 +1236,23 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
> >  		return ret;
> >  
> >  	case IIO_CHAN_INFO_SCALE:
> > -		*val = adc->common->vref_mv;
> > -		*val2 = chan->scan_type.realbits;
> > +		if (chan->differential) {
> > +			*val = adc->common->vref_mv * 2;
> > +			*val2 = chan->scan_type.realbits;
> > +		} else {
> > +			*val = adc->common->vref_mv;
> > +			*val2 = chan->scan_type.realbits;
> > +		}
> >  		return IIO_VAL_FRACTIONAL_LOG2;
> >  
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		if (chan->differential)
> > +			/* ADC_full_scale / 2 */
> > +			*val = -((1 << chan->scan_type.realbits) / 2);
> > +		else
> > +			*val = 0;
> > +		return IIO_VAL_INT;
> > +
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -1591,29 +1616,39 @@ static void stm32_adc_smpr_init(struct stm32_adc *adc, int channel, u32 smp_ns)
> >  
> >  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
> >  				    struct iio_chan_spec *chan, u32 vinp,
> > -				    int scan_index, u32 smp)
> > +				    u32 vinn, int scan_index, bool differential)
> >  {
> >  	struct stm32_adc *adc = iio_priv(indio_dev);
> >  	char *name = adc->chan_name[vinp];
> >  
> >  	chan->type = IIO_VOLTAGE;
> >  	chan->channel = vinp;
> > -	snprintf(name, STM32_ADC_CH_SZ, "in%d", vinp);
> > +	if (differential) {
> > +		chan->differential = 1;
> > +		chan->channel2 = vinn;
> > +		snprintf(name, STM32_ADC_CH_SZ, "in%d-in%d", vinp, vinn);
> > +	} else {
> > +		snprintf(name, STM32_ADC_CH_SZ, "in%d", vinp);
> > +	}
> >  	chan->datasheet_name = name;
> >  	chan->scan_index = scan_index;
> >  	chan->indexed = 1;
> >  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > -	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > +					 BIT(IIO_CHAN_INFO_OFFSET);
> >  	chan->scan_type.sign = 'u';
> >  	chan->scan_type.realbits = adc->cfg->adc_info->resolutions[adc->res];
> >  	chan->scan_type.storagebits = 16;
> >  	chan->ext_info = stm32_adc_ext_info;
> >  
> > -	/* Prepare sampling time settings */
> > -	stm32_adc_smpr_init(adc, chan->channel, smp);
> > -
> >  	/* pre-build selected channels mask */
> >  	adc->pcsel |= BIT(chan->channel);
> > +	if (differential) {
> > +		/* pre-build diff channels mask */
> > +		adc->difsel |= BIT(chan->channel);
> > +		/* Also add negative input to pre-selected channels */
> > +		adc->pcsel |= BIT(chan->channel2);
> > +	}
> >  }
> >  
> >  static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> > @@ -1621,17 +1656,40 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >  	struct device_node *node = indio_dev->dev.of_node;
> >  	struct stm32_adc *adc = iio_priv(indio_dev);
> >  	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
> > +	struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX];
> >  	struct property *prop;
> >  	const __be32 *cur;
> >  	struct iio_chan_spec *channels;
> > -	int scan_index = 0, num_channels, ret;
> > +	int scan_index = 0, num_channels = 0, num_diff = 0, ret, i;
> >  	u32 val, smp = 0;
> >  
> > -	num_channels = of_property_count_u32_elems(node, "st,adc-channels");
> > -	if (num_channels < 0 ||
> > -	    num_channels > adc_info->max_channels) {
> > +	ret = of_property_count_u32_elems(node, "st,adc-channels");
> > +	if (ret > adc_info->max_channels) {
> >  		dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
> > -		return num_channels < 0 ? num_channels : -EINVAL;
> > +		return -EINVAL;
> > +	} else if (ret > 0) {
> > +		num_channels += ret;
> > +	}
> > +
> > +	ret = of_property_count_elems_of_size(node, "st,adc-diff-channels",
> > +					      sizeof(*diff));
> > +	if (ret > adc_info->max_channels) {
> > +		dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n");
> > +		return -EINVAL;
> > +	} else if (ret > 0) {
> > +		int size = ret * sizeof(*diff) / sizeof(u32);
> > +
> > +		num_diff = ret;
> > +		num_channels += ret;
> > +		ret = of_property_read_u32_array(node, "st,adc-diff-channels",
> > +						 (u32 *)diff, size);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (!num_channels) {
> > +		dev_err(&indio_dev->dev, "No channels configured\n");
> > +		return -ENODATA;
> >  	}
> >  
> >  	/* Optional sample time is provided either for each, or all channels */
> > @@ -1652,6 +1710,33 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >  			return -EINVAL;
> >  		}
> >  
> > +		/* Channel can't be configured both as single-ended & diff */
> > +		for (i = 0; i < num_diff; i++) {
> > +			if (val == diff[i].vinp) {
> > +				dev_err(&indio_dev->dev,
> > +					"channel %d miss-configured\n",	val);
> > +				return -EINVAL;
> > +			}
> > +		}
> > +		stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val,
> > +					0, scan_index, false);
> > +		scan_index++;
> > +	}
> > +
> > +	for (i = 0; i < num_diff; i++) {
> > +		if (diff[i].vinp >= adc_info->max_channels ||
> > +		    diff[i].vinn >= adc_info->max_channels) {
> > +			dev_err(&indio_dev->dev, "Invalid channel in%d-in%d\n",
> > +				diff[i].vinp, diff[i].vinn);
> > +			return -EINVAL;
> > +		}
> > +		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
> > +					diff[i].vinp, diff[i].vinn, scan_index,
> > +					true);
> > +		scan_index++;
> > +	}
> > +
> > +	for (i = 0; i < scan_index; i++) {
> >  		/*
> >  		 * Using of_property_read_u32_index(), smp value will only be
> >  		 * modified if valid u32 value can be decoded. This allows to
> > @@ -1659,11 +1744,9 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
> >  		 * value per channel.
> >  		 */
> >  		of_property_read_u32_index(node, "st,min-sample-time-nsecs",
> > -					   scan_index, &smp);
> > -
> > -		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
> > -					val, scan_index, smp);
> > -		scan_index++;
> > +					   i, &smp);
> > +		/* Prepare sampling time settings */
> > +		stm32_adc_smpr_init(adc, channels[i].channel, smp);
> >  	}
> >  
> >  	indio_dev->num_channels = scan_index;  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5 3/4] pmbus (core): Add virtual page config bit
From: Guenter Roeck @ 2017-11-19 16:49 UTC (permalink / raw)
  To: Andrew Jeffery, linux-hwmon
  Cc: robh+dt, mark.rutland, jdelvare, corbet, devicetree, linux-kernel,
	linux-doc, joel, openbmc
In-Reply-To: <c5ccfeab881181dfbfdbdfb26051bb4cd332f6e6.1510974230.git-series.andrew@aj.id.au>

On 11/17/2017 07:26 PM, Andrew Jeffery wrote:
> Some circumstances call for virtual pages, to expose multiple values
> packed into an extended PMBus register in a manner non-compliant with
> the PMBus standard. An example of this is the Maxim MAX31785 controller, which
> extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
> tach readings for both rotors of a dual rotor fan. This extended register
> contains two word-sized values, one reporting the rate of the fastest rotor,
> the other the rate of the slowest. The concept of virtual pages aids this
> situation by mapping the page number onto the value to be selected from the
> vectored result.
> 
> We should not try to set virtual pages on the device as such a page explicitly
> doesn't exist; add a flag so we can avoid doing so.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/pmbus/pmbus.h      |  2 ++
>   drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++---
>   2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index b54d7604d3ef..d39d506aa63e 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -372,6 +372,8 @@ enum pmbus_sensor_classes {
>   #define PMBUS_HAVE_PWM12	BIT(20)
>   #define PMBUS_HAVE_PWM34	BIT(21)
>   
> +#define PMBUS_PAGE_VIRTUAL	BIT(31)
> +
>   enum pmbus_data_format { linear = 0, direct, vid };
>   enum vrm_version { vr11 = 0, vr12, vr13 };
>   
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 631db88526b6..ba97230fcde7 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, int page)
>   	int rv = 0;
>   	int newpage;
>   
> -	if (page >= 0 && page != data->currpage) {
> +	if (page < 0 || page == data->currpage)
> +		return 0;
> +
> +	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
>   		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
>   		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
>   		if (newpage != page)
>   			rv = -EIO;

This should probably be something like
		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
		if (rv < 0)
			return rv;
		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
		if (newpage < 0)
			return newpage;
		if (newpage != page)
			return -EIO;

We can actually drop 'newpage' and just use 'rv'.

> -		else
> -			data->currpage = page;
>   	}
> +
> +	data->currpage = page;
> +

... otherwise currpage is set even on error.

>   	return rv;

this can then be
	return 0;

>   }
>   EXPORT_SYMBOL_GPL(pmbus_set_page);
> 

^ permalink raw reply

* Re: [PATCH 1/2] iio: light: add support to UVIS25 sensor
From: Lorenzo Bianconi @ 2017-11-19 17:24 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171119164452.4f457021@archlinux>

Hi Jonathan,

comments inline.

> On Fri, 27 Oct 2017 15:26:09 +0200
> Lorenzo Bianconi <lorenzo.bianconi83-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> > On Wed, 25 Oct 2017 20:16:08 +0200
>> > Lorenzo Bianconi <lorenzo.bianconi83-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> > Really minor English comment to start.
>> > add support 'for' UVIS25 sensor
>> >
>>
>> Hi Jonathan,
>>
>> I added regmap support, thanks for the hint, the code is much better :)
>> Just wondering how to manage drdy issue in more elegant way.
>> As you outlined, the problem is due to the fact the trigger interrupt
>> is currently masked
>> but st_uvis25_set_enable() in st_uvis25_read_oneshot() actually
>> enables interrupt generation
>> so during the msleep() we get tons of interrupts if we configured the
>> line as IRQ_TYPE_LEVEL_HIGH.
> Can you clear it with a single read at the start of the read_oneshot?

I guess we could have a potential race doing in that way right?

>
>> One possible solution could be to drop trigger support and push data
>> to iio buffer directly in main irq thread handler (but in this way we
>> are force to use just this kind of trigger).
>> What do you think?
>
> Not ideal... It's another case of hardware doing something unhelpful
> for software.  We'll have to paper over it as usual ;)
>
> Go with which ever route you think works out nicest.

I guess the less ugly approach is to mask the irq line for one-shot reading.
Right, in this case hw is doing something unhelpful for sw guys :)

>
> Sorry for the delay on this reply - I missed your reply on my first
> pass through my emails in amongst the other parts of the thread.
>

No worries, I knew you were traveling :)
Btw I have to send the v2 with a different Signed-off-by, my email
address is no more valid since I moved to a different company

Regards,
Lorenzo

>>
>> Reagrds,
>> Lorenzo
>>
> <snip>
>>
>> >> +
>> >> +     err = hw->tf->read(hw->dev, addr, sizeof(data), &data);
>> >> +     if (err < 0) {
>> >> +             dev_err(hw->dev, "failed to read %02x register\n", addr);
>> >> +             goto unlock;
>> >> +     }
>> >> +
>> >> +     data = (data & ~mask) | ((val << __ffs(mask)) & mask);
>> >> +
>> >> +     err = hw->tf->write(hw->dev, addr, sizeof(data), &data);
>> >> +     if (err < 0)
>> >> +             dev_err(hw->dev, "failed to write %02x register\n", addr);
>> >> +
>> >> +unlock:
>> >> +     mutex_unlock(&hw->lock);
>> >> +
>> >> +     return err < 0 ? err : 0;
>> >> +}
>> >> +
>> >> +int st_uvis25_set_enable(struct st_uvis25_hw *hw, bool enable)
>> >> +{
>> >> +     int err;
>> >> +
>> >> +     err = st_uvis25_write_with_mask(hw, ST_UVIS25_REG_CTRL1_ADDR,
>> >> +                                     ST_UVIS25_REG_ODR_MASK, enable);
>> >> +     if (err < 0)
>> >> +             return err;
>> >> +
>> >> +     hw->enabled = enable;
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static int st_uvis25_read_oneshot(struct st_uvis25_hw *hw, u8 addr, int *val)
>> >> +{
>> >> +     u8 data;
>> >> +     int err;
>> >> +
>> >> +     err = st_uvis25_set_enable(hw, true);
>> >> +     if (err < 0)
>> >> +             return err;
>> >> +
>> >> +     msleep(1500);
>> >
>> > Could drive this off the interrupt rather than disabling the interrupt?
>> > Would that be a little neater (simple completion here).
>> >
>>
>> What do you mean? I did not get you.
> If the interrupt 'stuck' case wasn't occuring you could basically
> let the buffered path run once in order to get the oneshot read.
> Thus you could use an interrupt instead of a sleep.
> Doesn't really matter though.
>
>>
>> >> +
>> >> +     err = hw->tf->read(hw->dev, addr, sizeof(data), &data);
>> >> +     if (err < 0)
>> >> +             return err;
>> >
>> > Is there a potential race here if for some reason we managed to
>> > got to sleep for another conversion?  I think to be completely
>> > safe you need force an additional read after the disable (or
>> > will that fail to clear the data ready?
>> >
>>
>> We can move the read access after the st_uvis25_set_enable(hw, false)
>> in order to avoid an unnecessary read, do you agree?
> You could perhaps get a later reading if you did that?
> I don't suppose it really matters.

The output register will keep the right value since the ODR is just
1Hz, so I guess it does not matter

>>
>> >> +
>> >> +     st_uvis25_set_enable(hw, false);
>> >> +
>> >> +     *val = data;
>> >> +
>> >> +     return IIO_VAL_INT;
>> >> +}
>> >> +
>> >> +static int st_uvis25_read_raw(struct iio_dev *iio_dev,
>> >> +                           struct iio_chan_spec const *ch,
>> >> +                           int *val, int *val2, long mask)
>> >> +{
>> >> +     int ret;
>> >> +
>> >> +     ret = iio_device_claim_direct_mode(iio_dev);
>> >> +     if (ret)
>> >> +             return ret;
>> >> +
>> >> +     switch (mask) {
>> >> +     case IIO_CHAN_INFO_PROCESSED: {
>> >> +             struct st_uvis25_hw *hw = iio_priv(iio_dev);
>> >> +
>> >> +             /*
>> >> +              * mask irq line during oneshot read since the sensor
>> >> +              * does not export the capability to disable data-ready line
>> >> +              * in the register map and it is enabled by default.
>> >> +              * If the line is unmasked during read_raw() it will be set
>> >> +              * active and never reset since the trigger is disabled
>> >> +              */
>> >
>> > Nasty but well documented...
>> >
>> > I wonder if there is a nicer way to handle this... If we leave it on
>> > the issues is that we end up with the status being checked by the interrupt
>> > handler for the trigger (harmless if a waste of time) then the trigger
>> > being fired (with nothing associated with it).  No consumers are attached
>> > so we call notify done for all of them and finally that will result in
>> > a try reenable. You could supply one of those that results in a read
>> > to the device.  It think that would always deal with your case of
>> > the data ready getting stuck high..
>> >
>> > (Not totally sure though as it's been a while since I dealt with a
>> > sensor with this particular issue).
>> >
>> >> +             if (hw->irq > 0)
>> >> +                     disable_irq(hw->irq);
>> >> +             ret = st_uvis25_read_oneshot(hw, ch->address, val);
>> >> +             if (hw->irq > 0)
>> >> +                     enable_irq(hw->irq);
>> >> +             break;
>> >> +     }
>> >> +     default:
>> >> +             ret = -EINVAL;
>> >> +             break;
>> >> +     }



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v2 0/4] DRM driver for ILI9225 display panels
From: David Lechner @ 2017-11-19 20:12 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: David Lechner, Noralf Trønnes, Rob Herring, Mark Rutland,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

This is a new driver for ILI9225 based display panels.

v2 changes:
* New patch for ilitek vendor prefix.
* Use "ilitek" instead of "generic" in dt-bindings
* New patch to export 2 mipi_dbi_* functions
* Clean up ILI9225 driver based on feedback

David Lechner (4):
  dt-bindings: Add vendor prefix for ilitek
  dt-bindings: Add binding for Ilitek ILI9225 display panels
  drm/tinydrm: export mipi_dbi_buf_copy and mipi_dbi_spi_cmd_max_speed
  drm/tinydrm: add driver for ILI9225 panels

 .../devicetree/bindings/display/ilitek,ili9225.txt |  25 ++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 MAINTAINERS                                        |   6 +
 drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
 drivers/gpu/drm/tinydrm/Makefile                   |   1 +
 drivers/gpu/drm/tinydrm/ili9225.c                  | 468 +++++++++++++++++++++
 drivers/gpu/drm/tinydrm/mipi-dbi.c                 |  24 +-
 include/drm/tinydrm/mipi-dbi.h                     |   4 +-
 8 files changed, 534 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/ilitek,ili9225.txt
 create mode 100644 drivers/gpu/drm/tinydrm/ili9225.c

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v2 1/4] dt-bindings: Add vendor prefix for ilitek
From: David Lechner @ 2017-11-19 20:12 UTC (permalink / raw)
  To: dri-devel, devicetree
  Cc: Mark Rutland, David Lechner, Rob Herring, linux-kernel
In-Reply-To: <1511122328-31133-1-git-send-email-david@lechnology.com>

This adds the vendor prefix ilitek for ILI Technology Corporation (ILITEK).

This prefix is already used several places and I will be adding more.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* New patch in v2

 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 6cf1dc5..cf41a33 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -150,6 +150,7 @@ i2se	I2SE GmbH
 ibm	International Business Machines (IBM)
 idt	Integrated Device Technologies, Inc.
 ifi	Ingenieurburo Fur Ic-Technologie (I/F/I)
+ilitek	ILI Technology Corporation (ILITEK)
 img	Imagination Technologies Ltd.
 infineon Infineon Technologies
 inforce	Inforce Computing
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* [PATCH v2 2/4] dt-bindings: Add binding for Ilitek ILI9225 display panels
From: David Lechner @ 2017-11-19 20:12 UTC (permalink / raw)
  To: dri-devel, devicetree
  Cc: Mark Rutland, David Lechner, Rob Herring, linux-kernel
In-Reply-To: <1511122328-31133-1-git-send-email-david@lechnology.com>

This adds a new binding for display panels that use an Ilitek ILI9225
controller.

The "ilitek,ili9225-2.2in-176x220" device listed has no identification
markings whatsoever and an hour of googling turned up nothing, hence the use
of the size and resolution in the name instead of a model name.

An example of this nameless device can be found at:
https://github.com/Nkawu/TFT_22_ILI9225

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* renamed compatible string based on feedback

 .../devicetree/bindings/display/ilitek,ili9225.txt | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ilitek,ili9225.txt

diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9225.txt b/Documentation/devicetree/bindings/display/ilitek,ili9225.txt
new file mode 100644
index 0000000..21607a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ilitek,ili9225.txt
@@ -0,0 +1,25 @@
+Ilitek ILI9225 display panels
+
+This binding is for display panels using an Ilitek ILI9225 controller in SPI
+mode.
+
+Required properties:
+- compatible:	"ilitek,ili9225-2.2in-176x220"
+- rs-gpios:	Register select signal
+- reset-gpios:	Reset pin
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in ../spi/spi-bus.txt must be specified.
+
+Optional properties:
+- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
+
+Example:
+	display@0{
+		compatible = "ilitek,ili9225-2.2in-176x220";
+		reg = <0>;
+		spi-max-frequency = <12000000>;
+		rs-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
+		rotation = <270>;
+	};
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* [PATCH v2 3/4] drm/tinydrm: export mipi_dbi_buf_copy and mipi_dbi_spi_cmd_max_speed
From: David Lechner @ 2017-11-19 20:12 UTC (permalink / raw)
  To: dri-devel, devicetree
  Cc: Mark Rutland, David Lechner, Rob Herring, linux-kernel
In-Reply-To: <1511122328-31133-1-git-send-email-david@lechnology.com>

This exports the mipi_dbi_buf_copy() and mipi_dbi_spi_cmd_max_speed()
functions so that they can be shared with other drivers.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* new patch in v2

 drivers/gpu/drm/tinydrm/mipi-dbi.c | 24 ++++++++++++++++++++----
 include/drm/tinydrm/mipi-dbi.h     |  4 +++-
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 347f9b2..aa6b6ce 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -154,8 +154,18 @@ int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
 }
 EXPORT_SYMBOL(mipi_dbi_command_buf);
 
-static int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
-				struct drm_clip_rect *clip, bool swap)
+/**
+ * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
+ * @dst: The destination buffer
+ * @fb: The source framebuffer
+ * @clip: Clipping rectangle of the area to be copied
+ * @swap: When true, swap MSB/LSB of 16-bit values
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+		      struct drm_clip_rect *clip, bool swap)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
@@ -192,6 +202,7 @@ static int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 					     DMA_FROM_DEVICE);
 	return ret;
 }
+EXPORT_SYMBOL(mipi_dbi_buf_copy);
 
 static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 			     struct drm_file *file_priv,
@@ -444,18 +455,23 @@ EXPORT_SYMBOL(mipi_dbi_display_is_on);
 
 #if IS_ENABLED(CONFIG_SPI)
 
-/*
+/**
+ * mipi_dbi_spi_cmd_max_speed - get the maximum SPI bus speed
+ * @spi: SPI device
+ * @len: The transfer buffer length.
+ *
  * Many controllers have a max speed of 10MHz, but can be pushed way beyond
  * that. Increase reliability by running pixel data at max speed and the rest
  * at 10MHz, preventing transfer glitches from messing up the init settings.
  */
-static u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len)
+u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len)
 {
 	if (len > 64)
 		return 0; /* use default */
 
 	return min_t(u32, 10000000, spi->max_speed_hz);
 }
+EXPORT_SYMBOL(mipi_dbi_spi_cmd_max_speed);
 
 /*
  * MIPI DBI Type C Option 1
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index 83346dd..5d0e82b 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -72,10 +72,12 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
 void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
 void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
 bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
+u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
 
 int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
 int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
-
+int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+		      struct drm_clip_rect *clip, bool swap);
 /**
  * mipi_dbi_command - MIPI DCS command with optional parameter(s)
  * @mipi: MIPI structure
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* [PATCH v2 4/4] drm/tinydrm: add driver for ILI9225 panels
From: David Lechner @ 2017-11-19 20:12 UTC (permalink / raw)
  To: dri-devel, devicetree
  Cc: Mark Rutland, David Lechner, Rob Herring, linux-kernel
In-Reply-To: <1511122328-31133-1-git-send-email-david@lechnology.com>

This adds a new driver for display panels based on the Ilitek ILI9225
controller.

This was developed for a no-name panel with a red PCB that is commonly
marketed for Arduino. See <https://github.com/Nkawu/TFT_22_ILI9225>.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* use exported mipi_dbi_* functions from patch 3/4
* new ili9225_command function

 MAINTAINERS                       |   6 +
 drivers/gpu/drm/tinydrm/Kconfig   |  10 +
 drivers/gpu/drm/tinydrm/Makefile  |   1 +
 drivers/gpu/drm/tinydrm/ili9225.c | 468 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 485 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/ili9225.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0d77f22..72404f3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4372,6 +4372,12 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 S:	Maintained
 F:	drivers/gpu/drm/tve200/
 
+DRM DRIVER FOR ILITEK ILI9225 PANELS
+M:	David Lechner <david@lechnology.com>
+S:	Maintained
+F:	drivers/gpu/drm/tinydrm/ili9225.c
+F:	Documentation/devicetree/bindings/display/ili9225.txt
+
 DRM DRIVER FOR INTEL I810 VIDEO CARDS
 S:	Orphan / Obsolete
 F:	drivers/gpu/drm/i810/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 2e790e7..90c5bd5 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -12,6 +12,16 @@ menuconfig DRM_TINYDRM
 config TINYDRM_MIPI_DBI
 	tristate
 
+config TINYDRM_ILI9225
+	tristate "DRM support for ILI9225 display panels"
+	depends on DRM_TINYDRM && SPI
+	select TINYDRM_MIPI_DBI
+	help
+	  DRM driver for the following Ilitek ILI9225 panels:
+	  * No-name 2.2" color screen module
+
+	  If M is selected the module will be called ili9225.
+
 config TINYDRM_MI0283QT
 	tristate "DRM support for MI0283QT"
 	depends on DRM_TINYDRM && SPI
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 0c184bd..8aeee53 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_TINYDRM)		+= core/
 obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
 
 # Displays
+obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
 obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
new file mode 100644
index 0000000..3b766a2
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -0,0 +1,468 @@
+/*
+ * DRM driver for Ilitek ILI9225 panels
+ *
+ * Copyright 2017 David Lechner <david@lechnology.com>
+ *
+ * Some code copied from mipi-dbi.c
+ * Copyright 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/delay.h>
+#include <linux/dma-buf.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/spi/spi.h>
+#include <video/mipi_display.h>
+
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+
+#define ILI9225_DRIVER_READ_CODE	0x00
+#define ILI9225_DRIVER_OUTPUT_CONTROL	0x01
+#define ILI9225_LCD_AC_DRIVING_CONTROL	0x02
+#define ILI9225_ENTRY_MODE		0x03
+#define ILI9225_DISPLAY_CONTROL_1	0x07
+#define ILI9225_BLANK_PERIOD_CONTROL_1	0x08
+#define ILI9225_FRAME_CYCLE_CONTROL	0x0b
+#define ILI9225_INTERFACE_CONTROL	0x0c
+#define ILI9225_OSCILLATION_CONTROL	0x0f
+#define ILI9225_POWER_CONTROL_1		0x10
+#define ILI9225_POWER_CONTROL_2		0x11
+#define ILI9225_POWER_CONTROL_3		0x12
+#define ILI9225_POWER_CONTROL_4		0x13
+#define ILI9225_POWER_CONTROL_5		0x14
+#define ILI9225_VCI_RECYCLING		0x15
+#define ILI9225_RAM_ADDRESS_SET_1	0x20
+#define ILI9225_RAM_ADDRESS_SET_2	0x21
+#define ILI9225_WRITE_DATA_TO_GRAM	0x22
+#define ILI9225_SOFTWARE_RESET		0x28
+#define ILI9225_GATE_SCAN_CONTROL	0x30
+#define ILI9225_VERTICAL_SCROLL_1	0x31
+#define ILI9225_VERTICAL_SCROLL_2	0x32
+#define ILI9225_VERTICAL_SCROLL_3	0x33
+#define ILI9225_PARTIAL_DRIVING_POS_1	0x34
+#define ILI9225_PARTIAL_DRIVING_POS_2	0x35
+#define ILI9225_HORIZ_WINDOW_ADDR_1	0x36
+#define ILI9225_HORIZ_WINDOW_ADDR_2	0x37
+#define ILI9225_VERT_WINDOW_ADDR_1	0x38
+#define ILI9225_VERT_WINDOW_ADDR_2	0x39
+#define ILI9225_GAMMA_CONTROL_1		0x50
+#define ILI9225_GAMMA_CONTROL_2		0x51
+#define ILI9225_GAMMA_CONTROL_3		0x52
+#define ILI9225_GAMMA_CONTROL_4		0x53
+#define ILI9225_GAMMA_CONTROL_5		0x54
+#define ILI9225_GAMMA_CONTROL_6		0x55
+#define ILI9225_GAMMA_CONTROL_7		0x56
+#define ILI9225_GAMMA_CONTROL_8		0x57
+#define ILI9225_GAMMA_CONTROL_9		0x58
+#define ILI9225_GAMMA_CONTROL_10	0x59
+
+static inline int ili9225_command(struct mipi_dbi *mipi, u8 cmd, u16 data)
+{
+	u8 par[2] = { data >> 8, data & 0xff };
+
+	return mipi_dbi_command_buf(mipi, cmd, par, 2);
+}
+
+static int ili9225_fb_dirty(struct drm_framebuffer *fb,
+			    struct drm_file *file_priv, unsigned int flags,
+			    unsigned int color, struct drm_clip_rect *clips,
+			    unsigned int num_clips)
+{
+	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct tinydrm_device *tdev = fb->dev->dev_private;
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	bool swap = mipi->swap_bytes;
+	struct drm_clip_rect clip;
+	u16 x_start, y_start;
+	u16 x1, x2, y1, y2;
+	int ret = 0;
+	bool full;
+	void *tr;
+
+	mutex_lock(&tdev->dirty_lock);
+
+	if (!mipi->enabled)
+		goto out_unlock;
+
+	/* fbdev can flush even when we're not interested */
+	if (tdev->pipe.plane.fb != fb)
+		goto out_unlock;
+
+	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
+				   fb->width, fb->height);
+
+	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
+		  clip.x1, clip.x2, clip.y1, clip.y2);
+
+	if (!mipi->dc || !full || swap ||
+	    fb->format->format == DRM_FORMAT_XRGB8888) {
+		tr = mipi->tx_buf;
+		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap);
+		if (ret)
+			goto out_unlock;
+	} else {
+		tr = cma_obj->vaddr;
+	}
+
+	switch (mipi->rotation) {
+	default:
+		x1 = clip.x1;
+		x2 = clip.x2 - 1;
+		y1 = clip.y1;
+		y2 = clip.y2 - 1;
+		x_start = x1;
+		y_start = y1;
+		break;
+	case 90:
+		x1 = clip.y1;
+		x2 = clip.y2 - 1;
+		y1 = fb->width - clip.x2;
+		y2 = fb->width - clip.x1 - 1;
+		x_start = x1;
+		y_start = y2;
+		break;
+	case 180:
+		x1 = fb->width - clip.x2;
+		x2 = fb->width - clip.x1 - 1;
+		y1 = fb->height - clip.y2;
+		y2 = fb->height - clip.y1 - 1;
+		x_start = x2;
+		y_start = y2;
+		break;
+	case 270:
+		x1 = fb->height - clip.y2;
+		x2 = fb->height - clip.y1 - 1;
+		y1 = clip.x1;
+		y2 = clip.x2 - 1;
+		x_start = x2;
+		y_start = y1;
+		break;
+	}
+
+	ili9225_command(mipi, ILI9225_HORIZ_WINDOW_ADDR_1, x2);
+	ili9225_command(mipi, ILI9225_HORIZ_WINDOW_ADDR_2, x1);
+	ili9225_command(mipi, ILI9225_VERT_WINDOW_ADDR_1, y2);
+	ili9225_command(mipi, ILI9225_VERT_WINDOW_ADDR_2, y1);
+
+	ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_1, x_start);
+	ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_2, y_start);
+
+	ret = mipi_dbi_command_buf(mipi, ILI9225_WRITE_DATA_TO_GRAM, tr,
+				(clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2);
+
+out_unlock:
+	mutex_unlock(&tdev->dirty_lock);
+
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
+			     ret);
+
+	return ret;
+}
+
+static const struct drm_framebuffer_funcs ili9225_fb_funcs = {
+	.destroy	= drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+	.dirty		= ili9225_fb_dirty,
+};
+
+static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
+				struct drm_crtc_state *crtc_state)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	struct drm_framebuffer *fb = pipe->plane.fb;
+	struct device *dev = tdev->drm->dev;
+	int ret;
+	u8 am_id;
+
+	DRM_DEBUG_KMS("\n");
+
+	mipi_dbi_hw_reset(mipi);
+
+	/*
+	 * There don't seem to be two example init sequences that match, so
+	 * using the one from the popular Arduino library for this display.
+	 * https://github.com/Nkawu/TFT_22_ILI9225/blob/master/src/TFT_22_ILI9225.cpp
+	 */
+
+	ret = ili9225_command(mipi, ILI9225_POWER_CONTROL_1, 0x0000);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+		return;
+	}
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_2, 0x0000);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_3, 0x0000);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_4, 0x0000);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_5, 0x0000);
+
+	msleep(40);
+
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_2, 0x0018);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_3, 0x6121);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_4, 0x006f);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_5, 0x495f);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_1, 0x0800);
+
+	msleep(10);
+
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_2, 0x103b);
+
+	msleep(50);
+
+	switch (mipi->rotation) {
+	default:
+		am_id = 0x30;
+		break;
+	case 90:
+		am_id = 0x18;
+		break;
+	case 180:
+		am_id = 0x00;
+		break;
+	case 270:
+		am_id = 0x28;
+		break;
+	}
+	ili9225_command(mipi, ILI9225_DRIVER_OUTPUT_CONTROL, 0x011c);
+	ili9225_command(mipi, ILI9225_LCD_AC_DRIVING_CONTROL, 0x0100);
+	ili9225_command(mipi, ILI9225_ENTRY_MODE, 0x1000 | am_id);
+	ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x0000);
+	ili9225_command(mipi, ILI9225_BLANK_PERIOD_CONTROL_1, 0x0808);
+	ili9225_command(mipi, ILI9225_FRAME_CYCLE_CONTROL, 0x1100);
+	ili9225_command(mipi, ILI9225_INTERFACE_CONTROL, 0x0000);
+	ili9225_command(mipi, ILI9225_OSCILLATION_CONTROL, 0x0d01);
+	ili9225_command(mipi, ILI9225_VCI_RECYCLING, 0x0020);
+	ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_1, 0x0000);
+	ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_2, 0x0000);
+
+	ili9225_command(mipi, ILI9225_GATE_SCAN_CONTROL, 0x0000);
+	ili9225_command(mipi, ILI9225_VERTICAL_SCROLL_1, 0x00db);
+	ili9225_command(mipi, ILI9225_VERTICAL_SCROLL_2, 0x0000);
+	ili9225_command(mipi, ILI9225_VERTICAL_SCROLL_3, 0x0000);
+	ili9225_command(mipi, ILI9225_PARTIAL_DRIVING_POS_1, 0x00db);
+	ili9225_command(mipi, ILI9225_PARTIAL_DRIVING_POS_2, 0x0000);
+
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_1, 0x0000);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_2, 0x0808);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_3, 0x080a);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_4, 0x000a);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_5, 0x0a08);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_6, 0x0808);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_7, 0x0000);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_8, 0x0a00);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_9, 0x0710);
+	ili9225_command(mipi, ILI9225_GAMMA_CONTROL_10, 0x0710);
+
+	ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x0012);
+
+	msleep(50);
+
+	ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
+
+	mipi->enabled = true;
+
+	if (fb)
+		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+}
+
+static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+
+	DRM_DEBUG_KMS("\n");
+
+	if (!mipi->enabled)
+		return;
+
+	ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x0000);
+	msleep(50);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_2, 0x0007);
+	msleep(50);
+	ili9225_command(mipi, ILI9225_POWER_CONTROL_1, 0x0a02);
+
+	mipi->enabled = false;
+}
+
+static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
+			       size_t num)
+{
+	struct spi_device *spi = mipi->spi;
+	unsigned int bpw = 8;
+	u32 speed_hz;
+	int ret;
+
+	gpiod_set_value_cansleep(mipi->dc, 0);
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
+	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
+	if (ret || !num)
+		return ret;
+
+	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
+		bpw = 16;
+
+	gpiod_set_value_cansleep(mipi->dc, 1);
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
+
+	return tinydrm_spi_transfer(spi, speed_hz, NULL, bpw, par, num);
+}
+
+static const u32 ili9225_formats[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+};
+
+static int ili9225_init(struct device *dev, struct mipi_dbi *mipi,
+			const struct drm_simple_display_pipe_funcs *pipe_funcs,
+			struct drm_driver *driver,
+			const struct drm_display_mode *mode,
+			unsigned int rotation)
+{
+	size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16);
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	int ret;
+
+	if (!mipi->command)
+		return -EINVAL;
+
+	mutex_init(&mipi->cmdlock);
+
+	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
+	if (!mipi->tx_buf)
+		return -ENOMEM;
+
+	ret = devm_tinydrm_init(dev, tdev, &ili9225_fb_funcs, driver);
+	if (ret)
+		return ret;
+
+	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
+					DRM_MODE_CONNECTOR_VIRTUAL,
+					ili9225_formats,
+					ARRAY_SIZE(ili9225_formats), mode,
+					rotation);
+	if (ret)
+		return ret;
+
+	tdev->drm->mode_config.preferred_depth = 16;
+	mipi->rotation = rotation;
+
+	drm_mode_config_reset(tdev->drm);
+
+	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
+		      tdev->drm->mode_config.preferred_depth, rotation);
+
+	return 0;
+}
+
+static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
+	.enable		= ili9225_pipe_enable,
+	.disable	= ili9225_pipe_disable,
+	.update		= tinydrm_display_pipe_update,
+	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode ili9225_mode = {
+	TINYDRM_MODE(176, 220, 35, 44),
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(ili9225_fops);
+
+static struct drm_driver ili9225_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
+				  DRIVER_ATOMIC,
+	.fops			= &ili9225_fops,
+	TINYDRM_GEM_DRIVER_OPS,
+	.lastclose		= tinydrm_lastclose,
+	.name			= "ili9225",
+	.desc			= "Ilitek ILI9225",
+	.date			= "20171106",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static const struct of_device_id ili9225_of_match[] = {
+	{ .compatible = "ilitek,ili9225-2.2in-176x220" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ili9225_of_match);
+
+static const struct spi_device_id ili9225_id[] = {
+	{ "ili9225-2.2in-176x220", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, ili9225_id);
+
+static int ili9225_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct mipi_dbi *mipi;
+	struct gpio_desc *rs;
+	u32 rotation = 0;
+	int ret;
+
+	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
+	if (!mipi)
+		return -ENOMEM;
+
+	mipi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(mipi->reset)) {
+		DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
+		return PTR_ERR(mipi->reset);
+	}
+
+	rs = devm_gpiod_get(dev, "rs", GPIOD_OUT_LOW);
+	if (IS_ERR(rs)) {
+		DRM_DEV_ERROR(dev, "Failed to get gpio 'rs'\n");
+		return PTR_ERR(rs);
+	}
+
+	device_property_read_u32(dev, "rotation", &rotation);
+
+	ret = mipi_dbi_spi_init(spi, mipi, rs);
+	if (ret)
+		return ret;
+
+	/* override the command function set in  mipi_dbi_spi_init() */
+	mipi->command = ili9225_dbi_command;
+
+	ret = ili9225_init(&spi->dev, mipi, &ili9225_pipe_funcs,
+			   &ili9225_driver, &ili9225_mode, rotation);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, mipi);
+
+	return devm_tinydrm_register(&mipi->tinydrm);
+}
+
+static void ili9225_shutdown(struct spi_device *spi)
+{
+	struct mipi_dbi *mipi = spi_get_drvdata(spi);
+
+	tinydrm_shutdown(&mipi->tinydrm);
+}
+
+static struct spi_driver ili9225_spi_driver = {
+	.driver = {
+		.name = "ili9225",
+		.owner = THIS_MODULE,
+		.of_match_table = ili9225_of_match,
+	},
+	.id_table = ili9225_id,
+	.probe = ili9225_probe,
+	.shutdown = ili9225_shutdown,
+};
+module_spi_driver(ili9225_spi_driver);
+
+MODULE_DESCRIPTION("Ilitek ILI9225 DRM driver");
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_LICENSE("GPL");
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* Re: [RFC v1 7/8] Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check
From: Martin Blumenstingl @ 2017-11-19 20:24 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Rob Herring, devicetree, open list:BLUETOOTH DRIVERS,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Mark Rutland,
	Gustavo F. Padovan, Johan Hedberg, Greg Kroah-Hartman, Jiri Slaby,
	Johan Hovold, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ
In-Reply-To: <56889A64-AFB2-454C-9889-FAA4C051168A-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>

Hi Marcel,

thank you for going through my patches!

On Sun, Nov 19, 2017 at 9:21 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>> The three-wire (H5) protocol is the only protocol which uses
>> HCI_UART_INIT_PENDING.
>> Unfortunately the protocol implementation never receives data with this
>> check still in place. For the H5 protocol this means that the
>> initialization never completes and thus the firmware download never
>> starts. Even if the initialization would succeed later on the drivers
>> would call hci_uart_init_ready() which schedules the registration which
>> is currently not implemented by hci_serdev.c.
>>
>> Removing the HCI_UART_INIT_PENDING check makes the code easier to read
>> and also fixes the initalization of devices (implemented with the serdev
>> library) which use the H5 protocol.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>> drivers/bluetooth/hci_serdev.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
>> index e0e6461b9200..fe67eb6d4278 100644
>> --- a/drivers/bluetooth/hci_serdev.c
>> +++ b/drivers/bluetooth/hci_serdev.c
>> @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
>>       else
>>               hdev->dev_type = HCI_PRIMARY;
>>
>> -     if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
>> -             return 0;
>> -
>
> then lets also remove the flag definition itself. Or if that is still needed for some legacy operation, then describe this. For example I also see it used in hci_serdev.c and main question would be if it is used there as well or some legacy cruft.
this flag is still used in hci_ldisc.c (if that's what you mean
instead of hci_serdev.c):
as far as I understand the code it's used to postpone the
hci_register_dev() call until the sync response is received

Johan Hedberg added this code in 9f2aee848fe6 ("Bluetooth: Add delayed
init sequence support for UART controllers") - it would be great if he
could comment on this (he is CC'ed on this mail)


Regards
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules
From: Martin Blumenstingl @ 2017-11-19 20:28 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	Gustavo F. Padovan, Johan Hedberg,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, jslaby-IBi9RG/b67k,
	johan-DgEjT+Ai2ygdnm+yROfE0A,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ
In-Reply-To: <665B6C30-D115-437A-B991-999A862736FE-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>

Hi Marcel,

On Sun, Nov 19, 2017 at 9:29 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
>> Bluetooth controller which connects to the host via UART.
>> The H5 protocol is used for communication between host and device.
>>
>> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
>> initialization tools (rtk_hciattach) use the following sequence:
>> 1) send H5 sync pattern (already supported by hci_h5)
>> 2) get LMP version (already supported by btrtl)
>> 3) get ROM version (already supported by btrtl)
>> 4) load the firmware and config for the current chipset (already
>>   supported by btrtl)
>> 5) read UART settings from the config blob (already supported by btrtl)
>> 6) send UART settings via a vendor command to the device (which changes
>>   the baudrate of the device and enables or disables flow control
>>   depending on the config)
>> 7) change the baudrate and flow control settings on the host
>> 8) send the firmware and config blob to the device (already supported by
>>   btrtl)
>>
>> This uses the serdev library as well as the existing btrtl driver to
>> initialize the Bluetooth functionality, which consists of:
>> - identifying the device and loading the corresponding firmware and
>>  config blobs (steps #2, #3 and #4)
>> - configuring the baudrate and flow control (steps #6 and #7)
>> - uploading the firmware to the device (step #8)
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>> drivers/bluetooth/Kconfig  |   1 +
>> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 195 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 60e1c7d6986d..3001f1200c72 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
>> config BT_HCIUART_3WIRE
>>       bool "Three-wire UART (H5) protocol support"
>>       depends on BT_HCIUART
>> +     select BT_RTL if SERIAL_DEV_BUS
>>       help
>>         The HCI Three-wire UART Transport Layer makes it possible to
>>         user the Bluetooth HCI over a serial port interface. The HCI
>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
>> index 6a8d0d06aba7..7d584e5928bf 100644
>> --- a/drivers/bluetooth/hci_h5.c
>> +++ b/drivers/bluetooth/hci_h5.c
>> @@ -28,7 +28,14 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/serdev.h>
>> +
>> #include "hci_uart.h"
>> +#include "btrtl.h"
>>
>> #define HCI_3WIRE_ACK_PKT     0
>> #define HCI_3WIRE_LINK_PKT    15
>> @@ -97,6 +104,13 @@ struct h5 {
>>       } sleep;
>> };
>>
>> +struct h5_device {
>> +     struct hci_uart hu;
>> +     struct gpio_desc *disable_gpio;
>> +     struct gpio_desc *reset_gpio;
>> +     int (*vendor_setup)(struct h5_device *h5_dev);
>> +};
>> +
>> static void h5_reset_rx(struct h5 *h5);
>>
>> static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)
>> @@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu)
>> {
>>       struct h5 *h5;
>>       const unsigned char sync[] = { 0x01, 0x7e };
>> +     int err;
>>
>>       BT_DBG("hu %p", hu);
>>
>> @@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu)
>>
>>       h5->tx_win = H5_TX_WIN_MAX;
>>
>> +     if (hu->serdev) {
>> +             err = serdev_device_open(hu->serdev);
>> +             if (err) {
>> +                     bt_dev_err(hu->hdev, "failed to open serdev: %d", err);
>> +                     return err;
>> +             }
>> +     }
>> +
>>       set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
>>
>>       /* Send initial sync request */
>> @@ -219,6 +242,23 @@ static int h5_open(struct hci_uart *hu)
>>       return 0;
>> }
>>
>> +static int h5_setup(struct hci_uart *hu)
>> +{
>> +     int err;
>> +     struct h5_device *h5_dev;
>> +
>> +     if (!hu->serdev)
>> +             return 0;
>> +
>> +     h5_dev = serdev_device_get_drvdata(hu->serdev);
>> +
>> +     err = h5_dev->vendor_setup(h5_dev);
>> +     if (err)
>> +             return err;
>
>         if (h5_dev->vendor_setup)
>                 return h5_dev->vendor_setup(h5_dev);
sounds reasonable, this way new devices can be added which don't need
any vendor setup
I'll fix this in the next version, thanks for spotting this

>> +
>> +     return 0;
>> +}
>> +
>> static int h5_close(struct hci_uart *hu)
>> {
>>       struct h5 *h5 = hu->priv;
>> @@ -229,6 +269,15 @@ static int h5_close(struct hci_uart *hu)
>>       skb_queue_purge(&h5->rel);
>>       skb_queue_purge(&h5->unrel);
>>
>> +     if (hu->serdev) {
>> +             struct h5_device *h5_dev;
>> +
>> +             h5_dev = serdev_device_get_drvdata(hu->serdev);
>> +             gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
>> +
>> +             serdev_device_close(hu->serdev);
>> +     }
>> +
>>       kfree(h5);
>>
>>       return 0;
>> @@ -316,7 +365,10 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
>>                       h5->tx_win = (data[2] & 0x07);
>>               BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win);
>>               h5->state = H5_ACTIVE;
>> -             hci_uart_init_ready(hu);
>> +
>> +             /* serdev does not support the "init_ready" signal */
>> +             if (!hu->serdev)
>> +                     hci_uart_init_ready(hu);
>>               return;
>>       } else if (memcmp(data, sleep_req, 2) == 0) {
>>               BT_DBG("Peer went to sleep");
>> @@ -739,10 +791,147 @@ static int h5_flush(struct hci_uart *hu)
>>       return 0;
>> }
>>
>> +#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
>> +static int h5_setup_realtek(struct h5_device *h5_dev)
>> +{
>> +     struct hci_uart *hu = &h5_dev->hu;
>> +     int err = 0, retry = 3;
>> +     struct sk_buff *skb;
>> +     struct btrtl_device_info *btrtl_dev;
>> +     __le32 baudrate_data;
>> +     u32 device_baudrate;
>> +     unsigned int controller_baudrate;
>> +     bool flow_control;
>> +
>> +     /* devices always start with flow control disabled and even parity */
>> +     serdev_device_set_flow_control(hu->serdev, false);
>> +     serdev_device_set_parity(hu->serdev, true, false);
>> +
>> +     do {
>> +             /* Configure BT_DISn and BT_RST_N to LOW state */
>> +             gpiod_set_value_cansleep(h5_dev->reset_gpio, 1);
>> +             gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
>> +             msleep(500);
>> +             gpiod_set_value_cansleep(h5_dev->reset_gpio, 0);
>> +             gpiod_set_value_cansleep(h5_dev->disable_gpio, 0);
>> +             msleep(500);
>
> I really hate random msleep() without comments. Explain in the comment block why this specific wait is good.
OK, I'll add a comment *why* it's needed (without a delay between
toggling the GPIOs the device does not respond in the following
btrtl_initialize call)
the numbers however are based on "trial and error" as I could not find
any documentation for these

>> +
>> +             btrtl_dev = btrtl_initialize(hu->hdev);
>> +             if (!IS_ERR(btrtl_dev))
>> +                     break;
>> +
>> +             /* Toggle BT_DISn and retry */
>> +     } while (retry--);
>> +
>> +     if (IS_ERR(btrtl_dev))
>> +             return PTR_ERR(btrtl_dev);
>> +
>> +     err = btrtl_get_uart_settings(hu->hdev, btrtl_dev,
>> +                                   &controller_baudrate, &device_baudrate,
>> +                                   &flow_control);
>> +     if (err)
>> +             goto out_free;
>> +
>> +     baudrate_data = cpu_to_le32(device_baudrate);
>> +     skb = __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data),
>> +                          &baudrate_data, HCI_INIT_TIMEOUT);
>> +     if (IS_ERR(skb)) {
>> +             bt_dev_err(hu->hdev, "set baud rate command failed");
>> +             err = -PTR_ERR(skb);
>> +             goto out_free;
>> +     } else {
>> +             kfree_skb(skb);
>> +     }
>> +
>> +     msleep(500);
>
> Same here, explain why this time is the right time to wait.
you are right, this may be obsolete since the __hci_cmd_sync call
above is already waiting for a reply.
I'll verify this and either remove this msleep or add a comment why it's needed

>> +
>> +     serdev_device_set_baudrate(hu->serdev, controller_baudrate);
>> +     serdev_device_set_flow_control(hu->serdev, flow_control);
>> +
>> +     err = btrtl_download_firmware(hu->hdev, btrtl_dev);
>> +
>> +out_free:
>> +     btrtl_free(btrtl_dev);
>> +
>> +     return err;
>> +}
>> +
>> +static const struct hci_uart_proto h5p;
>> +
>> +static int hci_h5_probe(struct serdev_device *serdev)
>> +{
>> +     struct hci_uart *hu;
>> +     struct h5_device *h5_dev;
>> +
>> +     h5_dev = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL);
>> +     if (!h5_dev)
>> +             return -ENOMEM;
>> +
>> +     hu = &h5_dev->hu;
>> +     hu->serdev = serdev;
>> +
>> +     serdev_device_set_drvdata(serdev, h5_dev);
>> +
>> +     h5_dev->vendor_setup = of_device_get_match_data(&serdev->dev);
>> +
>> +     h5_dev->disable_gpio = devm_gpiod_get_optional(&serdev->dev, "disable",
>> +                                                    GPIOD_OUT_LOW);
>> +     if (IS_ERR(h5_dev->disable_gpio))
>> +             return PTR_ERR(h5_dev->disable_gpio);
>> +
>> +     h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset",
>> +                                                  GPIOD_OUT_LOW);
>> +     if (IS_ERR(h5_dev->reset_gpio))
>> +             return PTR_ERR(h5_dev->reset_gpio);
>> +
>> +     hci_uart_set_speeds(hu, 115200, 0);
>> +
>> +     return hci_uart_register_device(hu, &h5p);
>> +}
>> +
>> +static void hci_h5_remove(struct serdev_device *serdev)
>> +{
>> +     struct h5_device *h5_dev = serdev_device_get_drvdata(serdev);
>> +     struct hci_uart *hu = &h5_dev->hu;
>> +     struct hci_dev *hdev = hu->hdev;
>> +
>> +     cancel_work_sync(&hu->write_work);
>> +
>> +     hci_unregister_dev(hdev);
>> +     hci_free_dev(hdev);
>> +     hu->proto->close(hu);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id hci_h5_of_match[] = {
>> +     {
>> +             .compatible = "realtek,rtl8723bs-bluetooth",
>> +             .data = h5_setup_realtek
>> +     },
>> +     {
>> +             .compatible = "realtek,rtl8723ds-bluetooth",
>> +             .data = h5_setup_realtek
>> +     },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, hci_h5_of_match);
>> +#endif
>> +
>> +static struct serdev_device_driver hci_h5_drv = {
>> +     .driver         = {
>> +             .name   = "hci-h5",
>> +             .of_match_table = of_match_ptr(hci_h5_of_match),
>> +     },
>> +     .probe  = hci_h5_probe,
>> +     .remove = hci_h5_remove,
>> +};
>> +#endif
>> +
>> static const struct hci_uart_proto h5p = {
>>       .id             = HCI_UART_3WIRE,
>>       .name           = "Three-wire (H5)",
>>       .open           = h5_open,
>> +     .setup          = h5_setup,
>>       .close          = h5_close,
>>       .recv           = h5_recv,
>>       .enqueue        = h5_enqueue,
>> @@ -752,10 +941,14 @@ static const struct hci_uart_proto h5p = {
>>
>> int __init h5_init(void)
>> {
>> +     serdev_device_driver_register(&hci_h5_drv);
>> +
>>       return hci_uart_register_proto(&h5p);
>> }
>>
>> int __exit h5_deinit(void)
>> {
>> +     serdev_device_driver_unregister(&hci_h5_drv);
>> +
>>       return hci_uart_unregister_proto(&h5p);
>> }
>
> Regards
>
> Marcel
>


Regards
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips
From: Martin Blumenstingl @ 2017-11-19 20:38 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	Gustavo F. Padovan, Johan Hedberg,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, jslaby-IBi9RG/b67k,
	johan-DgEjT+Ai2ygdnm+yROfE0A, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ
In-Reply-To: <109FA59C-9875-4EAA-9DA5-EC811BAA77AE-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>

Hi Marcel,

On Sun, Nov 19, 2017 at 9:25 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>> The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
>> also contain a Bluetooth module which is connected via UART to the host.
>>
>> Realtek's userspace initialization tool (rtk_hciattach) differentiates
>> these two via the HCI version and revision returned by the
>> HCI_OP_READ_LOCAL_VERSION command.
>> Additionally we apply these checks only the for UART devices. Everything
>> else is assumed to be a "RTL8723B" which was originally supported by the
>> driver (communicating via USB).
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>> index 45b872f5ad22..d896f9421250 100644
>> --- a/drivers/bluetooth/btrtl.c
>> +++ b/drivers/bluetooth/btrtl.c
>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>               has_rom_version = false;
>>               break;
>>       case RTL_ROM_LMP_8723B:
>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>> +             /* all variants support reading the ROM version */
>>               has_rom_version = true;
>> +
>> +             /*
>> +              * RTL8723 devices exist in different variants:
>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>> +              * - for backwards-compatibility everything else is assumed to
>> +              *   be an RTL8723B communicating over USB
>> +              *
>> +              * Only UART devices really need the config because that
>> +              * contains the UART speed / flow control settings.
>> +              */
>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>> +                     cfg_needed = true;
>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>> +                     cfg_needed = true;
>> +             } else {
>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>> +             }
>
> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
this is an excellent question (and also the reason why it's an "RFC" patch)!
I'll try to figure out whether:
a) we can skip the config file completely
b) we can generate the config file in-memory
c) we have to stick with this config file

> What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.
to my knowledge this config file contains:
- the baudrate
- whether flow control is enabled or disabled
- (in some cases) the local-bd-address

do you mean a tool that is similar to bluez.git tools/nokfw.c?


Regards
Martin


[0] https://github.com/NextThingCo/rtl8723ds_bt/blob/f56ef37217665070e574253d23a595ee1ca5ca23/rtk_hciattach/hciattach_rtk.c#L1781
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC v1 7/8] Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check
From: Johan Hedberg @ 2017-11-19 20:43 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: Marcel Holtmann, Rob Herring, devicetree,
	open list:BLUETOOTH DRIVERS, linux-serial-u79uwXL29TY76Z2rM5mHXA,
	Mark Rutland, Gustavo F. Padovan, Greg Kroah-Hartman, Jiri Slaby,
	Johan Hovold, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ
In-Reply-To: <CAFBinCC_G845v4cZW9hvwYEDQhTCLW1iG_JWzAz9Gu42rM4_Zg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi Martin,

On Sun, Nov 19, 2017, Martin Blumenstingl wrote:
> >> @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
> >>       else
> >>               hdev->dev_type = HCI_PRIMARY;
> >>
> >> -     if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
> >> -             return 0;
> >> -
> >
> > then lets also remove the flag definition itself. Or if that is
> > still needed for some legacy operation, then describe this. For
> > example I also see it used in hci_serdev.c and main question would
> > be if it is used there as well or some legacy cruft.
> 
> this flag is still used in hci_ldisc.c (if that's what you mean
> instead of hci_serdev.c):
> as far as I understand the code it's used to postpone the
> hci_register_dev() call until the sync response is received
> 
> Johan Hedberg added this code in 9f2aee848fe6 ("Bluetooth: Add delayed
> init sequence support for UART controllers") - it would be great if he
> could comment on this (he is CC'ed on this mail)

That patch is over five years old, so don't count on me remembering the
details ;)  That said, your analysis of its use sounds correct. It's
possible however that we've since then given HCI drivers other
capabilities that would make the INIT_PENDING flag redundant. E.g. the
setup callback didn't exist at the time that I wrote the patch.

Johan

^ permalink raw reply

* Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips
From: Marcel Holtmann @ 2017-11-19 21:17 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: Rob Herring, devicetree, open list:BLUETOOTH DRIVERS,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Mark Rutland,
	Gustavo F. Padovan, Johan Hedberg, Greg Kroah-Hartman, Jiri Slaby,
	Johan Hovold, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ
In-Reply-To: <CAFBinCCtHUFMbbSOeKWYSJZKvFbNqhaMOrS-xgzr3E9hbkpnnw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi Martin,

>>> The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
>>> also contain a Bluetooth module which is connected via UART to the host.
>>> 
>>> Realtek's userspace initialization tool (rtk_hciattach) differentiates
>>> these two via the HCI version and revision returned by the
>>> HCI_OP_READ_LOCAL_VERSION command.
>>> Additionally we apply these checks only the for UART devices. Everything
>>> else is assumed to be a "RTL8723B" which was originally supported by the
>>> driver (communicating via USB).
>>> 
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>> ---
>>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>>> index 45b872f5ad22..d896f9421250 100644
>>> --- a/drivers/bluetooth/btrtl.c
>>> +++ b/drivers/bluetooth/btrtl.c
>>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>>              has_rom_version = false;
>>>              break;
>>>      case RTL_ROM_LMP_8723B:
>>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>>> +             /* all variants support reading the ROM version */
>>>              has_rom_version = true;
>>> +
>>> +             /*
>>> +              * RTL8723 devices exist in different variants:
>>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>>> +              * - for backwards-compatibility everything else is assumed to
>>> +              *   be an RTL8723B communicating over USB
>>> +              *
>>> +              * Only UART devices really need the config because that
>>> +              * contains the UART speed / flow control settings.
>>> +              */
>>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>>> +                     cfg_needed = true;
>>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>>> +                     cfg_needed = true;
>>> +             } else {
>>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>>> +             }
>> 
>> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
> this is an excellent question (and also the reason why it's an "RFC" patch)!
> I'll try to figure out whether:
> a) we can skip the config file completely
> b) we can generate the config file in-memory
> c) we have to stick with this config file
> 
>> What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.
> to my knowledge this config file contains:
> - the baudrate
> - whether flow control is enabled or disabled

these two are better done as part of the DT.

> - (in some cases) the local-bd-address

There is essentially DT support for that as well, but we also have Set Public Address mgmt command to handle this. Then again, first of all the hdev->set_bdaddr callback needs to be supported for that.

> do you mean a tool that is similar to bluez.git tools/nokfw.c?

Yes.

> [0] https://github.com/NextThingCo/rtl8723ds_bt/blob/f56ef37217665070e574253d23a595ee1ca5ca23/rtk_hciattach/hciattach_rtk.c#L1781

Please never ever make me look at code that defined struct sk_buff in userspace ;)

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 3/3] DT: leds: Add Qualcomm Light Pulse Generator binding
From: Jacek Anaszewski @ 2017-11-19 21:35 UTC (permalink / raw)
  To: Bjorn Andersson, Richard Purdie, Pavel Machek, Rob Herring,
	Mark Rutland
  Cc: linux-kernel, linux-leds, linux-arm-msm, devicetree, Fenglin Wu
In-Reply-To: <20171115071345.24331-4-bjorn.andersson@linaro.org>

Hi Bjorn,

Thanks for the update.

On 11/15/2017 08:13 AM, Bjorn Andersson wrote:
> This adds the binding document describing the three hardware blocks
> related to the Light Pulse Generator found in a wide range of Qualcomm
> PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - Squashed all things into one node
> - Removed quirks from the binding, compatible implies number of channels, their
>   configuration etc.
> - Binding describes LEDs connected as child nodes
> - Support describing multi-channel LEDs
> - Change style of the binding document, to match other LED bindings
> 
> Changes since v1:
> - Dropped custom pattern properties
> - Renamed cell-index to qcom,lpg-channel to clarify its purpose
> 
>  .../devicetree/bindings/leds/leds-qcom-lpg.txt     | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> new file mode 100644
> index 000000000000..9cee6f9f543c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> @@ -0,0 +1,66 @@
> +Binding for Qualcomm Light Pulse Generator
> +
> +The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> +a ramp generator with lookup table, the light pulse generator and a three
> +channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
> +
> +Required properties:
> +- compatible: one of:
> +	      "qcom,pm8916-pwm",
> +	      "qcom,pm8941-lpg",
> +	      "qcom,pm8994-lpg",
> +	      "qcom,pmi8994-lpg",
> +	      "qcom,pmi8998-lpg",
> +
> +Optional properties:
> +- qcom,power-source: power-source used to drive the output, as defined in the
> +		     datasheet. Should be specified if the TRILED block is
> +		     present

Range of possible values is missing here.

> +- qcom,dtest: configures the output into an internal test line of the
> +	      pmic. Specified by a list of u32 pairs, one pair per channel,
> +	      where each pair denotes the test line to drive and the second
> +	      configures how the value should be outputed, as defined in the
> +	      datasheet
> +- #pwm-cells: should be 2, see ../pwm/pwm.txt
> +
> +LED subnodes:
> +A set of subnodes can be used to specify LEDs connected to the LPG. Channels
> +not associated with a LED are available as pwm channels, see ../pwm/pwm.txt.
> +
> +Required properties:
> +- led-sources: list of channels associated with this LED, starting at 1 for the
> +	       first LPG channel
> +
> +Optional properties:
> +- label: see Documentation/devicetree/bindings/leds/common.txt
> +- default-state: see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +The following example defines a RGB LED attached to the PM8941.
> +
> +&spmi_bus {
> +	pm8941@1 {
> +		lpg {
> +			compatible = "qcom,pm8941-lpg";
> +			qcom,power-source = <1>;
> +
> +			rgb {
> +				led-sources = <7 6 5>;
> +			};
> +		};
> +	};
> +};
> +
> +The following example defines the single PWM channel of the PM8916, which can
> +be muxed by the MPP4 as a current sink.
> +
> +&spmi_bus {
> +	pm8916@1 {
> +		pm8916_pwm: pwm {
> +			compatible = "qcom,pm8916-pwm";
> +
> +			#pwm-cells = <2>;

LED has to be represented as a child node -
see Documentation/devicetree/bindings/leds/common.txt

> +		};
> +	};
> +};
> 

Could you please also provide an example of the arrangement on the
board DragonBoard820c, you were describing in the discussions under
the previous version of the patch set. i.e. three green LEDs connected
to TRILED and one to the GPIO sink?

Also any other non-trivial board configurations supported by the
driver would allow to increase our comprehensions of the device
capabilities.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox