[PATCH v4 0/2] iio: adc: ads7950: add gpio support

[PATCH v4 0/2] iio: adc: ads7950: add gpio support

[justinpopo6]

From: Justin Chen <justinpopo6@gmail.com>

Add GPIO support for ads7950.

v4
Split patch into two commits.
Refractored code to capture the state of the adc instead of only the GPIOs.
Added comments to clarify the intend of the code.
Fix improper use of mlock.

Justin Chen (2):
  iio: adc: ti-ads7950: Fix improper use of mlock
  iio: adc: ti-ads7950: add GPIO support

 drivers/iio/adc/ti-ads7950.c | 219 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 209 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v4 1/2] iio: adc: ti-ads7950: Fix improper use of mlock

[justinpopo6]

From: Justin Chen <justinpopo6@gmail.com>

Indio->mlock is used for protecting the different iio device modes.
It is currently not being used in this way. Replace the lock with
an internal lock specifically used for protecting the SPI transfer
buffer.

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

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 0ad6359..1e47bef 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -56,6 +56,9 @@ struct ti_ads7950_state {
 	struct spi_message	ring_msg;
 	struct spi_message	scan_single_msg;
 
+	/* Lock to protect the spi xfer buffers */
+	struct mutex		slock;
+
 	struct regulator	*reg;
 	unsigned int		vref_mv;
 
@@ -268,6 +271,7 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
 	struct ti_ads7950_state *st = iio_priv(indio_dev);
 	int ret;
 
+	mutex_lock(&st->slock);
 	ret = spi_sync(st->spi, &st->ring_msg);
 	if (ret < 0)
 		goto out;
@@ -276,6 +280,7 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
 					   iio_get_time_ns(indio_dev));
 
 out:
+	mutex_unlock(&st->slock);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -286,7 +291,7 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 	struct ti_ads7950_state *st = iio_priv(indio_dev);
 	int ret, cmd;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->slock);
 
 	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
 	st->single_tx = cmd;
@@ -298,7 +303,7 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 	ret = st->single_rx;
 
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->slock);
 
 	return ret;
 }
@@ -432,16 +437,19 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	if (ACPI_COMPANION(&spi->dev))
 		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
 
+	mutex_init(&st->slock);
+
 	st->reg = devm_regulator_get(&spi->dev, "vref");
 	if (IS_ERR(st->reg)) {
 		dev_err(&spi->dev, "Failed get get regulator \"vref\"\n");
-		return PTR_ERR(st->reg);
+		ret = PTR_ERR(st->reg);
+		goto error_destroy_mutex;
 	}
 
 	ret = regulator_enable(st->reg);
 	if (ret) {
 		dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
-		return ret;
+		goto error_destroy_mutex;
 	}
 
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
@@ -463,6 +471,8 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	iio_triggered_buffer_cleanup(indio_dev);
 error_disable_reg:
 	regulator_disable(st->reg);
+error_destroy_mutex:
+	mutex_destroy(&st->slock);
 
 	return ret;
 }
@@ -475,6 +485,7 @@ static int ti_ads7950_remove(struct spi_device *spi)
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
 	regulator_disable(st->reg);
+	mutex_destroy(&st->slock);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v4 2/2] iio: adc: ti-ads7950: add GPIO support

[justinpopo6]

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 | 200 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 194 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 1e47bef..0964053 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,12 +37,15 @@
  */
 #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(4)
 
 #define TI_ADS7950_MAX_CHAN	16
+#define TI_ADS7950_NUM_GPIOS	4
 
 #define TI_ADS7950_TIMESTAMP_SIZE (sizeof(int64_t) / sizeof(__be16))
 
@@ -49,6 +53,16 @@
 #define TI_ADS7950_EXTRACT(val, dec, bits) \
 	(((val) >> (dec)) & ((1 << (bits)) - 1))
 
+#define TI_ADS7950_MAN_CMD(cmd)         (TI_ADS7950_CR_MANUAL | (cmd))
+#define TI_ADS7950_GPIO_CMD(cmd)        (TI_ADS7950_CR_GPIO | (cmd))
+
+/* Manual mode configuration */
+#define TI_ADS7950_MAN_CMD_SETTINGS(st) \
+	(TI_ADS7950_MAN_CMD(TI_ADS7950_CR_WRITE | st->cmd_settings_bitmask))
+/* GPIO mode configuration */
+#define TI_ADS7950_GPIO_CMD_SETTINGS(st) \
+	(TI_ADS7950_GPIO_CMD(st->gpio_cmd_settings_bitmask))
+
 struct ti_ads7950_state {
 	struct spi_device	*spi;
 	struct spi_transfer	ring_xfer;
@@ -58,11 +72,34 @@ struct ti_ads7950_state {
 
 	/* Lock to protect the spi xfer buffers */
 	struct mutex		slock;
+	struct gpio_chip	chip;
 
 	struct regulator	*reg;
 	unsigned int		vref_mv;
 
-	unsigned int		settings;
+	/*
+	 * Bitmask of lower 7 bits used for configuration
+	 * These bits only can be written when TI_ADS7950_CR_WRITE
+	 * is set, otherwise it retains its original state.
+	 * [0-3] GPIO signal
+	 * [4]   Set following frame to return GPIO signal values
+	 * [5]   Powers down device
+	 * [6]   Sets Vref range1(2.5v) or range2(5v)
+	 *
+	 * Bits present on Manual/Auto1/Auto2 commands
+	 */
+	unsigned int		cmd_settings_bitmask;
+
+	/*
+	 * Bitmask of GPIO command
+	 * [0-3] GPIO direction
+	 * [4-6] Different GPIO alarm mode configurations
+	 * [7]   GPIO 2 as device range input
+	 * [8]   GPIO 3 as device power down input
+	 * [9]   Reset all registers
+	 * [10-11] N/A
+	 */
+	unsigned int		gpio_cmd_settings_bitmask;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -251,7 +288,7 @@ 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_MAN_CMD(TI_ADS7950_CR_CHAN(i));
 		st->tx_buf[len++] = cmd;
 	}
 
@@ -292,8 +329,7 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 	int ret, cmd;
 
 	mutex_lock(&st->slock);
-
-	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
+	cmd = TI_ADS7950_MAN_CMD(TI_ADS7950_CR_CHAN(ch));
 	st->single_tx = cmd;
 
 	ret = spi_sync(st->spi, &st->scan_single_msg);
@@ -322,7 +358,7 @@ static int ti_ads7950_get_range(struct ti_ads7950_state *st)
 		vref /= 1000;
 	}
 
-	if (st->settings & TI_ADS7950_CR_RANGE_5V)
+	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
 		vref *= 2;
 
 	return vref;
