From mboxrd@z Thu Jan 1 00:00:00 1970 From: jacopo mondi Date: Sat, 17 Mar 2018 09:25:16 +0000 Subject: Re: [RFC 2/4] sh: ecovec24: conditionally register backlight device Message-Id: <20180317092516.g5p5ybka57abyhex@apu3b> List-Id: References: <20180315224202.96668-1-dmitry.torokhov@gmail.com> <20180315224202.96668-3-dmitry.torokhov@gmail.com> <20180316100748.GH16424@w540> <20180316233800.GA32310@dtor-ws> In-Reply-To: <20180316233800.GA32310@dtor-ws> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dmitry Torokhov Cc: Laurent Pinchart , Lee Jones , Daniel Thompson , Jingoo Han , linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, Yoshinori Sato , Rich Felker , Linus Walleij Hi Dmitry, On Fri, Mar 16, 2018 at 04:38:00PM -0700, Dmitry Torokhov wrote: > Hi Jacopo, > > On Fri, Mar 16, 2018 at 11:07:48AM +0100, jacopo mondi wrote: > > Hello Dmitry > > > > FYI I am brushing the ecovec board these days as well > > https://www.spinics.net/lists/linux-sh/msg52536.html > > > > What is the ecovec board BTW? Is it some devkit or what? It seems quite > old to me. Yes, it is a SuperH 4 based development board. It is old for sure. I'm also working on removing some stuff the ecovec board file is the only user of... > > 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 > > > --- > > > 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. > > We could do that, but why would that be better? > That if backlight device registration fails we do not register all other devices. But yes that may be a bit too harsh, isn't it? > > > > > > > + 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 > > I would rather not, as the backlight device would be in unknown state > here, and using dev_warn with device that has not been fully registered > does not give any benefits. There is also no ambiguity as there is only > one backlight. You are very correct, sorry for the fuss. > > > are missing a '__func__' as second function argument. > > I'll fix this. > > > > > Also, you may want to return error. > > How would caller handle this error? Should we kill all successfully > registered devices on error adding backlight? As the function returned an error code for 'platform_add_devices()' I thought we may want to return one as well. That's why I proposed to invert the registration order :) All minor nits btw, sorry for jumping up, I understand this is an RFC and ecovec board file is not the real juice of this series ;) Ping me if I can help with testing as I've the board. Thanks j > > Thanks. > > -- > Dmitry From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751812AbeCQJZj (ORCPT ); Sat, 17 Mar 2018 05:25:39 -0400 Received: from relay2-d.mail.gandi.net ([217.70.183.194]:36531 "EHLO relay2-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750778AbeCQJZh (ORCPT ); Sat, 17 Mar 2018 05:25:37 -0400 X-Originating-IP: 2.224.242.101 Date: Sat, 17 Mar 2018 10:25:16 +0100 From: jacopo mondi To: Dmitry Torokhov Cc: Laurent Pinchart , Lee Jones , Daniel Thompson , Jingoo Han , linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, Yoshinori Sato , Rich Felker , Linus Walleij Subject: Re: [RFC 2/4] sh: ecovec24: conditionally register backlight device Message-ID: <20180317092516.g5p5ybka57abyhex@apu3b> References: <20180315224202.96668-1-dmitry.torokhov@gmail.com> <20180315224202.96668-3-dmitry.torokhov@gmail.com> <20180316100748.GH16424@w540> <20180316233800.GA32310@dtor-ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180316233800.GA32310@dtor-ws> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dmitry, On Fri, Mar 16, 2018 at 04:38:00PM -0700, Dmitry Torokhov wrote: > Hi Jacopo, > > On Fri, Mar 16, 2018 at 11:07:48AM +0100, jacopo mondi wrote: > > Hello Dmitry > > > > FYI I am brushing the ecovec board these days as well > > https://www.spinics.net/lists/linux-sh/msg52536.html > > > > What is the ecovec board BTW? Is it some devkit or what? It seems quite > old to me. Yes, it is a SuperH 4 based development board. It is old for sure. I'm also working on removing some stuff the ecovec board file is the only user of... > > 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 > > > --- > > > 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. > > We could do that, but why would that be better? > That if backlight device registration fails we do not register all other devices. But yes that may be a bit too harsh, isn't it? > > > > > > > + 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 > > I would rather not, as the backlight device would be in unknown state > here, and using dev_warn with device that has not been fully registered > does not give any benefits. There is also no ambiguity as there is only > one backlight. You are very correct, sorry for the fuss. > > > are missing a '__func__' as second function argument. > > I'll fix this. > > > > > Also, you may want to return error. > > How would caller handle this error? Should we kill all successfully > registered devices on error adding backlight? As the function returned an error code for 'platform_add_devices()' I thought we may want to return one as well. That's why I proposed to invert the registration order :) All minor nits btw, sorry for jumping up, I understand this is an RFC and ecovec board file is not the real juice of this series ;) Ping me if I can help with testing as I've the board. Thanks j > > Thanks. > > -- > Dmitry