leds: bd2802: Convert to use GPIO descriptors

Message ID 20191203095908.52926-1-linus.walleij@linaro.org
State Superseded
Headers show
Series
  • leds: bd2802: Convert to use GPIO descriptors
Related show

Commit Message

Linus Walleij Dec. 3, 2019, 9:59 a.m.
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>

---
 drivers/leds/leds-bd2802.c  | 27 ++++++++++++++++++---------
 include/linux/leds-bd2802.h |  1 -
 2 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.23.0

Comments

Dan Murphy Dec. 3, 2019, 12:41 p.m. | #1
Linus

On 12/3/19 3:59 AM, 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>

> ---

>   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..d551f7434897 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, "rgb-resetb", GPIOD_OUT_LOW);


We usually use "reset" for the property of the reset-gpios.

Unless there are other reset lines for this chip or you are keeping the 
name for compatibiilty

Dan
Pavel Machek Dec. 21, 2019, 6:52 p.m. | #2
On Tue 2019-12-03 10:59:08, 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>


Umm. What should I do here? Apply to next and see if it breaks
something?

Was this tested somehow?

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

Patch

diff --git a/drivers/leds/leds-bd2802.c b/drivers/leds/leds-bd2802.c
index e7ec6bff2b5f..d551f7434897 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, "rgb-resetb", 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;
 };