@@ -367,6 +403,135 @@ 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->slock);
+
+	if (value)
+		st->cmd_settings_bitmask |= BIT(offset);
+	else
+		st->cmd_settings_bitmask &= ~BIT(offset);
+
+	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
+	spi_sync(st->spi, &st->scan_single_msg);
+
+	mutex_unlock(&st->slock);
+}
+
+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->slock);
+
+	/* If set as output, return the output */
+	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
+		ret = st->cmd_settings_bitmask & BIT(offset);
+		goto out;
+	}
+
+	/* GPIO data bit sets SDO bits 12-15 to GPIO input */
+	st->cmd_settings_bitmask |= TI_ADS7950_CR_GPIO_DATA;
+	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		goto out;
+
+	ret = ((st->single_rx >> 12) & BIT(offset)) ? 1 : 0;
+
+	/* Revert back to original settings */
+	st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
+	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		goto out;
+
+out:
+	mutex_unlock(&st->slock);
+
+	return ret;
+}
+
+static int ti_ads7950_get_direction(struct gpio_chip *chip,
+				    unsigned int offset)
+{
+	struct ti_ads7950_state *st = gpiochip_get_data(chip);
+
+	/* Bitmask is inverted from GPIO framework 0=input/1=output */
+	return !(st->gpio_cmd_settings_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->slock);
+
+	/* Only change direction if needed */
+	if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
+		st->gpio_cmd_settings_bitmask &= ~BIT(offset);
+	else if (!input && !(st->gpio_cmd_settings_bitmask & BIT(offset)))
+		st->gpio_cmd_settings_bitmask |= BIT(offset);
+	else
+		goto out;
+
+	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+
+out:
+	mutex_unlock(&st->slock);
+
+	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_hw(struct ti_ads7950_state *st)
+{
+	int ret = 0;
+
+	mutex_lock(&st->slock);
+
+	/* Settings for Manual/Auto1/Auto2 commands */
+	/* Default to 5v ref */
+	st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
+	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		goto out;
+
+	/* Settings for GPIO command */
+	st->gpio_cmd_settings_bitmask = 0x0;
+	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	mutex_unlock(&st->slock);
+	if (ret)
+		goto out;
+
+out:
+	mutex_unlock(&st->slock);
+
+	return ret;
+}
+
 static int ti_ads7950_probe(struct spi_device *spi)
 {
 	struct ti_ads7950_state *st;
@@ -391,7 +556,6 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, indio_dev);
 
 	st->spi = spi;
-	st->settings = TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_RANGE_5V;
 
 	info = &ti_ads7950_chip_info[spi_get_device_id(spi)->driver_data];
 
@@ -465,6 +629,30 @@ static int ti_ads7950_probe(struct spi_device *spi)
 		goto error_cleanup_ring;
 	}
 
