Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

On 20 March 2018 at 06:24, hl <hl@xxxxxxxxxxxxxx> wrote:
> Hi Emil,
>
>
>
> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
>>
>> On 15 March 2018 at 02:35, hl <hl@xxxxxxxxxxxxxx> 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 <hl@xxxxxxxxxxxxxx> wrote:
>>>>>
>>>>> From: huang lin <hl@xxxxxxxxxxxxxx>
>>>>>
>>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>>> multi panel.
>>>>>
>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>>> Signed-off-by: Lin Huang <hl@xxxxxxxxxxxxxx>
>>>>> ---
>>>>> 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


>> 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 ;-)