iio: adf4350: Convert to use GPIO descriptor
diff mbox series

Message ID 20191202083830.71572-1-linus.walleij@linaro.org
State New
Headers show
Series
  • iio: adf4350: Convert to use GPIO descriptor
Related show

Commit Message

Linus Walleij Dec. 2, 2019, 8:38 a.m. UTC
The lock detect GPIO line is better to grab using
a GPIO descriptor. We drop the pdata for this: clients using board
files can use machine descriptor tables to pass this GPIO from
static data.

Cc: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/frequency/adf4350.c       | 30 +++++++--------------------
 include/linux/iio/frequency/adf4350.h |  4 ----
 2 files changed, 8 insertions(+), 26 deletions(-)

Comments

Ardelean, Alexandru Dec. 2, 2019, 8:50 a.m. UTC | #1
On Mon, 2019-12-02 at 09:38 +0100, Linus Walleij wrote:
> The lock detect GPIO line is better to grab using
> a GPIO descriptor. We drop the pdata for this: clients using board
> files can use machine descriptor tables to pass this GPIO from
> static data.

Hey,

Thanks for the patch.
A question inline. Maybe to Jonathan as well.
Other than that this looks good.

> 
> Cc: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/frequency/adf4350.c       | 30 +++++++--------------------
>  include/linux/iio/frequency/adf4350.h |  4 ----
>  2 files changed, 8 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c
> b/drivers/iio/frequency/adf4350.c
> index ae0ca09ae062..1c2dc9b00f31 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -14,11 +14,10 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/gcd.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <asm/div64.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
> -#include <linux/of_gpio.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -34,6 +33,7 @@ enum {
>  struct adf4350_state {
>  	struct spi_device		*spi;
>  	struct regulator		*reg;
> +	struct gpio_desc		*lock_detect_gpiod;
>  	struct adf4350_platform_data	*pdata;
>  	struct clk			*clk;
>  	unsigned long			clkin;
> @@ -61,7 +61,6 @@ static struct adf4350_platform_data default_pdata = {
>  	.r3_user_settings = ADF4350_REG3_12BIT_CLKDIV_MODE(0),
>  	.r4_user_settings = ADF4350_REG4_OUTPUT_PWR(3) |
>  			    ADF4350_REG4_MUTE_TILL_LOCK_EN,
> -	.gpio_lock_detect = -1,
>  };
>  
>  static int adf4350_sync_config(struct adf4350_state *st)
> @@ -317,8 +316,8 @@ static ssize_t adf4350_read(struct iio_dev
> *indio_dev,
>  			(u64)st->fpfd;
>  		do_div(val, st->r1_mod * (1 << st->r4_rf_div_sel));
>  		/* PLL unlocked? return error */
> -		if (gpio_is_valid(st->pdata->gpio_lock_detect))
> -			if (!gpio_get_value(st->pdata->gpio_lock_detect)) {
> +		if (st->lock_detect_gpiod)
> +			if (!gpiod_get_value(st->lock_detect_gpiod)) {
>  				dev_dbg(&st->spi->dev, "PLL un-locked\n");
>  				ret = -EBUSY;
>  			}
> @@ -381,7 +380,6 @@ static struct adf4350_platform_data
> *adf4350_parse_dt(struct device *dev)
>  	struct device_node *np = dev->of_node;
>  	struct adf4350_platform_data *pdata;
>  	unsigned int tmp;
> -	int ret;
>  
>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
> @@ -401,12 +399,6 @@ static struct adf4350_platform_data
> *adf4350_parse_dt(struct device *dev)
>  	of_property_read_u32(np, "adi,reference-div-factor", &tmp);
>  	pdata->ref_div_factor = tmp;
>  
> -	ret = of_get_gpio(np, 0);
> -	if (ret < 0)
> -		pdata->gpio_lock_detect = -1;
> -	else
> -		pdata->gpio_lock_detect = ret;
> -
>  	pdata->ref_doubler_en = of_property_read_bool(np,
>  			"adi,reference-doubler-enable");
>  	pdata->ref_div2_en = of_property_read_bool(np,
> @@ -561,16 +553,10 @@ static int adf4350_probe(struct spi_device *spi)
>  
>  	memset(st->regs_hw, 0xFF, sizeof(st->regs_hw));
>  
> -	if (gpio_is_valid(pdata->gpio_lock_detect)) {
> -		ret = devm_gpio_request(&spi->dev, pdata->gpio_lock_detect,
> -					indio_dev->name);
> -		if (ret) {
> -			dev_err(&spi->dev, "fail to request lock detect
> GPIO-%d",
> -				pdata->gpio_lock_detect);
> -			goto error_disable_reg;
> -		}
> -		gpio_direction_input(pdata->gpio_lock_detect);
> -	}
> +	st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev, NULL,

Would it make sense to name the GPIO here?
Maybe name it "lock-detect"?

I do realize that this goes into the realm of changing some default
behavior.
And I am not sure how acceptable this is [generally].

Thanks
Alex

> +							GPIOD_IN);
> +	if (IS_ERR(st->lock_detect_gpiod))
> +		return PTR_ERR(st->lock_detect_gpiod);
>  
>  	if (pdata->power_up_frequency) {
>  		ret = adf4350_set_freq(st, pdata->power_up_frequency);
> diff --git a/include/linux/iio/frequency/adf4350.h
> b/include/linux/iio/frequency/adf4350.h
> index ce9490bfeb89..de45cf2ee1e4 100644
> --- a/include/linux/iio/frequency/adf4350.h
> +++ b/include/linux/iio/frequency/adf4350.h
> @@ -103,9 +103,6 @@
>   * @r2_user_settings:	User defined settings for ADF4350/1
> REGISTER_2.
>   * @r3_user_settings:	User defined settings for ADF4350/1
> REGISTER_3.
>   * @r4_user_settings:	User defined settings for ADF4350/1
> REGISTER_4.
> - * @gpio_lock_detect:	Optional, if set with a valid GPIO number,
> - *			pll lock state is tested upon read.
> - *			If not used - set to -1.
>   */
>  
>  struct adf4350_platform_data {
> @@ -121,7 +118,6 @@ struct adf4350_platform_data {
>  	unsigned		r2_user_settings;
>  	unsigned		r3_user_settings;
>  	unsigned		r4_user_settings;
> -	int			gpio_lock_detect;
>  };
>  
>  #endif /* IIO_PLL_ADF4350_H_ */
Linus Walleij Dec. 2, 2019, 9:16 a.m. UTC | #2
On Mon, Dec 2, 2019 at 9:50 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:

> > +     st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev, NULL,
>
> Would it make sense to name the GPIO here?
> Maybe name it "lock-detect"?
>
> I do realize that this goes into the realm of changing some default
> behavior.
> And I am not sure how acceptable this is [generally].

You can't name it in the devm_gpiod_get_optional() call as this
indicates the name the GPIO lines have in the device tree.

What you can do is add a call to
gpiod_set_consumer_name(gpiod, "name");
to explicitly name the line.

This will only affect the name assigned to the line
in debugfs and in the userspace tools like "lsgpio".

Yours,
Linus Walleij
Ardelean, Alexandru Dec. 2, 2019, 9:28 a.m. UTC | #3
On Mon, 2019-12-02 at 10:16 +0100, Linus Walleij wrote:
> On Mon, Dec 2, 2019 at 9:50 AM Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> wrote:
> 
> > > +     st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev,
> > > NULL,
> > 
> > Would it make sense to name the GPIO here?
> > Maybe name it "lock-detect"?
> > 
> > I do realize that this goes into the realm of changing some default
> > behavior.
> > And I am not sure how acceptable this is [generally].
> 
> You can't name it in the devm_gpiod_get_optional() call as this
> indicates the name the GPIO lines have in the device tree.
> 
> What you can do is add a call to
> gpiod_set_consumer_name(gpiod, "name");
> to explicitly name the line.
> 
> This will only affect the name assigned to the line
> in debugfs and in the userspace tools like "lsgpio".
> 

Ack.
Thanks for the info.

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>


> Yours,
> Linus Walleij
Jonathan Cameron Dec. 7, 2019, 9:45 a.m. UTC | #4
On Mon, 2 Dec 2019 09:28:32 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2019-12-02 at 10:16 +0100, Linus Walleij wrote:
> > On Mon, Dec 2, 2019 at 9:50 AM Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> wrote:
> >   
> > > > +     st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev,
> > > > NULL,  
> > > 
> > > Would it make sense to name the GPIO here?
> > > Maybe name it "lock-detect"?
> > > 
> > > I do realize that this goes into the realm of changing some default
> > > behavior.
> > > And I am not sure how acceptable this is [generally].  
> > 
> > You can't name it in the devm_gpiod_get_optional() call as this
> > indicates the name the GPIO lines have in the device tree.
> > 
> > What you can do is add a call to
> > gpiod_set_consumer_name(gpiod, "name");
> > to explicitly name the line.
> > 
> > This will only affect the name assigned to the line
> > in debugfs and in the userspace tools like "lsgpio".
> >   
> 
> Ack.
> Thanks for the info.
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied,

Thanks,

J
> 
> 
> > Yours,
> > Linus Walleij

Patch
diff mbox series

diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index ae0ca09ae062..1c2dc9b00f31 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -14,11 +14,10 @@ 
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/gcd.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <asm/div64.h>
 #include <linux/clk.h>
 #include <linux/of.h>
-#include <linux/of_gpio.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -34,6 +33,7 @@  enum {
 struct adf4350_state {
 	struct spi_device		*spi;
 	struct regulator		*reg;
+	struct gpio_desc		*lock_detect_gpiod;
 	struct adf4350_platform_data	*pdata;
 	struct clk			*clk;
 	unsigned long			clkin;
@@ -61,7 +61,6 @@  static struct adf4350_platform_data default_pdata = {
 	.r3_user_settings = ADF4350_REG3_12BIT_CLKDIV_MODE(0),
 	.r4_user_settings = ADF4350_REG4_OUTPUT_PWR(3) |
 			    ADF4350_REG4_MUTE_TILL_LOCK_EN,
-	.gpio_lock_detect = -1,
 };
 
 static int adf4350_sync_config(struct adf4350_state *st)
@@ -317,8 +316,8 @@  static ssize_t adf4350_read(struct iio_dev *indio_dev,
 			(u64)st->fpfd;
 		do_div(val, st->r1_mod * (1 << st->r4_rf_div_sel));
 		/* PLL unlocked? return error */
-		if (gpio_is_valid(st->pdata->gpio_lock_detect))
-			if (!gpio_get_value(st->pdata->gpio_lock_detect)) {
+		if (st->lock_detect_gpiod)
+			if (!gpiod_get_value(st->lock_detect_gpiod)) {
 				dev_dbg(&st->spi->dev, "PLL un-locked\n");
 				ret = -EBUSY;
 			}
@@ -381,7 +380,6 @@  static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
 	struct device_node *np = dev->of_node;
 	struct adf4350_platform_data *pdata;
 	unsigned int tmp;
-	int ret;
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
@@ -401,12 +399,6 @@  static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
 	of_property_read_u32(np, "adi,reference-div-factor", &tmp);
 	pdata->ref_div_factor = tmp;
 
-	ret = of_get_gpio(np, 0);
-	if (ret < 0)
-		pdata->gpio_lock_detect = -1;
-	else
-		pdata->gpio_lock_detect = ret;
-
 	pdata->ref_doubler_en = of_property_read_bool(np,
 			"adi,reference-doubler-enable");
 	pdata->ref_div2_en = of_property_read_bool(np,
@@ -561,16 +553,10 @@  static int adf4350_probe(struct spi_device *spi)
 
 	memset(st->regs_hw, 0xFF, sizeof(st->regs_hw));
 
-	if (gpio_is_valid(pdata->gpio_lock_detect)) {
-		ret = devm_gpio_request(&spi->dev, pdata->gpio_lock_detect,
-					indio_dev->name);
-		if (ret) {
-			dev_err(&spi->dev, "fail to request lock detect GPIO-%d",
-				pdata->gpio_lock_detect);
-			goto error_disable_reg;
-		}
-		gpio_direction_input(pdata->gpio_lock_detect);
-	}
+	st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev, NULL,
+							GPIOD_IN);
+	if (IS_ERR(st->lock_detect_gpiod))
+		return PTR_ERR(st->lock_detect_gpiod);
 
 	if (pdata->power_up_frequency) {
 		ret = adf4350_set_freq(st, pdata->power_up_frequency);
diff --git a/include/linux/iio/frequency/adf4350.h b/include/linux/iio/frequency/adf4350.h
index ce9490bfeb89..de45cf2ee1e4 100644
--- a/include/linux/iio/frequency/adf4350.h
+++ b/include/linux/iio/frequency/adf4350.h
@@ -103,9 +103,6 @@ 
  * @r2_user_settings:	User defined settings for ADF4350/1 REGISTER_2.
  * @r3_user_settings:	User defined settings for ADF4350/1 REGISTER_3.
  * @r4_user_settings:	User defined settings for ADF4350/1 REGISTER_4.
- * @gpio_lock_detect:	Optional, if set with a valid GPIO number,
- *			pll lock state is tested upon read.
- *			If not used - set to -1.
  */
 
 struct adf4350_platform_data {
@@ -121,7 +118,6 @@  struct adf4350_platform_data {
 	unsigned		r2_user_settings;
 	unsigned		r3_user_settings;
 	unsigned		r4_user_settings;
-	int			gpio_lock_detect;
 };
 
 #endif /* IIO_PLL_ADF4350_H_ */