Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
Am Mittwoch, 14. März 2018, 13:02:13 CEST schrieb Emil Velikov: > Hi Lin, > > On 14 March 2018 at 09:12, Lin Huang wrote: > > From: huang lin > > > > Refactor Innolux P079ZCA panel driver, let it support > > multi panel. > > > > Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 > > Signed-off-by: Lin Huang [...] > > @@ -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. using the regulator_bulk APIs should help to make this far easier, as you can just define the per-panel supplies in in the panel_desc and then get + enable the correct ones per bound panel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
On Wed, Mar 14, 2018 at 05:12:14PM +0800, Lin Huang wrote: > From: huang lin > > Refactor Innolux P079ZCA panel driver, let it support > multi panel. > > Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 When you send out a revised set of these patches, please drop the Change-Id: line from the commit message. It is useless for upstream. Thierry signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
Hi On Tuesday, March 20, 2018 06:20 PM, Emil Velikov wrote: On 20 March 2018 at 06:24, hl wrote: Hi Emil, On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote: On 15 March 2018 at 02:35, hl 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 wrote: From: huang lin Refactor Innolux P079ZCA panel driver, let it support multi panel. Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 Signed-off-by: Lin Huang --- 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 ;-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
Hi Emil, On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote: On 15 March 2018 at 02:35, hl 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 wrote: From: huang lin Refactor Innolux P079ZCA panel driver, let it support multi panel. Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 Signed-off-by: Lin Huang --- 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: rdev = regulator_dev_lookup(dev, id); if (IS_ERR(rdev)) { . .. switch (get_type) { case NORMAL_GET: /* * Assume that a regulator is physically present and * enabled, even if it isn't hooked up, and just * provide a dummy. */ dev_warn(dev, "%s supply %s not found, using dummy regulator\n", devname, id); rdev = dummy_regulator_rdev; get_device(&rdev->dev); break; ... ... } regulator = create_regulator(rdev, dev, id); ... it wil get a dummy_regulator for it. 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). 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
On 20 March 2018 at 06:24, hl wrote: > Hi Emil, > > > > On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote: >> >> On 15 March 2018 at 02:35, hl 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 wrote: > > From: huang lin > > Refactor Innolux P079ZCA panel driver, let it support > multi panel. > > Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 > Signed-off-by: Lin Huang > --- > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
On 15 March 2018 at 02:35, hl 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 wrote: >>> >>> From: huang lin >>> >>> Refactor Innolux P079ZCA panel driver, let it support >>> multi panel. >>> >>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 >>> Signed-off-by: Lin Huang >>> --- >>> 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
Hi Emil, On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote: Hi Lin, On 14 March 2018 at 09:12, Lin Huang wrote: From: huang lin Refactor Innolux P079ZCA panel driver, let it support multi panel. Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 Signed-off-by: Lin Huang --- 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 "); +MODULE_AUTHOR("Lin Huang "); I don't think refactoring existing code classify as being the module author. Then again, I could be wrong. HTH Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
From: huang lin Refactor Innolux P079ZCA panel driver, let it support multi panel. Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 Signed-off-by: Lin Huang --- 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 drivers/gpu/drm/panel/panel-innolux-p079zca.c | 142 ++ 1 file changed, 101 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c index 57df39b..2075a9d 100644 --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c @@ -20,12 +20,28 @@ #include +struct panel_desc { + const struct drm_display_mode *modes; + unsigned int bpc; + struct { + unsigned int width; + unsigned int height; + } size; + + unsigned long flags; + enum mipi_dsi_pixel_format format; + unsigned int lanes; +}; + 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; 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; + regulator_disable(innolux->avee); + regulator_disable(innolux->avdd); + regulator_disable(innolux->vddi); innolux->prepared = false; @@ -89,17 +105,25 @@ static int innolux_panel_unprepare(struct drm_panel *panel) static int innolux_panel_prepare(struct drm_panel *panel) { struct innolux_panel *innolux = to_innolux_panel(panel); - int err, regulator_err; + int err; if (innolux->prepared) return 0; gpiod_set_value_cansleep(innolux->enable_gpio, 0); - err = regulator_enable(innolux->supply); + err = regulator_enable(innolux->vddi); if (err < 0) return err; + err = regulator_enable(innolux->avdd); + if (err < 0) + goto disable_vddi; + + err = regulator_enable(innolux->avee); + if (err < 0) + goto disable_avdd; + /* T2: 15ms - 1000ms */ usleep_range(15000, 16000); @@ -133,12 +157,13 @@ static int innolux_panel_prepare(struct drm_panel *panel) return 0; poweroff: - regulator_err = regulator_disable(innolux->supply); - if (regulator_err) - DRM_DEV_ERROR(panel->dev, "failed to disable regulator: %d\n", - regulator_err); - gpiod_set_value_cansleep(innolux->enable_gpio, 0); + regulator_disable(innolux->avee); +disable_avdd: + regulator_disable(innolux->avdd); +disable_vddi: + regulator_disable(innolux->vddi); + return err; } @@ -162,7 +187,7 @@ static int innolux_panel_enable(struct drm_panel *panel) return 0; } -static const struct drm_display_mode default_mode = { +static const struct drm_display_mode innolux_p079zca_mode = { .clock = 56900, .hdisplay = 768, .hsync_start = 768 + 40, @@ -175,15 +200,29 @@ static const struct drm_display_mode default_mode = { .vrefresh = 60, }; +static const struct panel_desc innolux_p079zca_panel_desc = { + .modes = &innolux_p079zca_mode, + .bpc = 8, + .size = { + .width = 120, + .height = 160, + }, + .flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | +MIPI_DSI_MODE_LPM, + .format = MIPI_DSI_FMT_RGB888, + .lanes = 4, +}; + static int innolux_panel_get_modes(struct drm_panel *panel) { struct drm_display_mode *mode; + struct innolux_panel *innolux = to_innolux_panel(panel); + const struct drm_display_mode *m = innolux->desc->modes; - mode = drm_mode_duplicate(panel->drm, &default_mode); + mode = drm_mode_duplicate(panel->drm, m); if (!mode) { DRM_DEV_ERROR(panel->drm->dev, "failed to add mode %ux%ux@%u\n", - default_mode.hdisplay, default_mode.vdisplay, - default_mode.vrefresh); + m->hdisplay, m->vdisplay, m->vrefresh); return -ENOMEM; } @@ -191,9 +230,11 @@ static int innolux_panel_get_modes(struct drm_panel *panel) drm_mode_probed_add(panel->connector, mode); - panel->connector->display_info.width_mm = 120; - panel->connector->display_inf
Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
Hi Lin, On 14 March 2018 at 09:12, Lin Huang wrote: > From: huang lin > > Refactor Innolux P079ZCA panel driver, let it support > multi panel. > > Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2 > Signed-off-by: Lin Huang > --- > 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. > @@ -318,5 +377,6 @@ static struct mipi_dsi_driver innolux_panel_driver = { > module_mipi_dsi_driver(innolux_panel_driver); > > MODULE_AUTHOR("Chris Zhong "); > +MODULE_AUTHOR("Lin Huang "); I don't think refactoring existing code classify as being the module author. Then again, I could be wrong. HTH Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel