Re: [EXT] Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

Hi Fabio,

On Vi, 2019-06-14 at 09:27 -0300, Fabio Estevam wrote:
> Hi Robert,
> 
> On Fri, Jun 14, 2019 at 8:52 AM Robert Chiras <robert.chiras@xxxxxxx>
> wrote:
> 
> > 
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > @@ -0,0 +1,730 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * i.MX drm driver - Raydium MIPI-DSI panel driver
> > + *
> > + * Copyright (C) 2017 NXP
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.ÂÂSee the
> > + * GNU General Public License for more details.
> No need for this text as you are using SPDX tag.
> 
> > 
> > +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;
> Could you use defines for these magic 0x55, 0x66 and 0x77 numbers?
Those magic numbers mean exactly what their case statements are. They
come from the panel documentation. I thought that the already existing
defines (MIPI_DSI_FMT_) are self explanatory here, so using defines
seemed redundant for me. But, if you think that using defines for them
is better, I can do that.
> 
> > 
> > +static int rad_panel_prepare(struct drm_panel *panel)
> > +{
> > +ÂÂÂÂÂÂÂstruct rad_panel *rad = to_rad_panel(panel);
> > +
> > +ÂÂÂÂÂÂÂif (rad->prepared)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂÂÂif (rad->reset) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 0);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(5000, 10000);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 1);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(20000, 25000);
> This does not look correct.
> 
> The correct way to do a reset with gpiod API is:
> 
> Âgpiod_set_value(rad->reset, 1);
> 
> delay
> 
> gpiod_set_value(rad->reset, 0);
> 
> I don't have the datasheet for the RM67191 panel, but I assume the
> reset GPIO is active low.
> 
> Since you inverted the polarity in the dts and inside the driver, you
> got it right by accident.
The GPIO is active high, and the above sequence was received from the
panel vendor in the following form:
	SET_RESET_PIN(1);
	MDELAY(10);
	SET_RESET_PIN(0);
	MDELAY(5);
	SET_RESET_PIN(1);
	MDELAY(20);
I got rid of the first transition to high since seemed redundant.
Also, according to the manual reference, the RSTB pin needs to be
active high while operating the display.
> 
> You could also consider using gpiod_set_value_cansleep() variant
> instead because the GPIO reset could be provided by an I2C GPIO
> expander, for example.
Thanks, will use this in the next revision.
> 
> Also, when sleeping for more than 10ms, msleep is a better fit as per
> Documentation/timers/timers-howto.txt.
OK, I will use msleep. That documentation recommends using usleep_range
for sleeps <20m, but also using msleep for >10ms (so I followed the
recomendations from usleep_range)
> 
> > 
> > +ÂÂÂÂÂÂÂif (rad->reset) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 0);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(15000, 17000);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 1);
> > +ÂÂÂÂÂÂÂ}
> Another reset?
Yes. This is tricky, since this GPIO is shared between the DSI
controller and touch controller. The Android guys needs the touch
controller to be active, while the display can be turned off. So this
is why, after the display is turned off, the reset pin is put back to
HIGH in order to keep the touch working.