Re: [EXT] Re: [PATCH v2 2/2] drm/panel: Add support for Raydium RM67191 panel driver
On Mi, 2019-06-19 at 10:28 -0300, Fabio Estevam wrote:
> Caution: EXT Email
>
> Hi Robert,
>
> On Tue, Jun 18, 2019 at 10:31 AM Robert Chiras > wrote:
>
> >
> > +static const struct display_timing rad_default_timing = {
> > + .pixelclock = { 6600, 13200, 13200 },
> > + .hactive = { 1080, 1080, 1080 },
> > + .hfront_porch = { 20, 20, 20 },
> > + .hsync_len = { 2, 2, 2 },
> > + .hback_porch = { 34, 34, 34 },
> > + .vactive = { 1920, 1920, 1920 },
> > + .vfront_porch = { 10, 10, 10 },
> > + .vsync_len = { 2, 2, 2 },
> > + .vback_porch = { 4, 4, 4 },
> Are you sure that the sync_len and porch parameters are the same for
> both 66MHz and 132MHz?
Probably they are not, since I didn't get this panel to work well with
66Hz. So I will just keep 132MHz, since these are the only timinings we
received from vendor.
Re: [EXT] Re: [PATCH v2 2/2] drm/panel: Add support for Raydium RM67191 panel driver
Hi Sam,
On Mi, 2019-06-19 at 15:25 +0200, Sam Ravnborg wrote:
> On Tue, Jun 18, 2019 at 04:30:46PM +0300, Robert Chiras wrote:
> >
> > This patch adds Raydium RM67191 TFT LCD panel driver (MIPI-DSI
> > protocol).
> >
> > Signed-off-by: Robert Chiras
> Please include in the changelog a list of what was updated - like
> this:
>
> v2:
> - added kconif symbol sorted (sam)
> - another nitpick (foo)
> - etc
>
> In general try to namme who gave feedback to give them some credit.
Thanks. I will keep this in mind, next time
>
> Who is maintainer for this onwards?
I can offer myself to maintain this.
>
> >
> > ---
> > drivers/gpu/drm/panel/Kconfig | 9 +
> > drivers/gpu/drm/panel/Makefile| 1 +
> > drivers/gpu/drm/panel/panel-raydium-rm67191.c | 709
> > ++
> > 3 files changed, 719 insertions(+)
> > create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
> >
> > +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_cansleep(rad->reset, 1);
> > + usleep_range(3000, 5000);
> > + gpiod_set_value_cansleep(rad->reset, 0);
> So writing a 0 will release reset.
> >
> > + usleep_range(18000, 2);
> > + }
> > +
> > + rad->prepared = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int rad_panel_unprepare(struct drm_panel *panel)
> > +{
> > + struct rad_panel *rad = to_rad_panel(panel);
> > +
> > + if (!rad->prepared)
> > + return 0;
> > +
> > + if (rad->reset) {
> > + gpiod_set_value_cansleep(rad->reset, 1);
> > + usleep_range(15000, 17000);
> > + gpiod_set_value_cansleep(rad->reset, 0);
> Looks strange that reset is released in unprepare.
> I would expect it to stay reset to minimize power etc.
Yes, it looks strange indeed. The reason here is coming from the fact
that this panel also has touch-screen that shares this reset pin with
the OLED display. While the display may be turned off, the touch driver
may need to keep the connection active with the touch controller from
this panel. This is the reason for releasing the reset pin in
unprepare. I will add this in a comment in the code, to eliminate the
confusion.
>
> >
> > +
> > + ret = drm_display_info_set_bus_formats(
> > >display_info,
> > +rad_bus_formats,
> > +ARRAY_SIZE(rad_bus_for
> > mats));
> Other drivers has this as the last stement in their enable function.
> I did not check why, but maybe something to invest a few minutes
> into.
> Be different only if there is a reason to do so.
Ok, I will move this as the last statement. I also noticed that the
other panel drivers do not check the return of this call, like I do
here, so I will also remove this too.
>
> >
> > + if (ret)
> > + return ret;
> > +
> > + drm_mode_probed_add(panel->connector, mode);
> > +
> > + return 1;
> > +}
> > +
> > +
> > +#ifdef CONFIG_PM
> > +static int rad_panel_suspend(struct device *dev)
> > +{
> > + struct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > + if (!rad->reset)
> > + return 0;
> > +
> > + devm_gpiod_put(dev, rad->reset);
> > + rad->reset = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static int rad_panel_resume(struct device *dev)
> > +{
> > + struct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > + if (rad->reset)
> > + return 0;
> > +
> > + rad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(rad->reset))
> > + rad->reset = NULL;
> > +
> > + return PTR_ERR_OR_ZERO(rad->reset);
> > +}
> > +
> > +#endif
> Use __maybe_unused for the tw functions above.
> And loose the ifdef...
Thanks. Will apply.
>
> >
> > +
> > +static const struct dev_pm_ops rad_pm_ops = {
> > + SET_RUNTIME_PM_OPS(rad_panel_suspend, rad_panel_resume, NULL)
> > + SET_SYSTEM_SLEEP_PM_OPS(rad_panel_suspend, rad_panel_resume)
> > +};
> > +
> > +static const struct of_device_id rad_of_match[] = {
> > + { .compatible = "raydium,rm67191", },
> > + { }
> We often, but not always, write this as { /* sentinal */ },
Thanks. Will apply.
>
> >
> > +};
> > +MODULE_DEVICE_TABLE(of, rad_of_match);
> > +
> > +static struct mipi_dsi_driver rad_panel_driver = {
> > + .driver = {
> > + .name = "panel-raydium-rm67191",
> > + .of_match_table = rad_of_match,
> > + .pm = _pm_ops,
> > + },
> > + .probe = rad_panel_probe,
> > + .remove = rad_panel_remove,
> > + .shutdown = rad_panel_shutdown,
> > +};
> > +module_mipi_dsi_driver(rad_panel_driver);
> > +
> > +MODULE_AUTHOR("Robert Chiras ");
> > +MODULE_DESCRIPTION("DRM Driver for Raydium RM67191 MIPI DSI
> >
Re: [PATCH v2 2/2] drm/panel: Add support for Raydium RM67191 panel driver
On Tue, Jun 18, 2019 at 10:31 AM Robert Chiras wrote:
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> @@ -0,0 +1,709 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX drm driver - Raydium MIPI-DSI panel driver
Please remove the "i.MX drm driver" as there is nothing i.MX specific
in this driver.
> + *
> + * Copyright 2019 NXP
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +/* Write Manufacture Command Set Control */
> +#define WRMAUCCTR 0xFE
> +
> +#define COL_FMT_16BPP 0x55
> +#define COL_FMT_18BPP 0x66
> +#define COL_FMT_24BPP 0x77
> +
> +/* Manufacturer Command Set pages (CMD2) */
> +struct cmd_set_entry {
> + u8 cmd;
> + u8 param;
> +};
> +
> +/*
> + * There is no description in the Reference Manual about these commands.
> + * We received them from vendor, so just use them as is.
> + */
> +static const struct cmd_set_entry manufacturer_cmd_set[] = {
> + {0xFE, 0x0B},
> + {0x28, 0x40},
> + {0x29, 0x4F},
> + {0xFE, 0x0E},
> + {0x4B, 0x00},
> + {0x4C, 0x0F},
> + {0x4D, 0x20},
> + {0x4E, 0x40},
> + {0x4F, 0x60},
> + {0x50, 0xA0},
> + {0x51, 0xC0},
> + {0x52, 0xE0},
> + {0x53, 0xFF},
> + {0xFE, 0x0D},
> + {0x18, 0x08},
> + {0x42, 0x00},
> + {0x08, 0x41},
> + {0x46, 0x02},
> + {0x72, 0x09},
> + {0xFE, 0x0A},
> + {0x24, 0x17},
> + {0x04, 0x07},
> + {0x1A, 0x0C},
> + {0x0F, 0x44},
> + {0xFE, 0x04},
> + {0x00, 0x0C},
> + {0x05, 0x08},
> + {0x06, 0x08},
> + {0x08, 0x08},
> + {0x09, 0x08},
> + {0x0A, 0xE6},
> + {0x0B, 0x8C},
> + {0x1A, 0x12},
> + {0x1E, 0xE0},
> + {0x29, 0x93},
> + {0x2A, 0x93},
> + {0x2F, 0x02},
> + {0x31, 0x02},
> + {0x33, 0x05},
> + {0x37, 0x2D},
> + {0x38, 0x2D},
> + {0x3A, 0x1E},
> + {0x3B, 0x1E},
> + {0x3D, 0x27},
> + {0x3F, 0x80},
> + {0x40, 0x40},
> + {0x41, 0xE0},
> + {0x4F, 0x2F},
> + {0x50, 0x1E},
> + {0xFE, 0x06},
> + {0x00, 0xCC},
> + {0x05, 0x05},
> + {0x07, 0xA2},
> + {0x08, 0xCC},
> + {0x0D, 0x03},
> + {0x0F, 0xA2},
> + {0x32, 0xCC},
> + {0x37, 0x05},
> + {0x39, 0x83},
> + {0x3A, 0xCC},
> + {0x41, 0x04},
> + {0x43, 0x83},
> + {0x44, 0xCC},
> + {0x49, 0x05},
> + {0x4B, 0xA2},
> + {0x4C, 0xCC},
> + {0x51, 0x03},
> + {0x53, 0xA2},
> + {0x75, 0xCC},
> + {0x7A, 0x03},
> + {0x7C, 0x83},
> + {0x7D, 0xCC},
> + {0x82, 0x02},
> + {0x84, 0x83},
> + {0x85, 0xEC},
> + {0x86, 0x0F},
> + {0x87, 0xFF},
> + {0x88, 0x00},
> + {0x8A, 0x02},
> + {0x8C, 0xA2},
> + {0x8D, 0xEA},
> + {0x8E, 0x01},
> + {0x8F, 0xE8},
> + {0xFE, 0x06},
> + {0x90, 0x0A},
> + {0x92, 0x06},
> + {0x93, 0xA0},
> + {0x94, 0xA8},
> + {0x95, 0xEC},
> + {0x96, 0x0F},
> + {0x97, 0xFF},
> + {0x98, 0x00},
> + {0x9A, 0x02},
> + {0x9C, 0xA2},
> + {0xAC, 0x04},
> + {0xFE, 0x06},
> + {0xB1, 0x12},
> + {0xB2, 0x17},
> + {0xB3, 0x17},
> + {0xB4, 0x17},
> + {0xB5, 0x17},
> + {0xB6, 0x11},
> + {0xB7, 0x08},
> + {0xB8, 0x09},
> + {0xB9, 0x06},
> + {0xBA, 0x07},
> + {0xBB, 0x17},
> + {0xBC, 0x17},
> + {0xBD, 0x17},
> + {0xBE, 0x17},
> + {0xBF, 0x17},
> + {0xC0, 0x17},
> + {0xC1, 0x17},
> + {0xC2, 0x17},
> + {0xC3, 0x17},
> + {0xC4, 0x0F},
> + {0xC5, 0x0E},
> + {0xC6, 0x00},
> + {0xC7, 0x01},
> + {0xC8, 0x10},
> + {0xFE, 0x06},
> + {0x95, 0xEC},
> + {0x8D, 0xEE},
> + {0x44, 0xEC},
> + {0x4C, 0xEC},
> + {0x32, 0xEC},
> + {0x3A, 0xEC},
> + {0x7D, 0xEC},
> + {0x75, 0xEC},
> + {0x00, 0xEC},
> + {0x08, 0xEC},
> + {0x85, 0xEC},
> + {0xA6, 0x21},
> + {0xA7, 0x05},
> + {0xA9, 0x06},
> + {0x82, 0x06},
> + {0x41, 0x06},
> + {0x7A, 0x07},
> + {0x37, 0x07},
> + {0x05, 0x06},
> + {0x49, 0x06},
> + {0x0D, 0x04},
> + {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 panel;
> + struct mipi_dsi_device *dsi;
> +
> + struct gpio_desc *reset;
> + struct backlight_device *backlight;
> +
> + bool prepared;
> + bool enabled;
> +
> + struct videomode vm;
> + u32 width_mm;
> + u32 height_mm;
> +};
> +
>
Re: [PATCH v2 2/2] drm/panel: Add support for Raydium RM67191 panel driver
Hi Robert,
On Tue, Jun 18, 2019 at 10:31 AM Robert Chiras wrote:
> +static const struct display_timing rad_default_timing = {
> + .pixelclock = { 6600, 13200, 13200 },
> + .hactive = { 1080, 1080, 1080 },
> + .hfront_porch = { 20, 20, 20 },
> + .hsync_len = { 2, 2, 2 },
> + .hback_porch = { 34, 34, 34 },
> + .vactive = { 1920, 1920, 1920 },
> + .vfront_porch = { 10, 10, 10 },
> + .vsync_len = { 2, 2, 2 },
> + .vback_porch = { 4, 4, 4 },
Are you sure that the sync_len and porch parameters are the same for
both 66MHz and 132MHz?
Re: [PATCH v2 2/2] drm/panel: Add support for Raydium RM67191 panel driver
On Tue, Jun 18, 2019 at 04:30:46PM +0300, Robert Chiras wrote:
> This patch adds Raydium RM67191 TFT LCD panel driver (MIPI-DSI
> protocol).
>
> Signed-off-by: Robert Chiras
Please include in the changelog a list of what was updated - like this:
v2:
- added kconif symbol sorted (sam)
- another nitpick (foo)
- etc
In general try to namme who gave feedback to give them some credit.
Who is maintainer for this onwards?
> ---
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile| 1 +
> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 709
> ++
> 3 files changed, 719 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
>
> +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_cansleep(rad->reset, 1);
> + usleep_range(3000, 5000);
> + gpiod_set_value_cansleep(rad->reset, 0);
So writing a 0 will release reset.
> + usleep_range(18000, 2);
> + }
> +
> + rad->prepared = true;
> +
> + return 0;
> +}
> +
> +static int rad_panel_unprepare(struct drm_panel *panel)
> +{
> + struct rad_panel *rad = to_rad_panel(panel);
> +
> + if (!rad->prepared)
> + return 0;
> +
> + if (rad->reset) {
> + gpiod_set_value_cansleep(rad->reset, 1);
> + usleep_range(15000, 17000);
> + gpiod_set_value_cansleep(rad->reset, 0);
Looks strange that reset is released in unprepare.
I would expect it to stay reset to minimize power etc.
> +
> + ret = drm_display_info_set_bus_formats(>display_info,
> +rad_bus_formats,
> +ARRAY_SIZE(rad_bus_formats));
Other drivers has this as the last stement in their enable function.
I did not check why, but maybe something to invest a few minutes into.
Be different only if there is a reason to do so.
> + if (ret)
> + return ret;
> +
> + drm_mode_probed_add(panel->connector, mode);
> +
> + return 1;
> +}
> +
> +
> +#ifdef CONFIG_PM
> +static int rad_panel_suspend(struct device *dev)
> +{
> + struct rad_panel *rad = dev_get_drvdata(dev);
> +
> + if (!rad->reset)
> + return 0;
> +
> + devm_gpiod_put(dev, rad->reset);
> + rad->reset = NULL;
> +
> + return 0;
> +}
> +
> +static int rad_panel_resume(struct device *dev)
> +{
> + struct rad_panel *rad = dev_get_drvdata(dev);
> +
> + if (rad->reset)
> + return 0;
> +
> + rad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(rad->reset))
> + rad->reset = NULL;
> +
> + return PTR_ERR_OR_ZERO(rad->reset);
> +}
> +
> +#endif
Use __maybe_unused for the tw functions above.
And loose the ifdef...
> +
> +static const struct dev_pm_ops rad_pm_ops = {
> + SET_RUNTIME_PM_OPS(rad_panel_suspend, rad_panel_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(rad_panel_suspend, rad_panel_resume)
> +};
> +
> +static const struct of_device_id rad_of_match[] = {
> + { .compatible = "raydium,rm67191", },
> + { }
We often, but not always, write this as { /* sentinal */ },
> +};
> +MODULE_DEVICE_TABLE(of, rad_of_match);
> +
> +static struct mipi_dsi_driver rad_panel_driver = {
> + .driver = {
> + .name = "panel-raydium-rm67191",
> + .of_match_table = rad_of_match,
> + .pm = _pm_ops,
> + },
> + .probe = rad_panel_probe,
> + .remove = rad_panel_remove,
> + .shutdown = rad_panel_shutdown,
> +};
> +module_mipi_dsi_driver(rad_panel_driver);
> +
> +MODULE_AUTHOR("Robert Chiras ");
> +MODULE_DESCRIPTION("DRM Driver for Raydium RM67191 MIPI DSI panel");
> +MODULE_LICENSE("GPL v2");
With the above trivialities considered/fixed:
Reviewed-by: Sam Ravnborg
Sam