+	ret = ti_ads7950_init_hw(st);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to init adc chip\n");
+		goto error_cleanup_ring;
+	}
+
+	/* 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;
+
+	ret = gpiochip_add_data(&st->chip, st);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to init GPIOs\n");
+		goto error_cleanup_ring;
+	}
+
 	return 0;
 
 error_cleanup_ring:
-- 
2.7.4

Re: [PATCH v4 2/2] iio: adc: ti-ads7950: add GPIO support

[Linus Walleij]

On Thu, Feb 28, 2019 at 11:17 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>

This looks good from a GPIO point of view.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 2/2] iio: adc: ti-ads7950: add GPIO support

[Jonathan Cameron]

On Thu, 28 Feb 2019 14:16:49 -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

A few minor things below, particularly around the ordering in probe and
possible missing cleanup in the case of errors.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-ads7950.c | 200 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 194 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 1e47bef..0964053 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,12 +37,15 @@
>   */
>  #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(4)
>  
>  #define TI_ADS7950_MAX_CHAN	16
> +#define TI_ADS7950_NUM_GPIOS	4
>  
>  #define TI_ADS7950_TIMESTAMP_SIZE (sizeof(int64_t) / sizeof(__be16))
>  
> @@ -49,6 +53,16 @@
>  #define TI_ADS7950_EXTRACT(val, dec, bits) \
>  	(((val) >> (dec)) & ((1 << (bits)) - 1))
>  
> +#define TI_ADS7950_MAN_CMD(cmd)         (TI_ADS7950_CR_MANUAL | (cmd))
> +#define TI_ADS7950_GPIO_CMD(cmd)        (TI_ADS7950_CR_GPIO | (cmd))
> +
> +/* Manual mode configuration */
> +#define TI_ADS7950_MAN_CMD_SETTINGS(st) \
> +	(TI_ADS7950_MAN_CMD(TI_ADS7950_CR_WRITE | st->cmd_settings_bitmask))
> +/* GPIO mode configuration */
> +#define TI_ADS7950_GPIO_CMD_SETTINGS(st) \
> +	(TI_ADS7950_GPIO_CMD(st->gpio_cmd_settings_bitmask))
> +
>  struct ti_ads7950_state {
>  	struct spi_device	*spi;
>  	struct spi_transfer	ring_xfer;
> @@ -58,11 +72,34 @@ struct ti_ads7950_state {
>  
>  	/* Lock to protect the spi xfer buffers */
>  	struct mutex		slock;
> +	struct gpio_chip	chip;
>  
>  	struct regulator	*reg;
>  	unsigned int		vref_mv;
>  
> -	unsigned int		settings;
> +	/*
> +	 * Bitmask of lower 7 bits used for configuration
> +	 * These bits only can be written when TI_ADS7950_CR_WRITE
> +	 * is set, otherwise it retains its original state.
> +	 * [0-3] GPIO signal
> +	 * [4]   Set following frame to return GPIO signal values
> +	 * [5]   Powers down device
> +	 * [6]   Sets Vref range1(2.5v) or range2(5v)
> +	 *
> +	 * Bits present on Manual/Auto1/Auto2 commands
> +	 */
> +	unsigned int		cmd_settings_bitmask;
> +
> +	/*
> +	 * Bitmask of GPIO command
> +	 * [0-3] GPIO direction
> +	 * [4-6] Different GPIO alarm mode configurations
> +	 * [7]   GPIO 2 as device range input
> +	 * [8]   GPIO 3 as device power down input
> +	 * [9]   Reset all registers
> +	 * [10-11] N/A
> +	 */
> +	unsigned int		gpio_cmd_settings_bitmask;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
> @@ -251,7 +288,7 @@ 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_MAN_CMD(TI_ADS7950_CR_CHAN(i));
>  		st->tx_buf[len++] = cmd;
>  	}
>  
> @@ -292,8 +329,7 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  	int ret, cmd;
>  
>  	mutex_lock(&st->slock);
> -
> -	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
> +	cmd = TI_ADS7950_MAN_CMD(TI_ADS7950_CR_CHAN(ch));
>  	st->single_tx = cmd;
>  
>  	ret = spi_sync(st->spi, &st->scan_single_msg);
> @@ -322,7 +358,7 @@ static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>  		vref /= 1000;
>  	}
>  
> -	if (st->settings & TI_ADS7950_CR_RANGE_5V)
> +	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
>  		vref *= 2;
>  
>  	return vref;
> @@ -367,6 +403,135 @@ 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->slock);
> +
> +	if (value)
> +		st->cmd_settings_bitmask |= BIT(offset);
> +	else
> +		st->cmd_settings_bitmask &= ~BIT(offset);
> +
> +	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> +	spi_sync(st->spi, &st->scan_single_msg);
> +
> +	mutex_unlock(&st->slock);
> +}
> +
> +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->slock);
> +
> +	/* If set as output, return the output */
> +	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
> +		ret = st->cmd_settings_bitmask & BIT(offset);
> +		goto out;
> +	}
> +
> +	/* GPIO data bit sets SDO bits 12-15 to GPIO input */
> +	st->cmd_settings_bitmask |= TI_ADS7950_CR_GPIO_DATA;
> +	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		goto out;
> +
> +	ret = ((st->single_rx >> 12) & BIT(offset)) ? 1 : 0;
> +
> +	/* Revert back to original settings */
> +	st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
> +	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		goto out;
> +
> +out:
> +	mutex_unlock(&st->slock);
> +
> +	return ret;
> +}
> +
> +static int ti_ads7950_get_direction(struct gpio_chip *chip,
> +				    unsigned int offset)
> +{
> +	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> +
> +	/* Bitmask is inverted from GPIO framework 0=input/1=output */
> +	return !(st->gpio_cmd_settings_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->slock);
> +
> +	/* Only change direction if needed */
> +	if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
> +		st->gpio_cmd_settings_bitmask &= ~BIT(offset);
> +	else if (!input && !(st->gpio_cmd_settings_bitmask & BIT(offset)))
> +		st->gpio_cmd_settings_bitmask |= BIT(offset);
> +	else
> +		goto out;
> +
> +	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +
> +out:
> +	mutex_unlock(&st->slock);
> +
> +	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_hw(struct ti_ads7950_state *st)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&st->slock);
> +
> +	/* Settings for Manual/Auto1/Auto2 commands */
> +	/* Default to 5v ref */
> +	st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
> +	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		goto out;
> +
> +	/* Settings for GPIO command */
> +	st->gpio_cmd_settings_bitmask = 0x0;
> +	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	mutex_unlock(&st->slock);
> +	if (ret)
> +		goto out;

This path doesn't actually differently than if you
drop the two lines above.  Hence just drop them.

> +
> +out:
> +	mutex_unlock(&st->slock);
> +
> +	return ret;
> +}
> +
>  static int ti_ads7950_probe(struct spi_device *spi)
>  {
>  	struct ti_ads7950_state *st;
> @@ -391,7 +556,6 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, indio_dev);
>  
>  	st->spi = spi;
> -	st->settings = TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_RANGE_5V;
>  
>  	info = &ti_ads7950_chip_info[spi_get_device_id(spi)->driver_data];
>  
> @@ -465,6 +629,30 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  		goto error_cleanup_ring;
>  	}
>  
> +	ret = ti_ads7950_init_hw(st);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to init adc chip\n");
> +		goto error_cleanup_ring;
> +	}

At this point the IIO userspace interface is exposed.  Seems likely that
we should have called this earlier.  Also if it fails, you want to unregister
the iio_device.

> +
> +	/* 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;
> +
> +	ret = gpiochip_add_data(&st->chip, st);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to init GPIOs\n");
> +		goto error_cleanup_ring;

What about unregistering the iio_device?

> +	}
> +
>  	return 0;
>  
>  error_cleanup_ring:

Re: [PATCH v4 1/2] iio: adc: ti-ads7950: Fix improper use of mlock

[Jonathan Cameron]

On Thu, 28 Feb 2019 14:16:48 -0800
justinpopo6@gmail.com wrote:

> From: Justin Chen <justinpopo6@gmail.com>
> 
> Indio->mlock is used for protecting the different iio device modes.
> It is currently not being used in this way. Replace the lock with
> an internal lock specifically used for protecting the SPI transfer
> buffer.
> 
> Signed-off-by: Justin Chen <justinpopo6@gmail.com>

Great, thanks for doing this.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-ads7950.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 0ad6359..1e47bef 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -56,6 +56,9 @@ struct ti_ads7950_state {
>  	struct spi_message	ring_msg;
>  	struct spi_message	scan_single_msg;
>  
> +	/* Lock to protect the spi xfer buffers */
> +	struct mutex		slock;
> +
>  	struct regulator	*reg;
>  	unsigned int		vref_mv;
>  
> @@ -268,6 +271,7 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> +	mutex_lock(&st->slock);
>  	ret = spi_sync(st->spi, &st->ring_msg);
>  	if (ret < 0)
>  		goto out;
> @@ -276,6 +280,7 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>  					   iio_get_time_ns(indio_dev));
>  
>  out:
> +	mutex_unlock(&st->slock);
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
> @@ -286,7 +291,7 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
>  	int ret, cmd;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->slock);
>  
>  	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
>  	st->single_tx = cmd;
> @@ -298,7 +303,7 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  	ret = st->single_rx;
>  
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->slock);
>  
>  	return ret;
>  }
> @@ -432,16 +437,19 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	if (ACPI_COMPANION(&spi->dev))
>  		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
>  
> +	mutex_init(&st->slock);
> +
>  	st->reg = devm_regulator_get(&spi->dev, "vref");
>  	if (IS_ERR(st->reg)) {
>  		dev_err(&spi->dev, "Failed get get regulator \"vref\"\n");
> -		return PTR_ERR(st->reg);
> +		ret = PTR_ERR(st->reg);
> +		goto error_destroy_mutex;
>  	}
>  
>  	ret = regulator_enable(st->reg);
>  	if (ret) {
>  		dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
> -		return ret;
> +		goto error_destroy_mutex;
>  	}
>  
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> @@ -463,6 +471,8 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	iio_triggered_buffer_cleanup(indio_dev);
>  error_disable_reg:
>  	regulator_disable(st->reg);
> +error_destroy_mutex:
> +	mutex_destroy(&st->slock);
>  
>  	return ret;
>  }
> @@ -475,6 +485,7 @@ static int ti_ads7950_remove(struct spi_device *spi)
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	regulator_disable(st->reg);
> +	mutex_destroy(&st->slock);
>  
>  	return 0;
>  }

