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

[Fabio Estevam]

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?

> +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.

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.

Also, when sleeping for more than 10ms, msleep is a better fit as per
Documentation/timers/timers-howto.txt.

> +       if (rad->reset) {
> +               gpiod_set_value(rad->reset, 0);
> +               usleep_range(15000, 17000);
> +               gpiod_set_value(rad->reset, 1);
> +       }

Another reset?