Re: [RFC 2/4] sh: ecovec24: conditionally register backlight device

[jacopo mondi]

Hello Dmitry

FYI I am brushing the ecovec board these days as well
https://www.spinics.net/lists/linux-sh/msg52536.html

And I have a board to test with but without any display panel, I'm
afraid.

On Thu, Mar 15, 2018 at 03:42:00PM -0700, Dmitry Torokhov wrote:
> Commit fe79f919f47e ("sh: ecovec24: Use gpio-backlight") removed custom
> backlight support and switched over to generic gpio-backlight driver. The
> comment when we run with DVI states "no backlight", but setting
> gpio_backlight_data.fbdev to NULL actually makes gpio-backlight to react to
> events from any framebuffer device, not ignore them.
>
> We want to get rid of platform data in favor of generic device properties
> in gpio_backlight driver, so we can not have kernel pointers passed around
> to tie the framebuffer device to backlight. Assuming that the intent of the
> above referenced commit was to indeed not export backlight when using DVI,
> let's switch to conditionally registering backlight device so it is not
> present at all in DVI case.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>  arch/sh/boards/mach-ecovec24/setup.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
> index 6f929abe0b50f..67633d2d42390 100644
> --- a/arch/sh/boards/mach-ecovec24/setup.c
> +++ b/arch/sh/boards/mach-ecovec24/setup.c
> @@ -368,7 +368,6 @@ static struct platform_device lcdc_device = {
>  };
>
>  static struct gpio_backlight_platform_data gpio_backlight_data = {
> -	.fbdev = &lcdc_device.dev,
>  	.gpio = GPIO_PTR1,
>  	.def_value = 1,
>  	.name = "backlight",
> @@ -987,7 +986,6 @@ static struct platform_device *ecovec_devices[] __initdata = {
>  	&usb1_common_device,
>  	&usbhs_device,
>  	&lcdc_device,
> -	&gpio_backlight_device,
>  	&ceu0_device,
>  	&ceu1_device,
>  	&keysc_device,
> @@ -1077,6 +1075,8 @@ static int __init arch_setup(void)
>  {
>  	struct clk *clk;
>  	bool cn12_enabled = false;
> +	bool use_backlight = false;
> +	int error;
>
>  	/* register board specific self-refresh code */
>  	sh_mobile_register_self_refresh(SUSP_SH_STANDBY | SUSP_SH_SF |
> @@ -1193,9 +1193,6 @@ static int __init arch_setup(void)
>  		lcdc_info.ch[0].lcd_modes		= ecovec_dvi_modes;
>  		lcdc_info.ch[0].num_modes		= ARRAY_SIZE(ecovec_dvi_modes);
>
> -		/* No backlight */
> -		gpio_backlight_data.fbdev = NULL;
> -
>  		gpio_set_value(GPIO_PTA2, 1);
>  		gpio_set_value(GPIO_PTU1, 1);
>  	} else {
> @@ -1217,6 +1214,8 @@ static int __init arch_setup(void)
>  		/* enable TouchScreen */
>  		i2c_register_board_info(0, &ts_i2c_clients, 1);
>  		irq_set_irq_type(IRQ0, IRQ_TYPE_LEVEL_LOW);
> +
> +		use_backlight = true;
>  	}
>
>  	/* enable CEU0 */
> @@ -1431,8 +1430,19 @@ static int __init arch_setup(void)
>  	gpio_set_value(GPIO_PTG4, 1);
>  #endif
>
> -	return platform_add_devices(ecovec_devices,
> -				    ARRAY_SIZE(ecovec_devices));
> +	error = platform_add_devices(ecovec_devices,
> +				      ARRAY_SIZE(ecovec_devices));

I would invert this.
Register the backlight first, then all other devices.


> +	if (error)
> +		return error;
> +
> +	if (use_backlight) {
> +		error = platform_device_add(&gpio_backlight_device);
> +		if (error)
> +			pr_warn("%s: failed to register backlight: %d\n",
> +				error);

Could you use dev_warn here? Also the format is wrong, I assume you
are missing a '__func__' as second function argument.

Also, you may want to return error.

Thanks
   j

> +	}
> +
> +	return 0;
>  }
>  arch_initcall(arch_setup);
>
> --
> 2.16.2.804.g6dcf76e118-goog
>

Attachment: signature.asc
Description: PGP signature