Re: [PATCH v4 2/2] iio: adc: ti-ads7950: add GPIO support

[Dan Carpenter]

Hi Justin,

url:    https://github.com/0day-ci/linux/commits/justinpopo6-gmail-com/iio-adc-ads7950-add-gpio-support/20190301-201828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg

smatch warnings:
drivers/iio/adc/ti-ads7950.c:530 ti_ads7950_init_hw() error: double unlock 'mutex:&st->slock'

# https://github.com/0day-ci/linux/commit/5290a7752b734ae0b2f5b343bd761366decdbbeb
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 5290a7752b734ae0b2f5b343bd761366decdbbeb
vim +530 drivers/iio/adc/ti-ads7950.c

5290a775 Justin Chen 2019-02-28  506  
5290a775 Justin Chen 2019-02-28  507  static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
5290a775 Justin Chen 2019-02-28  508  {
5290a775 Justin Chen 2019-02-28  509  	int ret = 0;
5290a775 Justin Chen 2019-02-28  510  
5290a775 Justin Chen 2019-02-28  511  	mutex_lock(&st->slock);
5290a775 Justin Chen 2019-02-28  512  
5290a775 Justin Chen 2019-02-28  513  	/* Settings for Manual/Auto1/Auto2 commands */
5290a775 Justin Chen 2019-02-28  514  	/* Default to 5v ref */
5290a775 Justin Chen 2019-02-28  515  	st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
5290a775 Justin Chen 2019-02-28  516  	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
5290a775 Justin Chen 2019-02-28  517  	ret = spi_sync(st->spi, &st->scan_single_msg);
5290a775 Justin Chen 2019-02-28  518  	if (ret)
5290a775 Justin Chen 2019-02-28  519  		goto out;
5290a775 Justin Chen 2019-02-28  520  
5290a775 Justin Chen 2019-02-28  521  	/* Settings for GPIO command */
5290a775 Justin Chen 2019-02-28  522  	st->gpio_cmd_settings_bitmask = 0x0;
5290a775 Justin Chen 2019-02-28  523  	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
5290a775 Justin Chen 2019-02-28  524  	ret = spi_sync(st->spi, &st->scan_single_msg);
5290a775 Justin Chen 2019-02-28  525  	mutex_unlock(&st->slock);
                                        ^^^^^^^^^^^^^^^^^^^^^^^^

5290a775 Justin Chen 2019-02-28  526  	if (ret)
5290a775 Justin Chen 2019-02-28  527  		goto out;
5290a775 Justin Chen 2019-02-28  528  
5290a775 Justin Chen 2019-02-28  529  out:
5290a775 Justin Chen 2019-02-28 @530  	mutex_unlock(&st->slock);
                                        ^^^^^^^^^^^^^^^^^^^^^^^^

It's a bit disappointing that mutex_unlock() doesn't generate a warning
at runtime...  :(

5290a775 Justin Chen 2019-02-28  531  
5290a775 Justin Chen 2019-02-28  532  	return ret;
5290a775 Justin Chen 2019-02-28  533  }
5290a775 Justin Chen 2019-02-28  534  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation