| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 17 Mar 2018 19:21:17 +0900
From: John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
To: jacopo mondi <jacopo@...ndi.org>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Lee Jones <lee.jones@...aro.org>,
Daniel Thompson <daniel.thompson@...aro.org>,
Jingoo Han <jingoohan1@...il.com>, linux-fbdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-sh@...r.kernel.org,
Yoshinori Sato <ysato@...rs.sourceforge.jp>,
Rich Felker <dalias@...c.org>,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [RFC 2/4] sh: ecovec24: conditionally register backlight device
> On Mar 17, 2018, at 6:25 PM, jacopo mondi <jacopo@...ndi.org> wrote:
>
> 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...
Umh, but I’m still using the SH7724 Evovec board. Please don’t remove support for that.
The SuperH port of the Linux kernel is still maintained.
Adrian
>
>>> 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@...il.com>
>>>> ---
>>>> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists