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 ;-)
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
