Messages in this thread Patch in this message | | | Subject | Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver | From | hl <> | Date | Thu, 15 Mar 2018 10:35:10 +0800 |
| |
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@rock-chips.com> wrote: >> From: huang lin <hl@rock-chips.com> >> >> Refactor Innolux P079ZCA panel driver, let it support >> multi panel. >> >> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 >> Signed-off-by: Lin Huang <hl@rock-chips.com> >> --- >> 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}. These three regulator are optional, the p079zca will use "power" and p097pgf will use "avdd" and "avee", so i think it better not to check ERR here.
> >> @@ -318,5 +377,6 @@ static struct mipi_dsi_driver innolux_panel_driver = { >> module_mipi_dsi_driver(innolux_panel_driver); >> >> MODULE_AUTHOR("Chris Zhong <zyw@rock-chips.com>"); >> +MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>"); > I don't think refactoring existing code classify as being the module author. > Then again, I could be wrong. > > HTH > Emil > > >
| |