* [PATCH v5 0/2] iio: adc: ads7950: add gpio support @ 2019-03-08 22:03 justinpopo6 2019-03-08 22:03 ` [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock justinpopo6 2019-03-08 22:03 ` [PATCH v5 2/2] iio: adc: ti-ads7950: add GPIO support justinpopo6 0 siblings, 2 replies; 7+ messages in thread From: justinpopo6 @ 2019-03-08 22:03 UTC (permalink / raw) To: linux-iio Cc: linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, jic23, knaack.h, lars, pmeerw, linux-kernel, Justin Chen From: Justin Chen <justinpopo6@gmail.com> v5 Fixed ordering of how things are initalized. Fixed incomplete cleanup on error. Fixed double mutex_unlock. 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock 2019-03-08 22:03 [PATCH v5 0/2] iio: adc: ads7950: add gpio support justinpopo6 @ 2019-03-08 22:03 ` justinpopo6 2019-03-09 16:29 ` Tomasz Duszynski 2019-03-08 22:03 ` [PATCH v5 2/2] iio: adc: ti-ads7950: add GPIO support justinpopo6 1 sibling, 1 reply; 7+ messages in thread From: justinpopo6 @ 2019-03-08 22:03 UTC (permalink / raw) To: linux-iio Cc: linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, jic23, knaack.h, lars, pmeerw, linux-kernel, Justin Chen 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock 2019-03-08 22:03 ` [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock justinpopo6 @ 2019-03-09 16:29 ` Tomasz Duszynski 2019-03-09 18:37 ` Jonathan Cameron 0 siblings, 1 reply; 7+ messages in thread From: Tomasz Duszynski @ 2019-03-09 16:29 UTC (permalink / raw) To: justinpopo6 Cc: linux-iio, linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, jic23, knaack.h, lars, pmeerw, linux-kernel On Fri, Mar 08, 2019 at 02:03:27PM -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> > --- > 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); If your intention was to do resources cleanup then this is not what this api was designed for. This is actually for debugging unwanted subsequent mutex usage. > > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock 2019-03-09 16:29 ` Tomasz Duszynski @ 2019-03-09 18:37 ` Jonathan Cameron 2019-03-09 18:40 ` Jonathan Cameron 0 siblings, 1 reply; 7+ messages in thread From: Jonathan Cameron @ 2019-03-09 18:37 UTC (permalink / raw) To: Tomasz Duszynski Cc: justinpopo6, linux-iio, linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, knaack.h, lars, pmeerw, linux-kernel On Sat, 9 Mar 2019 17:29:36 +0100 Tomasz Duszynski <tduszyns@gmail.com> wrote: > On Fri, Mar 08, 2019 at 02:03:27PM -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> > > --- > > 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); > > If your intention was to do resources cleanup then this is not > what this api was designed for. This is actually for debugging unwanted > subsequent mutex usage. Yes. In a case like this where it is the last thing in a remove it adds little value as there should be nothing left to take the mutex anyway. This is the reason (I guess) there has never been a devm_mutex_init function to tidy this up automatically... > > > > > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock 2019-03-09 18:37 ` Jonathan Cameron @ 2019-03-09 18:40 ` Jonathan Cameron 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2019-03-09 18:40 UTC (permalink / raw) To: Tomasz Duszynski Cc: justinpopo6, linux-iio, linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, knaack.h, lars, pmeerw, linux-kernel On Sat, 9 Mar 2019 18:37:01 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 9 Mar 2019 17:29:36 +0100 > Tomasz Duszynski <tduszyns@gmail.com> wrote: > > > On Fri, Mar 08, 2019 at 02:03:27PM -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> > > > --- > > > 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); > > > > If your intention was to do resources cleanup then this is not > > what this api was designed for. This is actually for debugging unwanted > > subsequent mutex usage. > > Yes. In a case like this where it is the last thing in a remove > it adds little value as there should be nothing left to take the mutex > anyway. This is the reason (I guess) there has never been a > devm_mutex_init function to tidy this up automatically... > Having said that, I just realised I applied this anyway last week. I'm not fussed enough about this to revert the change, so right now the mutex_destroys are there. Jonathan > > > > > > > > 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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v5 2/2] iio: adc: ti-ads7950: add GPIO support 2019-03-08 22:03 [PATCH v5 0/2] iio: adc: ads7950: add gpio support justinpopo6 2019-03-08 22:03 ` [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock justinpopo6 @ 2019-03-08 22:03 ` justinpopo6 2019-03-09 18:42 ` Jonathan Cameron 1 sibling, 1 reply; 7+ messages in thread From: justinpopo6 @ 2019-03-08 22:03 UTC (permalink / raw) To: linux-iio Cc: linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, jic23, knaack.h, lars, pmeerw, 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> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> --- 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..2e66e4d 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,132 @@ 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); + +out: + mutex_unlock(&st->slock); + + return ret; +} + static int ti_ads7950_probe(struct spi_device *spi) { struct ti_ads7950_state *st; @@ -391,7 +553,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]; @@ -459,14 +620,40 @@ static int ti_ads7950_probe(struct spi_device *spi) goto error_disable_reg; } + ret = ti_ads7950_init_hw(st); + if (ret) { + dev_err(&spi->dev, "Failed to init adc chip\n"); + goto error_cleanup_ring; + } + ret = iio_device_register(indio_dev); if (ret) { dev_err(&spi->dev, "Failed to register iio device\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_iio_device; + } + return 0; +error_iio_device: + iio_device_unregister(indio_dev); error_cleanup_ring: iio_triggered_buffer_cleanup(indio_dev); error_disable_reg: @@ -482,6 +669,7 @@ static int ti_ads7950_remove(struct spi_device *spi) struct iio_dev *indio_dev = spi_get_drvdata(spi); struct ti_ads7950_state *st = iio_priv(indio_dev); + gpiochip_remove(&st->chip); iio_device_unregister(indio_dev); iio_triggered_buffer_cleanup(indio_dev); regulator_disable(st->reg); -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5 2/2] iio: adc: ti-ads7950: add GPIO support 2019-03-08 22:03 ` [PATCH v5 2/2] iio: adc: ti-ads7950: add GPIO support justinpopo6 @ 2019-03-09 18:42 ` Jonathan Cameron 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2019-03-09 18:42 UTC (permalink / raw) To: justinpopo6 Cc: linux-iio, linux-gpio, bcm-kernel-feedback-list, f.fainelli, bgolaszewski, linus.walleij, knaack.h, lars, pmeerw, linux-kernel On Fri, 8 Mar 2019 14:03:28 -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> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> 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 | 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..2e66e4d 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,132 @@ 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); > + > +out: > + mutex_unlock(&st->slock); > + > + return ret; > +} > + > static int ti_ads7950_probe(struct spi_device *spi) > { > struct ti_ads7950_state *st; > @@ -391,7 +553,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]; > > @@ -459,14 +620,40 @@ static int ti_ads7950_probe(struct spi_device *spi) > goto error_disable_reg; > } > > + ret = ti_ads7950_init_hw(st); > + if (ret) { > + dev_err(&spi->dev, "Failed to init adc chip\n"); > + goto error_cleanup_ring; > + } > + > ret = iio_device_register(indio_dev); > if (ret) { > dev_err(&spi->dev, "Failed to register iio device\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_iio_device; > + } > + > return 0; > > +error_iio_device: > + iio_device_unregister(indio_dev); > error_cleanup_ring: > iio_triggered_buffer_cleanup(indio_dev); > error_disable_reg: > @@ -482,6 +669,7 @@ static int ti_ads7950_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > struct ti_ads7950_state *st = iio_priv(indio_dev); > > + gpiochip_remove(&st->chip); > iio_device_unregister(indio_dev); > iio_triggered_buffer_cleanup(indio_dev); > regulator_disable(st->reg); ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-09 18:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-08 22:03 [PATCH v5 0/2] iio: adc: ads7950: add gpio support justinpopo6 2019-03-08 22:03 ` [PATCH v5 1/2] iio: adc: ti-ads7950: Fix improper use of mlock justinpopo6 2019-03-09 16:29 ` Tomasz Duszynski 2019-03-09 18:37 ` Jonathan Cameron 2019-03-09 18:40 ` Jonathan Cameron 2019-03-08 22:03 ` [PATCH v5 2/2] iio: adc: ti-ads7950: add GPIO support justinpopo6 2019-03-09 18:42 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).