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

[Emil Velikov]

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 devm_regulator_get
returns a ERR_PTR [1].
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.

HTH
Emil

[1] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/devres.c#L27
[2] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/core.c#L2189