Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] leds: bd2802: Convert to use GPIO descriptors
@ 2019-12-09  0:42 Linus Walleij
  2019-12-09 14:05 ` Dan Murphy
  2019-12-21 19:03 ` Pavel Machek
  0 siblings, 2 replies; 4+ messages in thread
From: Linus Walleij @ 2019-12-09  0:42 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek, Dan Murphy
  Cc: linux-leds, Linus Walleij, Kim Kyuwon

The Rohm BD2802 have no in-kernel users so we can drop the
GPIO number from the platform data and require users to
provide the GPIO line using machine descriptors.

As the descriptors come with inherent polarity inversion
semantics, we invert the calls to set the GPIO line such
that 0 means "unasserted" and 1 means "asserted".

Put a note in the driver that machine descriptor tables
will need to specify that the line is active low.

Cc: Kim Kyuwon <chammoru@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Ask simply for a GPIO descriptor named "reset", drop the
  idiomatic naming from the code.
---
 drivers/leds/leds-bd2802.c  | 27 ++++++++++++++++++---------
 include/linux/leds-bd2802.h |  1 -
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
index e7ec6bff2b5f..bd61a823d0ca 100644
--- a/drivers/leds/leds-bd2802.c
+++ b/drivers/leds/leds-bd2802.c
@@ -10,7 +10,7 @@
 
 #include <linux/module.h>
 #include <linux/i2c.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/leds.h>
 #include <linux/leds-bd2802.h>
@@ -67,6 +67,7 @@ struct led_state {
 struct bd2802_led {
 	struct bd2802_led_platform_data	*pdata;
 	struct i2c_client		*client;
+	struct gpio_desc		*reset;
 	struct rw_semaphore		rwsem;
 
 	struct led_state		led[2];
@@ -200,7 +201,7 @@ static void bd2802_update_state(struct bd2802_led *led, enum led_ids id,
 		return;
 
 	if (bd2802_is_all_off(led) && !led->adf_on) {
-		gpio_set_value(led->pdata->reset_gpio, 0);
+		gpiod_set_value(led->reset, 1);
 		return;
 	}
 
@@ -226,7 +227,7 @@ static void bd2802_configure(struct bd2802_led *led)
 
 static void bd2802_reset_cancel(struct bd2802_led *led)
 {
-	gpio_set_value(led->pdata->reset_gpio, 1);
+	gpiod_set_value(led->reset, 0);
 	udelay(100);
 	bd2802_configure(led);
 }
@@ -420,7 +421,7 @@ static void bd2802_disable_adv_conf(struct bd2802_led *led)
 						bd2802_addr_attributes[i]);
 
 	if (bd2802_is_all_off(led))
-		gpio_set_value(led->pdata->reset_gpio, 0);
+		gpiod_set_value(led->reset, 1);
 
 	led->adf_on = 0;
 }
@@ -670,8 +671,16 @@ static int bd2802_probe(struct i2c_client *client,
 	pdata = led->pdata = dev_get_platdata(&client->dev);
 	i2c_set_clientdata(client, led);
 
-	/* Configure RESET GPIO (L: RESET, H: RESET cancel) */
-	gpio_request_one(pdata->reset_gpio, GPIOF_OUT_INIT_HIGH, "RGB_RESETB");
+	/*
+	 * Configure RESET GPIO (L: RESET, H: RESET cancel)
+	 *
+	 * We request the reset GPIO as OUT_LOW which means de-asserted,
+	 * board files specifying this GPIO line in a machine descriptor
+	 * table should take care to specify GPIO_ACTIVE_LOW for this line.
+	 */
+	led->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(led->reset))
+		return PTR_ERR(led->reset);
 
 	/* Tacss = min 0.1ms */
 	udelay(100);
@@ -685,7 +694,7 @@ static int bd2802_probe(struct i2c_client *client,
 		dev_info(&client->dev, "return 0x%02x\n", ret);
 
 	/* To save the power, reset BD2802 after detecting */
-	gpio_set_value(led->pdata->reset_gpio, 0);
+	gpiod_set_value(led->reset, 1);
 
 	/* Default attributes */
 	led->wave_pattern = BD2802_PATTERN_HALF;
@@ -720,7 +729,7 @@ static int bd2802_remove(struct i2c_client *client)
 	struct bd2802_led *led = i2c_get_clientdata(client);
 	int i;
 
-	gpio_set_value(led->pdata->reset_gpio, 0);
+	gpiod_set_value(led->reset, 1);
 	bd2802_unregister_led_classdev(led);
 	if (led->adf_on)
 		bd2802_disable_adv_conf(led);
@@ -750,7 +759,7 @@ static int bd2802_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bd2802_led *led = i2c_get_clientdata(client);
 
-	gpio_set_value(led->pdata->reset_gpio, 0);
+	gpiod_set_value(led->reset, 1);
 
 	return 0;
 }
diff --git a/include/linux/leds-bd2802.h b/include/linux/leds-bd2802.h
index dd93c8d787b4..ec577f5f8707 100644
--- a/include/linux/leds-bd2802.h
+++ b/include/linux/leds-bd2802.h
@@ -11,7 +11,6 @@
 #define _LEDS_BD2802_H_
 
 struct bd2802_led_platform_data{
-	int	reset_gpio;
 	u8	rgb_time;
 };
 
-- 
2.23.0


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

* Re: [PATCH v2] leds: bd2802: Convert to use GPIO descriptors
  2019-12-09  0:42 [PATCH v2] leds: bd2802: Convert to use GPIO descriptors Linus Walleij
@ 2019-12-09 14:05 ` Dan Murphy
  2019-12-11  0:52   ` Linus Walleij
  2019-12-21 19:03 ` Pavel Machek
  1 sibling, 1 reply; 4+ messages in thread
From: Dan Murphy @ 2019-12-09 14:05 UTC (permalink / raw)
  To: Linus Walleij, Jacek Anaszewski, Pavel Machek; +Cc: linux-leds, Kim Kyuwon

Linus

On 12/8/19 6:42 PM, Linus Walleij wrote:
> The Rohm BD2802 have no in-kernel users so we can drop the
> GPIO number from the platform data and require users to
> provide the GPIO line using machine descriptors.
>
> As the descriptors come with inherent polarity inversion
> semantics, we invert the calls to set the GPIO line such
> that 0 means "unasserted" and 1 means "asserted".
>
> Put a note in the driver that machine descriptor tables
> will need to specify that the line is active low.
>
> Cc: Kim Kyuwon <chammoru@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Ask simply for a GPIO descriptor named "reset", drop the
>    idiomatic naming from the code.
> ---
>   drivers/leds/leds-bd2802.c  | 27 ++++++++++++++++++---------
>   include/linux/leds-bd2802.h |  1 -
>   2 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
> index e7ec6bff2b5f..bd61a823d0ca 100644
> --- a/drivers/leds/leds-bd2802.c
> +++ b/drivers/leds/leds-bd2802.c
> @@ -10,7 +10,7 @@
>   
>   #include <linux/module.h>
>   #include <linux/i2c.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/delay.h>
>   #include <linux/leds.h>
>   #include <linux/leds-bd2802.h>
> @@ -67,6 +67,7 @@ struct led_state {
>   struct bd2802_led {
>   	struct bd2802_led_platform_data	*pdata;
>   	struct i2c_client		*client;
> +	struct gpio_desc		*reset;
>   	struct rw_semaphore		rwsem;
>   
>   	struct led_state		led[2];
> @@ -200,7 +201,7 @@ static void bd2802_update_state(struct bd2802_led *led, enum led_ids id,
>   		return;
>   
>   	if (bd2802_is_all_off(led) && !led->adf_on) {
> -		gpio_set_value(led->pdata->reset_gpio, 0);
> +		gpiod_set_value(led->reset, 1);
>   		return;
>   	}
>   
> @@ -226,7 +227,7 @@ static void bd2802_configure(struct bd2802_led *led)
>   
>   static void bd2802_reset_cancel(struct bd2802_led *led)
>   {
> -	gpio_set_value(led->pdata->reset_gpio, 1);
> +	gpiod_set_value(led->reset, 0);
>   	udelay(100);
>   	bd2802_configure(led);
>   }
> @@ -420,7 +421,7 @@ static void bd2802_disable_adv_conf(struct bd2802_led *led)
>   						bd2802_addr_attributes[i]);
>   
>   	if (bd2802_is_all_off(led))
> -		gpio_set_value(led->pdata->reset_gpio, 0);
> +		gpiod_set_value(led->reset, 1);
>   
>   	led->adf_on = 0;
>   }
> @@ -670,8 +671,16 @@ static int bd2802_probe(struct i2c_client *client,
>   	pdata = led->pdata = dev_get_platdata(&client->dev);
>   	i2c_set_clientdata(client, led);
>   
> -	/* Configure RESET GPIO (L: RESET, H: RESET cancel) */
> -	gpio_request_one(pdata->reset_gpio, GPIOF_OUT_INIT_HIGH, "RGB_RESETB");
> +	/*
> +	 * Configure RESET GPIO (L: RESET, H: RESET cancel)
> +	 *
> +	 * We request the reset GPIO as OUT_LOW which means de-asserted,
> +	 * board files specifying this GPIO line in a machine descriptor
> +	 * table should take care to specify GPIO_ACTIVE_LOW for this line.
> +	 */
> +	led->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(led->reset))
> +		return PTR_ERR(led->reset);

What about EPROBE_DEFER?

Is the reset line optional or required?  I see that this device does not 
have DT docs and the call infers it is required.

But can this be optional?

Dan


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

* Re: [PATCH v2] leds: bd2802: Convert to use GPIO descriptors
  2019-12-09 14:05 ` Dan Murphy
@ 2019-12-11  0:52   ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2019-12-11  0:52 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Jacek Anaszewski, Pavel Machek, Linux LED Subsystem, Kim Kyuwon

On Mon, Dec 9, 2019 at 3:07 PM Dan Murphy <dmurphy@ti.com> wrote:
> On 12/8/19 6:42 PM, Linus Walleij wrote:

> > +     led->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(led->reset))
> > +             return PTR_ERR(led->reset);
>
> What about EPROBE_DEFER?

