On Thu, Feb 20, 2020 at 10:00:32AM +0100, Linus Walleij wrote:
> The code in the Corgi backlight driver can be considerably
> simplified by moving to GPIO descriptors and lookup tables
> from the board files instead of passing GPIO numbers using
> the old API.
>
> Make sure to encode inversion semantics for the Akita and
> Spitz platforms inside the GPIO lookup table and drop the
> custom inversion semantics from the driver.
>
> All in-tree users are converted in this patch.
>
> Cc: Andrea Adami <andrea.adami@xxxxxxxxx>
> Acked-by: Robert Jarzmik <robert.jarzmik@xxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
So... I'm still not 100% sure about the bus names. The code is
confusing and made more so by what I think is probably a misleading
comment! So test reports welcome but nevertheless this is...
Reviewed-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
Daniel.
> ---
> ChangeLog v2->v3:
> - Switch the SPI bus name to "spi1.1" rather than "spi0.1" for
> Corgi and "spi2.1" rather than "spi0.1" for Spitz, as
> pxa2xx_set_spi_info() sets the bus number to 1 and 2
> respectively.
> ChangeLog v1->v2:
> - Collect Robert's ACK.
> ---
> arch/arm/mach-pxa/corgi.c | 12 ++++-
> arch/arm/mach-pxa/spitz.c | 34 +++++++++++----
> drivers/video/backlight/corgi_lcd.c | 68 ++++++++---------------------
> include/linux/spi/corgi_lcd.h | 3 --
> 4 files changed, 54 insertions(+), 63 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
> index f2d73289230f..593c7f793da5 100644
> --- a/arch/arm/mach-pxa/corgi.c
> +++ b/arch/arm/mach-pxa/corgi.c
> @@ -563,13 +563,20 @@ static void corgi_bl_kick_battery(void)
> }
> }
>
> +static struct gpiod_lookup_table corgi_lcdcon_gpio_table = {
> + .dev_id = "spi1.1",
> + .table = {
> + GPIO_LOOKUP("gpio-pxa", CORGI_GPIO_BACKLIGHT_CONT,
> + "BL_CONT", GPIO_ACTIVE_HIGH),
> + { },
> + },
> +};
> +
> static struct corgi_lcd_platform_data corgi_lcdcon_info = {
> .init_mode = CORGI_LCD_MODE_VGA,
> .max_intensity = 0x2f,
> .default_intensity = 0x1f,
> .limit_mask = 0x0b,
> - .gpio_backlight_cont = CORGI_GPIO_BACKLIGHT_CONT,
> - .gpio_backlight_on = -1,
> .kick_battery = corgi_bl_kick_battery,
> };
>
> @@ -609,6 +616,7 @@ static struct spi_board_info corgi_spi_devices[] = {
> static void __init corgi_init_spi(void)
> {
> pxa2xx_set_spi_info(1, &corgi_spi_info);
> + gpiod_add_lookup_table(&corgi_lcdcon_gpio_table);
> spi_register_board_info(ARRAY_AND_SIZE(corgi_spi_devices));
> }
> #else
> diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
> index a4fdc399d152..371008e9bb02 100644
> --- a/arch/arm/mach-pxa/spitz.c
> +++ b/arch/arm/mach-pxa/spitz.c
> @@ -525,13 +525,33 @@ static void spitz_bl_kick_battery(void)
> }
> }
>
> +static struct gpiod_lookup_table spitz_lcdcon_gpio_table = {
> + .dev_id = "spi2.1",
> + .table = {
> + GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_BACKLIGHT_CONT,
> + "BL_CONT", GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_BACKLIGHT_ON,
> + "BL_ON", GPIO_ACTIVE_HIGH),
> + { },
> + },
> +};
> +
> +static struct gpiod_lookup_table akita_lcdcon_gpio_table = {
> + .dev_id = "spi2.1",
> + .table = {
> + GPIO_LOOKUP("gpio-pxa", AKITA_GPIO_BACKLIGHT_CONT,
> + "BL_CONT", GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP("gpio-pxa", AKITA_GPIO_BACKLIGHT_ON,
> + "BL_ON", GPIO_ACTIVE_HIGH),
> + { },
> + },
> +};
> +
> static struct corgi_lcd_platform_data spitz_lcdcon_info = {
> .init_mode = CORGI_LCD_MODE_VGA,
> .max_intensity = 0x2f,
> .default_intensity = 0x1f,
> .limit_mask = 0x0b,
> - .gpio_backlight_cont = SPITZ_GPIO_BACKLIGHT_CONT,
> - .gpio_backlight_on = SPITZ_GPIO_BACKLIGHT_ON,
> .kick_battery = spitz_bl_kick_battery,
> };
>
> @@ -574,12 +594,10 @@ static struct pxa2xx_spi_controller spitz_spi_info = {
>
> static void __init spitz_spi_init(void)
> {
> - struct corgi_lcd_platform_data *lcd_data = &spitz_lcdcon_info;
> -
> - if (machine_is_akita()) {
> - lcd_data->gpio_backlight_cont = AKITA_GPIO_BACKLIGHT_CONT;
> - lcd_data->gpio_backlight_on = AKITA_GPIO_BACKLIGHT_ON;
> - }
> + if (machine_is_akita())
> + gpiod_add_lookup_table(&akita_lcdcon_gpio_table);
> + else
> + gpiod_add_lookup_table(&spitz_lcdcon_gpio_table);
>
> pxa2xx_set_spi_info(2, &spitz_spi_info);
> spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices));
> diff --git a/drivers/video/backlight/corgi_lcd.c b/drivers/video/backlight/corgi_lcd.c
> index 68f7592c5060..25ef0cbd7583 100644
> --- a/drivers/video/backlight/corgi_lcd.c
> +++ b/drivers/video/backlight/corgi_lcd.c
> @@ -15,7 +15,7 @@
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/fb.h>
> #include <linux/lcd.h>
> #include <linux/spi/spi.h>
> @@ -90,9 +90,8 @@ struct corgi_lcd {
> int mode;
> char buf[2];
>
> - int gpio_backlight_on;
> - int gpio_backlight_cont;
> - int gpio_backlight_cont_inverted;
> + struct gpio_desc *backlight_on;
> + struct gpio_desc *backlight_cont;
>
> void (*kick_battery)(void);
> };
> @@ -403,13 +402,13 @@ static int corgi_bl_set_intensity(struct corgi_lcd *lcd, int intensity)
> corgi_ssp_lcdtg_send(lcd, DUTYCTRL_ADRS, intensity);
>
> /* Bit 5 via GPIO_BACKLIGHT_CONT */
> - cont = !!(intensity & 0x20) ^ lcd->gpio_backlight_cont_inverted;
> + cont = !!(intensity & 0x20);
>
> - if (gpio_is_valid(lcd->gpio_backlight_cont))
> - gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> + if (lcd->backlight_cont)
> + gpiod_set_value_cansleep(lcd->backlight_cont, cont);
>
> - if (gpio_is_valid(lcd->gpio_backlight_on))
> - gpio_set_value_cansleep(lcd->gpio_backlight_on, intensity);
> + if (lcd->backlight_on)
> + gpiod_set_value_cansleep(lcd->backlight_on, intensity);
>
> if (lcd->kick_battery)
> lcd->kick_battery();
> @@ -482,48 +481,17 @@ static int setup_gpio_backlight(struct corgi_lcd *lcd,
> struct corgi_lcd_platform_data *pdata)
> {
> struct spi_device *spi = lcd->spi_dev;
> - int err;
> -
> - lcd->gpio_backlight_on = -1;
> - lcd->gpio_backlight_cont = -1;
> -
> - if (gpio_is_valid(pdata->gpio_backlight_on)) {
> - err = devm_gpio_request(&spi->dev, pdata->gpio_backlight_on,
> - "BL_ON");
> - if (err) {
> - dev_err(&spi->dev,
> - "failed to request GPIO%d for backlight_on\n",
> - pdata->gpio_backlight_on);
> - return err;
> - }
> -
> - lcd->gpio_backlight_on = pdata->gpio_backlight_on;
> - gpio_direction_output(lcd->gpio_backlight_on, 0);
> - }
>
> - if (gpio_is_valid(pdata->gpio_backlight_cont)) {
> - err = devm_gpio_request(&spi->dev, pdata->gpio_backlight_cont,
> - "BL_CONT");
> - if (err) {
> - dev_err(&spi->dev,
> - "failed to request GPIO%d for backlight_cont\n",
> - pdata->gpio_backlight_cont);
> - return err;
> - }
> -
> - lcd->gpio_backlight_cont = pdata->gpio_backlight_cont;
> -
> - /* spitz and akita use both GPIOs for backlight, and
> - * have inverted polarity of GPIO_BACKLIGHT_CONT
> - */
> - if (gpio_is_valid(lcd->gpio_backlight_on)) {
> - lcd->gpio_backlight_cont_inverted = 1;
> - gpio_direction_output(lcd->gpio_backlight_cont, 1);
> - } else {
> - lcd->gpio_backlight_cont_inverted = 0;
> - gpio_direction_output(lcd->gpio_backlight_cont, 0);
> - }
> - }
> + lcd->backlight_on = devm_gpiod_get_optional(&spi->dev,
> + "BL_ON", GPIOD_OUT_LOW);
> + if (IS_ERR(lcd->backlight_on))
> + return PTR_ERR(lcd->backlight_on);
> +
> + lcd->backlight_cont = devm_gpiod_get_optional(&spi->dev, "BL_CONT",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(lcd->backlight_cont))
> + return PTR_ERR(lcd->backlight_cont);
> +
> return 0;
> }
>
> diff --git a/include/linux/spi/corgi_lcd.h b/include/linux/spi/corgi_lcd.h
> index edf4beccdadb..0b857616919c 100644
> --- a/include/linux/spi/corgi_lcd.h
> +++ b/include/linux/spi/corgi_lcd.h
> @@ -11,9 +11,6 @@ struct corgi_lcd_platform_data {
> int default_intensity;
> int limit_mask;
>
> - int gpio_backlight_on; /* -1 if n/a */
> - int gpio_backlight_cont; /* -1 if n/a */
> -
> void (*notify)(int intensity);
> void (*kick_battery)(void);
> };
> --
> 2.24.1
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
