On Wed, Mar 15, 2017 at 03:19:13PM +0800, Chris Zhong wrote:
> Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI
> panel.
>
> Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - add some error check
> - always use Low power mode to send commend
> - add comments for all the sleep
> - use DRM_DEV_ERROR instead of dev_err
>
Minor suggestion below. Once that's cleared up, you can add:
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
Also, please add Thierry directly to your patches so he sees them.
> drivers/gpu/drm/panel/Kconfig | 11 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344 ++++++++++++++++++++++++++
> 3 files changed, 356 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c
>
<snip>
> +static int innolux_panel_prepare(struct drm_panel *panel)
> +{
> + struct innolux_panel *innolux = to_innolux_panel(panel);
> + int err, ret;
> +
> + if (innolux->prepared)
> + return 0;
> +
> + gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> +
> + err = regulator_enable(innolux->supply);
> + if (err < 0)
> + return err;
> +
> + /* T2: 15ms - 1000ms */
> + usleep_range(15000, 16000);
> +
> + gpiod_set_value_cansleep(innolux->enable_gpio, 1);
> +
> + /* T4: 15ms - 1000ms */
> + usleep_range(15000, 16000);
> +
> + err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> + if (err < 0) {
> + DRM_DEV_ERROR(panel->dev, "failed to exit sleep mode: %d\n",
> + err);
> + goto poweroff;
> + }
> +
> + /* T6: 120ms - 1000ms*/
> + msleep(120);
> +
> + err = mipi_dsi_dcs_set_display_on(innolux->link);
> + if (err < 0) {
> + DRM_DEV_ERROR(panel->dev, "failed to set display on: %d\n",
> + err);
> + goto poweroff;
> + }
> +
> + /* T7: 5ms */
> + usleep_range(5000, 6000);
> +
> + innolux->prepared = true;
> +
> + return 0;
> +
> +poweroff:
> + ret = regulator_disable(innolux->supply);
> + if (ret)
> + return ret;
I don't think we should be returning this error code. If we're here, it's
because something else triggered err, and we should return that. Change this to:
if (regulator_disable(innolux->supply))
DRM_DEV_ERROR(panel->dev, "failed to disable regulator after error\n");
Sean
> +
> + gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> + return err;
> +}
<snip>
> --
> 2.6.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