That's fine? I don't understand this comment, the driver will
bail out and be retried if deferral happens.

> Is the reset line optional or required?  I see that this device does not
> have DT docs and the call infers it is required.

This driver does not use DT. It needs to be used with board files.

> But can this be optional?

Since there are no users in the kernel it doesn't matter, if it needs
to be optional then whoever is going to use it can make it optional
I suppose. I can't really half-guess use cases when noone is
stepping up and providing one.

Yours,
Linus Walleij

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

* Re: [PATCH v2] leds: bd2802: Convert to use GPIO descriptors
  2019-12-09  0:42 [PATCH v2] leds: bd2802: Convert to use GPIO descriptors Linus Walleij
  2019-12-09 14:05 ` Dan Murphy
@ 2019-12-21 19:03 ` Pavel Machek
  1 sibling, 0 replies; 4+ messages in thread
From: Pavel Machek @ 2019-12-21 19:03 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Jacek Anaszewski, Dan Murphy, linux-leds, Kim Kyuwon

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Mon 2019-12-09 01:42:42, Linus Walleij wrote:
> The Rohm BD2802 have no in-kernel users so we can drop the
> GPIO number from the platform data and require users to
> provide the GPIO line using machine descriptors.
> 
> As the descriptors come with inherent polarity inversion
> semantics, we invert the calls to set the GPIO line such
> that 0 means "unasserted" and 1 means "asserted".
> 
> Put a note in the driver that machine descriptor tables
> will need to specify that the line is active low.

Thanks, applied.
						Pavel
						
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09  0:42 [PATCH v2] leds: bd2802: Convert to use GPIO descriptors Linus Walleij
2019-12-09 14:05 ` Dan Murphy
2019-12-11  0:52   ` Linus Walleij
2019-12-21 19:03 ` Pavel Machek

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/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-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

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


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