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

[hl]

Hi


On Tuesday, March 20, 2018 06:20 PM, Emil Velikov wrote:
> 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
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 ;-)