From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754053AbeCVBth (ORCPT ); Wed, 21 Mar 2018 21:49:37 -0400 Received: from lucky1.263xmail.com ([211.157.147.132]:42826 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752725AbeCVBtf (ORCPT ); Wed, 21 Mar 2018 21:49:35 -0400 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: hl@rock-chips.com X-FST-TO: dri-devel@lists.freedesktop.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: hl@rock-chips.com X-UNIQUE-TAG: <1606a364f1b77120ae2c87bc4969b297> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver To: Emil Velikov Cc: Thierry Reding , Brian Norris , Rob Herring , Sean Paul , David Airlie , "Linux-Kernel@Vger. Kernel. Org" , ML dri-devel References: <1521018736-20980-1-git-send-email-hl@rock-chips.com> <577e35f7-b7ca-ca97-1391-f5759e81254e@rock-chips.com> From: hl Message-ID: Date: Thu, 22 Mar 2018 09:49:25 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi On Tuesday, March 20, 2018 06:20 PM, Emil Velikov wrote: > On 20 March 2018 at 06:24, hl wrote: >> Hi Emil, >> >> >> >> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote: >>> On 15 March 2018 at 02:35, hl wrote: >>>> Hi Emil, >>>> >>>> >>>> >>>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote: >>>>> Hi Lin, >>>>> >>>>> On 14 March 2018 at 09:12, Lin Huang wrote: >>>>>> From: huang lin >>>>>> >>>>>> Refactor Innolux P079ZCA panel driver, let it support >>>>>> multi panel. >>>>>> >>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 >>>>>> Signed-off-by: Lin Huang >>>>>> --- >>>>>> Changes in v2: >>>>>> - Change regulator property name to meet the panel datasheet >>>>>> Changes in v3: >>>>>> - this patch only refactor P079ZCA panel to support multi panel, >>>>>> support >>>>>> P097PFG panel in another patch >>>>>> Changes in v4: >>>>>> - Modify the patch which suggest by Thierry >>>>>> >>>>> Thanks for splitting this up. I think there's another piece that fell >>>>> through the cracks. >>>>> I'm not deeply familiar with the driver, so just sharing some quick >>>>> notes. >>>>> >>>>> >>>>>> struct innolux_panel { >>>>>> struct drm_panel base; >>>>>> struct mipi_dsi_device *link; >>>>>> + const struct panel_desc *desc; >>>>>> >>>>>> struct backlight_device *backlight; >>>>>> - struct regulator *supply; >>>>>> + struct regulator *vddi; >>>>>> + struct regulator *avdd; >>>>>> + struct regulator *avee; >>>>> These two seem are new addition, as opposed to a dummy refactor. >>>>> Are they optional, does one need them for the existing panel (separate >>>>> patch?) or only for the new one (squash with the new panel code)? >>>>> >>>>> >>>>>> struct gpio_desc *enable_gpio; >>>>>> >>>>>> bool prepared; >>>>>> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel >>>>>> *panel) >>>>>> /* T8: 80ms - 1000ms */ >>>>>> msleep(80); >>>>>> >>>>>> - err = regulator_disable(innolux->supply); >>>>>> - if (err < 0) >>>>>> - return err; >>>>> Good call on dropping the early return here. >>>>> >>>>> >>>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs >>>>>> innolux_panel_funcs = { >>>>>> - innolux->supply = devm_regulator_get(dev, "power"); >>>>>> - if (IS_ERR(innolux->supply)) >>>>>> - return PTR_ERR(innolux->supply); >>>>>> + innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL); >>>>>> + if (!innolux) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + innolux->desc = desc; >>>>>> + innolux->vddi = devm_regulator_get(dev, "power"); >>>>>> + innolux->avdd = devm_regulator_get(dev, "avdd"); >>>>>> + innolux->avee = devm_regulator_get(dev, "avee"); >>>>>> >>>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be >>>>> passed into regulator_{enable,disable}. >>>>> Hence, the IS_ERR check should stay. If any of the regulators are >>>>> optional, you want to call regulator_{enable,disable} only as >>>>> applicable. >>>> >>>> devm_regulator_get() will use dummy_regulator if there not regulator pass >>>> to >>>> driver, so it not affect regulator_{enable, disable}. >>> One of us is getting confused here: >>> devm_regulator_get does not _use_ a regulator, it returns a pointer to >>> a regulator, right? >>> >>> According to the 4.16-rc6 codebase - one error >>> returns a ERR_PTR [1]. >> devm_regulator_get() will not reurn a ERR_PTR, it will pass NORMAL_GET mode >> to >> _regulator_get() when you call devm_regulator_get(), and with following >> code: >> > Just before the _regulator_get() call we have "return ERR_PTR(-ENOMEM);" > See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/devres.c?h=v4.16-rc6#n34 Okay, i got what you concern now, yes, you are right, i will keep IS_ERR checking here. > >>> With the pointer dereferenced in regulator_enable [2], without a >>> IS_ERR check, hence thing will go boom(?) >>> >>>> These three regulator are >>>> optional, >>>> the p079zca will use "power" and , >>>> so i think it better not to check ERR here. >>>> >>> What should happen if p079zca is missing "power" or p097pgf - "avdd" and >>> "avee"? >>> Obviously the latter two should be introduced with the p097pgf support. >> i think it need dts to make sure configure the power node correct, if >> missing >> "power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can >> not work, but do not affcet other driver, the kernel do not crash(as i >> explain before and i also test it). >> > If you know it won't work just don't continue? And yes, it will crash ;-) > Either way, if you don't like my feedback so be it. > > HTH > Emil > P.S. As a non native English speaker to another - spell checker helps a lot ;-) > > >