Hi Linus.
On Tue, Oct 16, 2018 at 02:34:09PM +0200, Linus Walleij wrote:
> The Samsung S6D16D0 is a simple comman mode only DSI display
> that is used on the ST-Ericsson Ux500 reference design
> TVK1281618 user interface board (UIB).
>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Drop Backlight dependency from the Kconfig - this display
> has built-in backlight, I think.
> - Drop the big <drm/drmP.h> include and replace by more precise
> to-the-point includes.
> - Move display on/off to the enable/disable callbacks.
> - Use DRM_DEV_ERROR() instead of dev_err().
Some small things noticed. It has my
Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
if you consider my comments (no matter your decision).
Sam
> ---
> drivers/gpu/drm/panel/Kconfig | 6 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-samsung-s6d16d0.c | 255 ++++++++++++++++++
> 3 files changed, 262 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6d16d0.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6020c30a33b3..d3bb5b526015 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -126,6 +126,12 @@ config DRM_PANEL_RAYDIUM_RM68200
> Say Y here if you want to enable support for Raydium RM68200
> 720x1280 DSI video mode panel.
>
> +config DRM_PANEL_SAMSUNG_S6D16D0
> + tristate "Samsung S6D16D0 DSI video mode panel"
> + depends on OF
> + depends on DRM_MIPI_DSI
> + select VIDEOMODE_HELPERS
I cannot see this driver uses any of the helpers, and think
this select is not needed.
> +static int s6d16d0_get_modes(struct drm_panel *panel)
> +{
> + struct drm_connector *connector = panel->connector;
> + struct drm_display_mode *mode;
> +
> + strncpy(connector->display_info.name, "Samsung S6D16D0\0",
> + DRM_DISPLAY_INFO_LEN);
> +
> + mode = drm_mode_duplicate(panel->drm, &samsung_s6d16d0_mode);
> + if (!mode) {
> + DRM_ERROR("bad mode or failed to add mode\n");
> + return -EINVAL;
> + }
> + drm_mode_set_name(mode);
> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> + mode->width_mm = 84;
> + mode->height_mm = 48;
If samsung_s6d16d0_mode had width_mm and height_mm set then
this assignment was not required.
And it would be cleaner to have timing and physical size set
in the same place.
> + connector->display_info.width_mm = mode->width_mm;
> + connector->display_info.height_mm = mode->height_mm;
> +
> + drm_mode_probed_add(connector, mode);
> +
> + return 1; /* Number of modes */
> +}
> +static int s6d16d0_probe(struct mipi_dsi_device *dsi)
> +{
> + struct device *dev = &dsi->dev;
> + struct s6d16d0 *s6;
> + int ret;
> +
> + s6 = devm_kzalloc(dev, sizeof(struct s6d16d0), GFP_KERNEL);
> + if (!s6)
> + return -ENOMEM;
Add empty line here (makes it easier to read and follows style in rest of file)
> + mipi_dsi_set_drvdata(dsi, s6);
> + s6->dev = dev;
> +
> + dsi->lanes = 2;
> + dsi->format = MIPI_DSI_FMT_RGB888;
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
