Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel

[Robert Chiras <robert.chiras@nxp.com>]

On Vi, 2018-04-27 at 11:08 +0000, Thierry Reding wrote:
> On Fri, Apr 27, 2018 at 10:37:16AM +0000, Robert Chiras wrote:
> > 
> > 
> > Hi Thierry,
> > 
> > Thanks a lot for reviewing this. Your suggestions are very
> > valuable.
> > Please see my detailed answers inline.
> > 
> > Best regards,
> > Robert
> Couple of comments regarding email replies. Please use proper quoting
> of
> what you're replying to. Your mail user agent should be able to do
> that
> automatically. If not, you might want to consider switching to a
> better
> one.
> 
> Also, please try to stick to 78 columns of text. It's difficult to
> read
> something that is wider than that and may get wrapped. It's also
> difficult to reply to paragraphs that are too wide.
> 
> Other than than, some technical comments below.
Sorry about the replies, I was using the web version of Office365 (not
like the Windows app is much better). I switched to Evolution, now.
Also, sorry for the late reply, I waited to include a fix regarding DSI
init sequence. This fix was needed after a client complained about the
fact that there are DSI signals on the data lanes during the reset of
the panel, so I had to separate the GPIO reset from the DSI init.
All your comments should be addressed in the v2 of this patch I just
sent.
> 
> > 
> > ________________________________
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Sent: Thursday, April 26, 2018 5:54 PM
> > To: Robert Chiras
> > Cc: dl-linux-imx; dri-devel@lists.freedesktop.org
> > Subject: Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
> > 
> > On Tue, Apr 03, 2018 at 01:30:00PM +0300, Robert Chiras wrote:
> > > 
> > > Add support for the OLED display based on MIPI-DSI protocol from
> > > Raydium:
> > > RM67191.
> > > 
> > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
> > > ---
> > >  .../bindings/display/panel/raydium,rm67191.txt     |  55 ++
> > >  drivers/gpu/drm/panel/Kconfig                      |   9 +
> > >  drivers/gpu/drm/panel/Makefile                     |   1 +
> > >  drivers/gpu/drm/panel/panel-raydium-rm67191.c      | 645
> > > +++++++++++++++++++++
> > >  4 files changed, 710 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/display/panel/raydium,rm67191.t
> > > xt
> > >  create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/panel/raydium,rm67191
> > > .txt
> > > b/Documentation/devicetree/bindings/display/panel/raydium,rm67191
> > > .txt
> > > new file mode 100644
> > > index 0000000..18a57de
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/display/panel/raydium,rm67191
> > > .txt
> > > @@ -0,0 +1,55 @@
> > > +Raydium RM67171 OLED LCD panel with MIPI-DSI protocol
> > > +
> > > +Required properties:
> > > +- compatible:                 "raydium,rm67191"
> > > +- reg:                       virtual channel for MIPI-DSI
> > > protocol
> > > +                     must be <0>
> > > +- dsi-lanes:         number of DSI lanes to be used
> > > +                     must be <3> or <4>
> > > +- port:               input port node with endpoint definition
> > > as
> > > +                     defined in
> > > Documentation/devicetree/bindings/graph.txt;
> > > +                     the input port should be connected to a
> > > MIPI-DSI device
> > > +                     driver
> > > +
> > > +Optional properties:
> > > +- reset-gpio:                a GPIO spec for the RST_B GPIO pin
> > > +- display-timings:   timings for the connected panel according
> > > to [1]
> > > +- pinctrl-0          phandle to the pin settings for the reset
> > > pin
> > > +- panel-width-mm:    physical panel width [mm]
> > > +- panel-height-mm:   physical panel height [mm]
> > > +
> > > +[1]: Documentation/devicetree/bindings/display/display-
> > > timing.txt
> > > +
> > > +Example:
> > > +
> > > +     panel@0 {
> > > +             compatible = "raydium,rm67191";
> > > +             reg = <0>;
> > > +             pinctrl-0 = <&pinctrl_mipi_dsi_0_1_en>;
> > > +             reset-gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> > > +             dsi-lanes = <4>;
> > > +             panel-width-mm = <68>;
> > > +             panel-height-mm = <121>;
> > > +             display-timings {
> > > +                     timing {
> > > +                             clock-frequency = <132000000>;
> > > +                             hactive = <1080>;
> > > +                             vactive = <1920>;
> > > +                             hback-porch = <11>;
> > > +                             hfront-porch = <4>;
> > > +                             vback-porch = <48>;
> > > +                             vfront-porch = <20>;
> > > +                             hsync-len = <5>;
> > > +                             vsync-len = <12>;
> > > +                             hsync-active = <0>;
> > > +                             vsync-active = <0>;
> > > +                             de-active = <0>;
> > > +                             pixelclk-active = <0>;
> > > +                     };
> > > +             };
> > This shouldn't be necessary. You already have the timings in your
> > driver, why the extra work of getting it from DT?
> > 
> > [Robert] I added this as optional in the DT in case someone would
> > like
> > to use the panel with different timings than ones from driver.
> > Anyway,
> > after some tests I saw that the blanking timings are kind of fixed,
> > so
> > only the clock-frequency could be changed. For example, if someone
> > wants to use this panel at 30Hz (66MHz pixel clock) instead of the
> > default one of 60Hz (132MHz pixel clock). But, I think you are
> > right,
> > I could get rid of the display-timings and just add an optional
> > property for changing the pixel clock in driver.
> To be honest, I wouldn't bother with that. Let someone else add that
> if
> they want to drive the panel at a different mode. Chances are nobody
> will, in which case adding it now would just be dead code that we
> need
> to maintain for no good reason.
> 
> [...]
> > 
> > > 
> > > +
> > > +/* Write Manufacture Command Set Control */
> > > +#define WRMAUCCTR 0xFE
> > > +
> > > +/* Manufacturer Command Set pages (CMD2) */
> > > +static const cmd_set_table manufacturer_cmd_set[] = {
> > There a lot of magic values in this table. Can you add a reference
> > to
> > where you got these from? Also, looking at these commands it seems
> > like they are actually <command, parameter> pairs, so maybe a
> > better
> > representation would be something along the lines of:
> > 
> >         struct cmd_set_entry {
> >                 u8 command;
> >                 u8 param;
> >         };
> > 
> >         static const struct cmd_set_entry manufacturer_cmd_set[] =
> > {
> >                 ...
> >         };
> > 
> > [Robert] Your suggestion is good and I will change the code
> > accordingly. Regarding the "magic values": I got them from a sample
> > driver we received from the vendor. According to the Reference
> > Manual,
> > the IC on this panel has two sets of registers: Manufacture Command
> > Set (MCS = CMD2) and User Command Set (UCS = CMD1). In the RM,
> > there
> > is no detailed description of the MCS, only a reference to a
> > command
> > on how to switch between these command sets. Now, what are those
> > commands used in the MCS mode: we got no details for them (probably
> > to
> > protect their IP?)
> Okay, not much you can do about it, then.
> 
> > 
> > > 
> > > +static const struct backlight_ops rad_bl_ops = {
> > > +     .update_status = rad_bl_update_status,
> > > +     .get_brightness = rad_bl_get_brightness,
> > > +};
> [...]
> > 
> > > 
> > > +static int rad_panel_probe(struct mipi_dsi_device *dsi)
> > > +{
> [...]
> > 
> > > 
> > > +     memset(&bl_props, 0, sizeof(bl_props));
> > > +     bl_props.type = BACKLIGHT_RAW;
> > > +     bl_props.brightness = 255;
> > Maybe this should read the current brightness from the panel to
> > make
> > sure it's correct?
> > 
> > [Robert] In order to read the current brightness from the panel, we
> > need to be able to send DSI commands. Since the DSI commands can be
> > sent by means of the DSI host controller, at this moment is
> > impossible
> > to do this. Anyway, during the panel_prepare, when the panel is
> > initialized, the backlight value is defaulted there, so I can
> > update
> > the current brightness there.
> Couldn't you do it right after the call to mipi_dsi_attach()? It
> seems
> like the panel should be able to execute DSI commands at that time.
> It
> may have to call ->prepare() and ->unprepare() around those, though.
> 
> I guess another possibility would be to do it as part of the
> ->prepare()
> callback.
> 
> Looking a little closer at drivers/video/backlight/backlight.c it
> seems
> like maybe even that wouldn't be necessary. Drivers that implement
> the
> ->get_brightness() callback don't really need to set .brightness, so
> maybe just drop that line?
> 
> > 
> > > 
> > > +
> > > +     ret = mipi_dsi_detach(dsi);
> > > +     if (ret < 0)
> > > +             DRM_DEV_ERROR(dev, "Failed to detach from host
> > > (%d)\n",
> > > +                     ret);
> > > +
> > > +     drm_panel_detach(&rad->base);
> > Please don't do that. I've just merged a set of patches which
> > document
> > this as being wrong and which remove similar calls from other panel
> > drivers.
> > 
> > [Robert] I am sorry, but I don't understand here. Don't do what?
> > mipi_dsi_detach or drm_panel_detach? Or are you referring to the
> > fact
> > that I'm still calling drm_panel_detach even though mipi_dsi_detach
> > fails?
> drm_panel_detach() should not be called from panel drivers. It should
> be
> called from display drivers to detach from the panel.
> 
> Thierry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel