Linux-IIO Archive on lore.kernel.org
 help / Atom feed
* [PATCH] iio: adc: ti-ads7950: add GPIO support
@ 2019-02-08 19:24 justinpopo6
  2019-02-09 17:00 ` Jonathan Cameron
  2019-02-11  8:34 ` Linus Walleij
  0 siblings, 2 replies; 7+ messages in thread
From: justinpopo6 @ 2019-02-08 19:24 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski,
	linus.walleij, jic23, knaack.h, lars, pmeerw, david,
	linux-kernel, Justin Chen

From: Justin Chen <justinpopo6@gmail.com>

The ADS79XX has GPIO pins that can be used. Add support for the GPIO
pins using the GPIO chip framework.

Signed-off-by: Justin Chen <justinpopo6@gmail.com>
---
 drivers/iio/adc/ti-ads7950.c | 166 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 164 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 0ad6359..c54706ad 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -17,6 +17,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -36,10 +37,14 @@
  */
 #define TI_ADS7950_VA_MV_ACPI_DEFAULT	5000
 
+#define TI_ADS7950_CR_GPIO	BIT(14)
 #define TI_ADS7950_CR_MANUAL	BIT(12)
 #define TI_ADS7950_CR_WRITE	BIT(11)
 #define TI_ADS7950_CR_CHAN(ch)	((ch) << 7)
 #define TI_ADS7950_CR_RANGE_5V	BIT(6)
+#define TI_ADS7950_CR_GPIO_DATA	BIT(5)
+#define TI_ADS7950_NUM_GPIOS	4
+#define TI_ADS7950_GPIO_MASK	GENMASK(TI_ADS7950_NUM_GPIOS - 1, 0)
 
 #define TI_ADS7950_MAX_CHAN	16
 
@@ -56,11 +61,17 @@ struct ti_ads7950_state {
 	struct spi_message	ring_msg;
 	struct spi_message	scan_single_msg;
 
+	struct iio_dev		*indio_dev;
+	struct gpio_chip	*chip;
+
 	struct regulator	*reg;
 	unsigned int		vref_mv;
 
 	unsigned int		settings;
 
+	unsigned int		gpio_direction_bitmask;
+	unsigned int		gpio_signal_bitmask;
+
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -248,7 +259,8 @@ static int ti_ads7950_update_scan_mode(struct iio_dev *indio_dev,
 
 	len = 0;
 	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels) {
-		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings;
+		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings
+		      | (st->gpio_signal_bitmask & TI_ADS7950_GPIO_MASK);
 		st->tx_buf[len++] = cmd;
 	}
 
@@ -288,7 +300,8 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 
 	mutex_lock(&indio_dev->mlock);
 
-	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
+	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings
+	      | (st->gpio_signal_bitmask & TI_ADS7950_GPIO_MASK);
 	st->single_tx = cmd;
 
 	ret = spi_sync(st->spi, &st->scan_single_msg);
@@ -362,10 +375,149 @@ static const struct iio_info ti_ads7950_info = {
 	.update_scan_mode	= ti_ads7950_update_scan_mode,
 };
 
+static void ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
+			   int value)
+{
+	struct ti_ads7950_state *st = gpiochip_get_data(chip);
+
+	mutex_lock(&st->indio_dev->mlock);
+
+	if (value)
+		st->gpio_signal_bitmask |= BIT(offset);
+	else
+		st->gpio_signal_bitmask &= ~BIT(offset);
+
+	st->single_tx = cpu_to_be16(TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_WRITE |
+			(st->gpio_signal_bitmask & TI_ADS7950_GPIO_MASK));
+	spi_sync(st->spi, &st->scan_single_msg);
+
+	mutex_unlock(&st->indio_dev->mlock);
+}
+
+static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ti_ads7950_state *st = gpiochip_get_data(chip);
+	int ret;
+
+	mutex_lock(&st->indio_dev->mlock);
+
+	/* If set as output, return the output */
+	if (st->gpio_direction_bitmask & BIT(offset)) {
+		ret = st->gpio_signal_bitmask & BIT(offset);
+		goto out;
+	}
+
+	st->single_tx = cpu_to_be16(TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_WRITE |
+				    TI_ADS7950_CR_GPIO_DATA);
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		goto out;
+
+	ret = ((st->single_rx >> 12) & BIT(offset)) ? 1 : 0;
+
+out:
+	mutex_unlock(&st->indio_dev->mlock);
+
+	return ret;
+}
+
+static int ti_ads7950_get_direction(struct gpio_chip *chip,
+				    unsigned int offset)
+{
+	struct ti_ads7950_state *st = gpiochip_get_data(chip);
+
+	return !(st->gpio_direction_bitmask & BIT(offset));
+}
+
+static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
+				     int input)
+{
+	struct ti_ads7950_state *st = gpiochip_get_data(chip);
+	int ret = 0;
+
+	mutex_lock(&st->indio_dev->mlock);
+
+	if (input && (st->gpio_direction_bitmask & BIT(offset)))
+		st->gpio_direction_bitmask &= ~BIT(offset);
+	else if (!input && !(st->gpio_direction_bitmask & BIT(offset)))
+		st->gpio_direction_bitmask |= BIT(offset);
+	else
+		goto out;
+
+
+	st->single_tx = cpu_to_be16(TI_ADS7950_CR_GPIO |
+				    (st->gpio_direction_bitmask &
+				    TI_ADS7950_GPIO_MASK));
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+
+out:
+	mutex_unlock(&st->indio_dev->mlock);
+
+	return ret;
+}
+
+static int ti_ads7950_direction_input(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	return _ti_ads7950_set_direction(chip, offset, 1);
+}
+
+static int ti_ads7950_direction_output(struct gpio_chip *chip,
+				       unsigned int offset, int value)
+{
+	ti_ads7950_set(chip, offset, value);
+
+	return _ti_ads7950_set_direction(chip, offset, 0);
+}
+
+static int ti_ads7950_init_gpio(struct ti_ads7950_state *st)
+{
+	int ret;
+
+	/* Initialize GPIO */
+	mutex_lock(&st->indio_dev->mlock);
+
+	/* Default to GPIO input */
+	st->gpio_direction_bitmask = 0x0;
+	st->single_tx = cpu_to_be16(TI_ADS7950_CR_GPIO |
+				    (st->gpio_direction_bitmask &
+				    TI_ADS7950_GPIO_MASK));
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	mutex_unlock(&st->indio_dev->mlock);
+	if (ret)
+		return ret;
+
+	/* Default to signal low */
+	st->gpio_signal_bitmask = 0x0;
+	st->single_tx = cpu_to_be16(TI_ADS7950_CR_MANUAL |
+				    TI_ADS7950_CR_WRITE |
+				    (st->gpio_signal_bitmask &
+				    TI_ADS7950_GPIO_MASK));
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	mutex_unlock(&st->indio_dev->mlock);
+	if (ret)
+		return ret;
+
+	/* Add GPIO chip */
+	st->chip->label = dev_name(&st->spi->dev);
+	st->chip->parent = &st->spi->dev;
+	st->chip->owner = THIS_MODULE;
+	st->chip->base = -1;
+	st->chip->ngpio = TI_ADS7950_NUM_GPIOS;
+	st->chip->get_direction = ti_ads7950_get_direction;
+	st->chip->direction_input = ti_ads7950_direction_input;
+	st->chip->direction_output = ti_ads7950_direction_output;
+	st->chip->get = ti_ads7950_get;
+	st->chip->set = ti_ads7950_set;
+
+	return gpiochip_add_data(st->chip, st);
+}
+
 static int ti_ads7950_probe(struct spi_device *spi)
 {
 	struct ti_ads7950_state *st;
 	struct iio_dev *indio_dev;
+	struct gpio_chip *chip;
 	const struct ti_ads7950_chip_info *info;
 	int ret;
 
@@ -381,6 +533,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	if (!indio_dev)
 		return -ENOMEM;
 
+	chip = devm_kzalloc(&spi->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
 	st = iio_priv(indio_dev);
 
 	spi_set_drvdata(spi, indio_dev);
@@ -396,6 +552,8 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	indio_dev->channels = info->channels;
 	indio_dev->num_channels = info->num_channels;
 	indio_dev->info = &ti_ads7950_info;
+	st->indio_dev = indio_dev;
+	st->chip = chip;
 
 	/* build spi ring message */
 	spi_message_init(&st->ring_msg);
@@ -457,6 +615,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
 		goto error_cleanup_ring;
 	}
 
+	ret = ti_ads7950_init_gpio(st);
+	if (ret)
+		dev_warn(&spi->dev, "Unable to initialize GPIOs\n");
+
 	return 0;
 
 error_cleanup_ring:
-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio: adc: ti-ads7950: add GPIO support
  2019-02-08 19:24 [PATCH] iio: adc: ti-ads7950: add GPIO support justinpopo6
@ 2019-02-09 17:00 ` Jonathan Cameron
  2019-02-09 18:56   ` David Lechner
  2019-02-11  8:34 ` Linus Walleij
  1 sibling, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2019-02-09 17:00 UTC (permalink / raw)
  To: justinpopo6
  Cc: linux-iio, linux-gpio, bcm-kernel-feedback-list, f.fainelli,
	bgolaszewski, linus.walleij, knaack.h, lars, pmeerw, david,
	linux-kernel

On Fri,  8 Feb 2019 11:24:16 -0800
justinpopo6@gmail.com wrote:

> From: Justin Chen <justinpopo6@gmail.com>
> 
> The ADS79XX has GPIO pins that can be used. Add support for the GPIO
> pins using the GPIO chip framework.
> 
> Signed-off-by: Justin Chen <justinpopo6@gmail.com>

Hi Justin,

Unfortunately the existing driver is making incorrect use of
the indio_dev->mlock mutex.  That used to get abused a lot before
we clamped down on it being for only one purpose, protection of
the 'mode' of the iio device. It's about stopping anything interfering
with buffered captures by talking to the device in a way that breaks
them.

I'm not sure if the original driver was trying to use it for this
purpose and protecting it's buffers.  Even if so, there should be
an additional local lock to cover those chip specific elements.
Any use of mlock should be through the iio_device_claim_direct_mode
function etc.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-ads7950.c | 166 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 164 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 0ad6359..c54706ad 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -17,6 +17,7 @@
>  #include <linux/bitops.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -36,10 +37,14 @@
>   */
>  #define TI_ADS7950_VA_MV_ACPI_DEFAULT	5000
>  
> +#define TI_ADS7950_CR_GPIO	BIT(14)
>  #define TI_ADS7950_CR_MANUAL	BIT(12)
>  #define TI_ADS7950_CR_WRITE	BIT(11)
>  #define TI_ADS7950_CR_CHAN(ch)	((ch) << 7)
>  #define TI_ADS7950_CR_RANGE_5V	BIT(6)
> +#define TI_ADS7950_CR_GPIO_DATA	BIT(5)
> +#define TI_ADS7950_NUM_GPIOS	4
> +#define TI_ADS7950_GPIO_MASK	GENMASK(TI_ADS7950_NUM_GPIOS - 1, 0)
>  
>  #define TI_ADS7950_MAX_CHAN	16
>  
> @@ -56,11 +61,17 @@ struct ti_ads7950_state {
>  	struct spi_message	ring_msg;
>  	struct spi_message	scan_single_msg;
>  
> +	struct iio_dev		*indio_dev;
> +	struct gpio_chip	*chip;
> +
>  	struct regulator	*reg;
>  	unsigned int		vref_mv;
>  
>  	unsigned int		settings;
>  
> +	unsigned int		gpio_direction_bitmask;
> +	unsigned int		gpio_signal_bitmask;
> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -248,7 +259,8 @@ static int ti_ads7950_update_scan_mode(struct iio_dev *indio_dev,
>  
>  	len = 0;
>  	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels) {
> -		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings;
> +		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings
> +		      | (st->gpio_signal_bitmask & TI_ADS7950_GPIO_MASK);
>  		st->tx_buf[len++] = cmd;
>  	}
>  
> @@ -288,7 +300,8 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  
>  	mutex_lock(&indio_dev->mlock);
>  
> -	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
> +	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings
> +	      | (st->gpio_signal_bitmask & TI_ADS7950_GPIO_MASK);
>  	st->single_tx = cmd;
>  
>  	ret = spi_sync(st->spi, &st->scan_single_msg);
> @@ -362,10 +375,149 @@ static const struct iio_info ti_ads7950_info = {
>  	.update_scan_mode	= ti_ads7950_update_scan_mode,
>  };
>  
> +static void ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
> +			   int value)
> +{
> +	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> +
> +	mutex_lock(&st->indio_dev->mlock);
> +
> +	if (value)
> +		st->gpio_signal_bitmask |= BIT(offset);
> +	else
> +		st->gpio_signal_bitmask &= ~BIT(offset);
> +
> +	st->single_tx = cpu_to_be16(TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_WRITE |
> +			(st->gpio_signal_bitmask & TI_ADS7950_GPIO_MASK));
> +	spi_sync(st->spi, &st->scan_single_msg);
> +
> +	mutex_unlock(&st->indio_dev->mlock);
> +}
> +
> +static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> +	int ret;
> +
> +	mutex_lock(&st->indio_dev->mlock);
> +
> +	/* If set as output, return the output */
> +	if (st->gpio_direction_bitmask & BIT(offset)) {
> +		ret = st->gpio_signal_bitmask & BIT(offset);
> +		goto out;
> +	}
> +
> +	st->single_tx = cpu_to_be16(TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_WRITE |
> +				    TI_ADS7950_CR_GPIO_DATA);
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		goto out;
> +
> +	ret = ((st->single_rx >> 12) & BIT(offset)) ? 1 : 0;
> +
> +out:
> +	mutex_unlock(&st->indio_dev->mlock);
> +
> +	return ret;
> +}
> +
> +static int ti_ads7950_get_direction(struct gpio_chip *chip,
> +				    unsigned int offset)
> +{
> +	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> +
> +	return !(st->gpio_direction_bitmask & BIT(offset));
> +}
> +
> +static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
> +				     int input)
> +{
> +	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> +	int ret = 0;
> +
> +	mutex_lock(&st->indio_dev->mlock);
> +
> +	if (input && (st->gpio_direction_bitmask & BIT(offset)))
> +		st->gpio_direction_bitmask &= ~BIT(offset);
> +	else if (!input && !(st->gpio_direction_bitmask & BIT(offset)))
> +		st->gpio_direction_bitmask |= BIT(offset);
> +	else
> +		goto out;
> +
> +
> +	st->single_tx = cpu_to_be16(TI_ADS7950_CR_GPIO |
> +				    (st->gpio_direction_bitmask &
> +				    TI_ADS7950_GPIO_MASK));
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +
> +out:
> +	mutex_unlock(&st->indio_dev->mlock);
> +
> +	return ret;
> +}
> +
> +static int ti_ads7950_direction_input(struct gpio_chip *chip,
> +				      unsigned int offset)
> +{
> +	return _ti_ads7950_set_direction(chip, offset, 1);
> +}
> +
> +static int ti_ads7950_direction_output(struct gpio_chip *chip,
> +				       unsigned int offset, int value)
> +{
> +	ti_ads7950_set(chip, offset, value);
> +
> +	return _ti_ads7950_set_direction(chip, offset, 0);
> +}
> +
> +static int ti_ads7950_init_gpio(struct ti_ads7950_state *st)
> +{
> +	int ret;
> +
> +	/* Initialize GPIO */
> +	mutex_lock(&st->indio_dev->mlock);
> +
> +	/* Default to GPIO input */
> +	st->gpio_direction_bitmask = 0x0;
> +	st->single_tx = cpu_to_be16(TI_ADS7950_CR_GPIO |
> +				    (st->gpio_direction_bitmask &
> +				    TI_ADS7950_GPIO_MASK));
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	mutex_unlock(&st->indio_dev->mlock);

Nope.  This is a state lock used to protect against transitions between
different modes of the IIO device (buffered vs polled), it
isn't suitable for general use.

The driver should be modified to handle that correctly.
We have iio_claim_direct_mode etc that deal with the case
where a device can't do certain operations whilst in buffered
mode.  Note it can fail and should.

Seems there are more drivers still doing this than I thought.
If anyone is bored and wants to clean them out, that would be
most appreciated!

If you need locking to protect a local buffer or the device
state, define a new lock to do it with clearly documented
scope.

> +	if (ret)
> +		return ret;
> +
> +	/* Default to signal low */
> +	st->gpio_signal_bitmask = 0x0;
> +	st->single_tx = cpu_to_be16(TI_ADS7950_CR_MANUAL |
> +				    TI_ADS7950_CR_WRITE |
> +				    (st->gpio_signal_bitmask &
> +				    TI_ADS7950_GPIO_MASK));
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	mutex_unlock(&st->indio_dev->mlock);
> +	if (ret)
> +		return ret;
> +
> +	/* Add GPIO chip */
> +	st->chip->label = dev_name(&st->spi->dev);
> +	st->chip->parent = &st->spi->dev;
> +	st->chip->owner = THIS_MODULE;
> +	st->chip->base = -1;
> +	st->chip->ngpio = TI_ADS7950_NUM_GPIOS;
> +	st->chip->get_direction = ti_ads7950_get_direction;
> +	st->chip->direction_input = ti_ads7950_direction_input;
> +	st->chip->direction_output = ti_ads7950_direction_output;
> +	st->chip->get = ti_ads7950_get;
> +	st->chip->set = ti_ads7950_set;
> +
> +	return gpiochip_add_data(st->chip, st);
> +}
> +
>  static int ti_ads7950_probe(struct spi_device *spi)
>  {
>  	struct ti_ads7950_state *st;
>  	struct iio_dev *indio_dev;
> +	struct gpio_chip *chip;
>  	const struct ti_ads7950_chip_info *info;
>  	int ret;
>  
> @@ -381,6 +533,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> +	chip = devm_kzalloc(&spi->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +

What stops us putting the chip structure inside the existing
info structure?  That avoids the need to separately allocate
memory and to set the pointer.  A cleaner option all round.

>  	st = iio_priv(indio_dev);
>  
>  	spi_set_drvdata(spi, indio_dev);
> @@ -396,6 +552,8 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	indio_dev->channels = info->channels;
>  	indio_dev->num_channels = info->num_channels;
>  	indio_dev->info = &ti_ads7950_info;
> +	st->indio_dev = indio_dev;

Hmm. As a general rule I'm against things having pointers to the structure
that contains them as it usual means something is inconsistent in the
software architecture.

From the above, looks like this is about accessing mlock. The driver
shouldn't be doing that directly at all, so a precursor to this
patch cleaning that up would be great.

> +	st->chip = chip;
>  
>  	/* build spi ring message */
>  	spi_message_init(&st->ring_msg);
> @@ -457,6 +615,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  		goto error_cleanup_ring;
>  	}
>  
> +	ret = ti_ads7950_init_gpio(st);
> +	if (ret)
> +		dev_warn(&spi->dev, "Unable to initialize GPIOs\n");
Why are we not treating this as an error?  I can't see any
obvious reason to make that choice.

> +
>  	return 0;
>  
>  error_cleanup_ring:


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio: adc: ti-ads7950: add GPIO support
  2019-02-09 17:00 ` Jonathan Cameron
@ 2019-02-09 18:56   ` David Lechner
  2019-02-11 20:05     ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: David Lechner @ 2019-02-09 18:56 UTC (permalink / raw)
  To: Jonathan Cameron, justinpopo6
  Cc: linux-iio, linux-gpio, bcm-kernel-feedback-list, f.fainelli,
	bgolaszewski, linus.walleij, knaack.h, lars, pmeerw,
	linux-kernel

On 2/9/19 11:00 AM, Jonathan Cameron wrote:
> Nope.  This is a state lock used to protect against transitions between
> different modes of the IIO device (buffered vs polled), it
> isn't suitable for general use.
> 
> The driver should be modified to handle that correctly.
> We have iio_claim_direct_mode etc that deal with the case
> where a device can't do certain operations whilst in buffered
> mode.  Note it can fail and should.
> 
> Seems there are more drivers still doing this than I thought.
> If anyone is bored and wants to clean them out, that would be
> most appreciated!
> 
> If you need locking to protect a local buffer or the device
> state, define a new lock to do it with clearly documented
> scope.

Just as a reminder, there is a use case for this particular
chip that requires buffered mode and direct mode at the same
time.

https://patchwork.kernel.org/patch/10539021/
https://patchwork.kernel.org/patch/10527757/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio: adc: ti-ads7950: add GPIO support
  2019-02-08 19:24 [PATCH] iio: adc: ti-ads7950: add GPIO support justinpopo6
  2019-02-09 17:00 ` Jonathan Cameron
@ 2019-02-11  8:34 ` Linus Walleij
  1 sibling, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2019-02-11  8:34 UTC (permalink / raw)
  To: Justin Chen
  Cc: linux-iio, open list:GPIO SUBSYSTEM, bcm-kernel-feedback-list,
	Florian Fainelli, Bartosz Golaszewski, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald,
	David Lechner, linux-kernel

Hi Justin,

thanks for your patch!

On Fri, Feb 8, 2019 at 8:25 PM <justinpopo6@gmail.com> wrote:

> From: Justin Chen <justinpopo6@gmail.com>
>
> The ADS79XX has GPIO pins that can be used. Add support for the GPIO
> pins using the GPIO chip framework.
>
> Signed-off-by: Justin Chen <justinpopo6@gmail.com>
(...)

> @@ -56,11 +61,17 @@ struct ti_ads7950_state {
>         struct spi_message      ring_msg;
>         struct spi_message      scan_single_msg;
>
> +       struct iio_dev          *indio_dev;
> +       struct gpio_chip        *chip;

Why use a pointer here? Correct me if wrong by a struct ti_ads7950_state is
always allocated for each instance of the hardware right?
So just struct gpio_chip chip; should be fine, then just fill in that
struct.

> +       /* Add GPIO chip */
> +       st->chip->label = dev_name(&st->spi->dev);

So it would be st->chip.label = ... etc.

> +       chip = devm_kzalloc(&spi->dev, sizeof(*chip), GFP_KERNEL);
> +       if (!chip)
> +               return -ENOMEM;

And no need to do this.

Apart from that it looks OK to me, but there are some locking comments
I see.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio: adc: ti-ads7950: add GPIO support
  2019-02-09 18:56   ` David Lechner
@ 2019-02-11 20:05     ` Jonathan Cameron
  2019-02-11 20:17       ` Justin Chen
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2019-02-11 20:05 UTC (permalink / raw)
  To: David Lechner
  Cc: justinpopo6, linux-iio, linux-gpio, bcm-kernel-feedback-list,
	f.fainelli, bgolaszewski, linus.walleij, knaack.h, lars, pmeerw,
	linux-kernel

On Sat, 9 Feb 2019 12:56:11 -0600
David Lechner <david@lechnology.com> wrote:

> On 2/9/19 11:00 AM, Jonathan Cameron wrote:
> > Nope.  This is a state lock used to protect against transitions between
> > different modes of the IIO device (buffered vs polled), it
> > isn't suitable for general use.
> > 
> > The driver should be modified to handle that correctly.
> > We have iio_claim_direct_mode etc that deal with the case
> > where a device can't do certain operations whilst in buffered
> > mode.  Note it can fail and should.
> > 
> > Seems there are more drivers still doing this than I thought.
> > If anyone is bored and wants to clean them out, that would be
> > most appreciated!
> > 
> > If you need locking to protect a local buffer or the device
> > state, define a new lock to do it with clearly documented
> > scope.  
> 
> Just as a reminder, there is a use case for this particular
> chip that requires buffered mode and direct mode at the same
> time.
> 
> https://patchwork.kernel.org/patch/10539021/
> https://patchwork.kernel.org/patch/10527757/

Thanks, I had indeed forgotten that entirely.
So it should have a local lock and not take mlock explicitly at all.

Jonathan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio: adc: ti-ads7950: add GPIO support
  2019-02-11 20:05     ` Jonathan Cameron
@ 2019-02-11 20:17       ` Justin Chen
  2019-02-12 20:35         ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Justin Chen @ 2019-02-11 20:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: David Lechner, linux-iio, linux-gpio, bcm-kernel-feedback-list,
	Florian Fainelli, bgolaszewski, Linus Walleij, knaack.h, lars,
	Peter Meerwald-Stadler, linux-kernel

On Mon, Feb 11, 2019 at 12:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 9 Feb 2019 12:56:11 -0600
> David Lechner <david@lechnology.com> wrote:
>
> > On 2/9/19 11:00 AM, Jonathan Cameron wrote:
> > > Nope.  This is a state lock used to protect against transitions between
> > > different modes of the IIO device (buffered vs polled), it
> > > isn't suitable for general use.
> > >
> > > The driver should be modified to handle that correctly.
> > > We have iio_claim_direct_mode etc that deal with the case
> > > where a device can't do certain operations whilst in buffered
> > > mode.  Note it can fail and should.
> > >
> > > Seems there are more drivers still doing this than I thought.
> > > If anyone is bored and wants to clean them out, that would be
> > > most appreciated!
> > >
> > > If you need locking to protect a local buffer or the device
> > > state, define a new lock to do it with clearly documented
> > > scope.
> >
> > Just as a reminder, there is a use case for this particular
> > chip that requires buffered mode and direct mode at the same
> > time.
> >
> > https://patchwork.kernel.org/patch/10539021/
> > https://patchwork.kernel.org/patch/10527757/
>
> Thanks, I had indeed forgotten that entirely.
> So it should have a local lock and not take mlock explicitly at all.
>
Thanks for all the feedback.
So If I am understanding this correctly. I should create a local lock
to synchronize three different type of transactions (buffered, direct,
gpio).
I do not want to use iio_claim_direct_mode because that doesn't allow
simultaneous use of buffered mode and direct mode, which is necessary
for this driver because of the above mentioned patch.

Justin
> Jonathan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio: adc: ti-ads7950: add GPIO support
  2019-02-11 20:17       ` Justin Chen
@ 2019-02-12 20:35         ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2019-02-12 20:35 UTC (permalink / raw)
  To: Justin Chen
  Cc: David Lechner, linux-iio, linux-gpio, bcm-kernel-feedback-list,
	Florian Fainelli, bgolaszewski, Linus Walleij, knaack.h, lars,
	Peter Meerwald-Stadler, linux-kernel

On Mon, 11 Feb 2019 12:17:37 -0800
Justin Chen <justinpopo6@gmail.com> wrote:

> On Mon, Feb 11, 2019 at 12:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 9 Feb 2019 12:56:11 -0600
> > David Lechner <david@lechnology.com> wrote:
> >  
> > > On 2/9/19 11:00 AM, Jonathan Cameron wrote:  
> > > > Nope.  This is a state lock used to protect against transitions between
> > > > different modes of the IIO device (buffered vs polled), it
> > > > isn't suitable for general use.
> > > >
> > > > The driver should be modified to handle that correctly.
> > > > We have iio_claim_direct_mode etc that deal with the case
> > > > where a device can't do certain operations whilst in buffered
> > > > mode.  Note it can fail and should.
> > > >
> > > > Seems there are more drivers still doing this than I thought.
> > > > If anyone is bored and wants to clean them out, that would be
> > > > most appreciated!
> > > >
> > > > If you need locking to protect a local buffer or the device
> > > > state, define a new lock to do it with clearly documented
> > > > scope.  
> > >
> > > Just as a reminder, there is a use case for this particular
> > > chip that requires buffered mode and direct mode at the same
> > > time.
> > >
> > > https://patchwork.kernel.org/patch/10539021/
> > > https://patchwork.kernel.org/patch/10527757/  
> >
> > Thanks, I had indeed forgotten that entirely.
> > So it should have a local lock and not take mlock explicitly at all.
> >  
> Thanks for all the feedback.
> So If I am understanding this correctly. I should create a local lock
> to synchronize three different type of transactions (buffered, direct,
> gpio).
> I do not want to use iio_claim_direct_mode because that doesn't allow
> simultaneous use of buffered mode and direct mode, which is necessary
> for this driver because of the above mentioned patch.
No, you should create a local lock to do just one thing. Protect
the buffers used in those transactions.  There is no need
to protect anything else explicitly.

On some devices there are multiple transaction writes, but I don't recall
that being the case here.

Jonathan

> 
> Justin
> > Jonathan  


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08 19:24 [PATCH] iio: adc: ti-ads7950: add GPIO support justinpopo6
2019-02-09 17:00 ` Jonathan Cameron
2019-02-09 18:56   ` David Lechner
2019-02-11 20:05     ` Jonathan Cameron
2019-02-11 20:17       ` Justin Chen
2019-02-12 20:35         ` Jonathan Cameron
2019-02-11  8:34 ` Linus Walleij

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox