Re: [PATCH 2/2 v2] drm/panel: Add driver for Samsung S6D16D0 panel

[Sam Ravnborg <sam@ravnborg.org>]

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@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> 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@ravnborg.org>

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel