Re: [PATCH 2/2] drm/panel: rm67191: Add support for new bus formats

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5021 bytes --]

On Fri, Apr 27, 2018 at 10:38:24AM +0000, Robert Chiras wrote:
> 
> 
> 
> ________________________________
> From: Thierry Reding <thierry.reding@gmail.com>
> Sent: Thursday, April 26, 2018 5:56 PM
> To: Robert Chiras
> Cc: dl-linux-imx; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/panel: rm67191: Add support for new bus formats
> 
> On Tue, Apr 03, 2018 at 01:30:01PM +0300, Robert Chiras wrote:
> > From: Mirela Rabulea <mirela.rabulea@nxp.com>
> >
> > Do not hardcode pixel_format to 0x77 but calculate it from dsi->format.
> > Report all the supported bus formats in get_modes:
> >         MEDIA_BUS_FMT_RGB888_1X24
> >         MEDIA_BUS_FMT_RGB666_1X18
> >         MEDIA_BUS_FMT_RGB565_1X16
> > Change pixelclock from 120 to 132 MHz, or 16 bpp formats will not work.
> >
> > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > ---
> >  drivers/gpu/drm/panel/panel-raydium-rm67191.c | 33 +++++++++++++++++++++++----
> >  1 file changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > index 07b0bd4..6331fef 100644
> > --- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > @@ -178,6 +178,12 @@ static const cmd_set_table manufacturer_cmd_set[] = {
> >        {0x51, 0x04},
> >  };
> >
> > +static const u32 rad_bus_formats[] = {
> > +     MEDIA_BUS_FMT_RGB888_1X24,
> > +     MEDIA_BUS_FMT_RGB666_1X18,
> > +     MEDIA_BUS_FMT_RGB565_1X16,
> > +};
> > +
> >  struct rad_panel {
> >        struct drm_panel base;
> >        struct mipi_dsi_device *dsi;
> > @@ -215,11 +221,27 @@ static int rad_panel_push_cmd_list(struct mipi_dsi_device *dsi)
> >        return ret;
> >  };
> >
> > +static int color_format_from_dsi_format(enum mipi_dsi_pixel_format format)
> > +{
> > +     switch (format) {
> > +     case MIPI_DSI_FMT_RGB565:
> > +             return 0x55;
> > +     case MIPI_DSI_FMT_RGB666:
> > +     case MIPI_DSI_FMT_RGB666_PACKED:
> > +             return 0x66;
> > +     case MIPI_DSI_FMT_RGB888:
> > +             return 0x77;
> > +     default:
> > +             return 0x77; /* for backward compatibility */
> > +     }
> > +};
> > +
> >  static int rad_panel_prepare(struct drm_panel *panel)
> >  {
> >        struct rad_panel *rad = to_rad_panel(panel);
> >        struct mipi_dsi_device *dsi = rad->dsi;
> >        struct device *dev = &dsi->dev;
> > +     int color_format = color_format_from_dsi_format(dsi->format);
> >        int ret;
> >
> >        if (rad->prepared)
> > @@ -276,8 +298,10 @@ static int rad_panel_prepare(struct drm_panel *panel)
> >                DRM_DEV_ERROR(dev, "Failed to set tear scanline (%d)\n", ret);
> >                goto fail;
> >        }
> > -     /* Set pixel format to RGB888 */
> > -     ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);
> > +     /* Set pixel format */
> > +     ret = mipi_dsi_dcs_set_pixel_format(dsi, color_format);
> > +     DRM_DEV_DEBUG_DRIVER(dev, "Interface color format set to 0x%x\n",
> > +                             color_format);
> >        if (ret < 0) {
> >                DRM_DEV_ERROR(dev, "Failed to set pixel format (%d)\n", ret);
> >                goto fail;
> > @@ -393,7 +417,6 @@ static int rad_panel_get_modes(struct drm_panel *panel)
> >        struct device *dev = &rad->dsi->dev;
> >        struct drm_connector *connector = panel->connector;
> >        struct drm_display_mode *mode;
> > -     u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >        u32 *bus_flags = &connector->display_info.bus_flags;
> >        int ret;
> >
> > @@ -420,7 +443,7 @@ static int rad_panel_get_modes(struct drm_panel *panel)
> >                *bus_flags |= DRM_BUS_FLAG_PIXDATA_POSEDGE;
> >
> >        ret = drm_display_info_set_bus_formats(&connector->display_info,
> > -                                            &bus_format, 1);
> > +                     rad_bus_formats, ARRAY_SIZE(rad_bus_formats));
> >        if (ret)
> >                return ret;
> >
> > @@ -492,7 +515,7 @@ static const struct drm_panel_funcs rad_panel_funcs = {
> >   * to 132MHz (60Hz refresh rate)
> >   */
> >  static const struct display_timing rad_default_timing = {
> > -     .pixelclock = { 66000000, 120000000, 132000000 },
> > +     .pixelclock = { 66000000, 132000000, 132000000 },
> 
> This seems like a fix that should be squashed into patch 1.
> 
> [Robert] I did that to preserve the author, but I can squash it into
> patch 1 and add her there.

If you want to preserve authorship, just split it off into a separate
patch. We don't usually do that for initial submissions of a driver, so
you can usually squash such subsequent fixes into the initial patch for
an initial submission and mention contributors in the commit message. I
don't have any strong objections if you want to split the patch out
separately, though.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel