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.
> ________________________________
> From: Thierry Reding <thierry.reding@xxxxxxxxx>
> Sent: Thursday, April 26, 2018 5:54 PM
> To: Robert Chiras
> Cc: dl-linux-imx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > .../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.txt
> > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
