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

[Sam Ravnborg <sam@ravnborg.org>]

Hi Linus.

Few comments below, nothing serious.

On Mon, Oct 08, 2018 at 12:58:52PM +0200, Linus Walleij wrote:
> The Samsung S6D16D0 is a simple comman mode only DSI display
s/comman/command/
> that is used on the ST-Ericsson Ux500 reference design
> TVK1281618 user interface board (UIB).
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/panel/Kconfig                 |   7 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  drivers/gpu/drm/panel/panel-samsung-s6d16d0.c | 235 ++++++++++++++++++
>  3 files changed, 243 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..2aacffee7a86 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -126,6 +126,13 @@ 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
> +	depends on BACKLIGHT_CLASS_DEVICE
Here BACKLIGHT is selected

> +	select VIDEOMODE_HELPERS
> +
>  config DRM_PANEL_SAMSUNG_S6E3HA2
>  	tristate "Samsung S6E3HA2 DSI video mode panel"
>  	depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 5ccaaa9d13af..b52e5c5804ca 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
>  obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
>  obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
> +obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6d16d0.c b/drivers/gpu/drm/panel/panel-samsung-s6d16d0.c
> new file mode 100644
> index 000000000000..da93484ee22f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6d16d0.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MIPI-DSI Samsung s6d16d0 panel driver. This is a 864x480
> + * AMOLED panel with a command-only DSI interface.
> + */
> +
> +#include <drm/drmP.h>
We are trying to get rid of this header.
Does this driver really use anything from that header or
can it be replaced by some other includes?

> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>

In Kconfig BACKLIGHT was selected but there is no backlight
include, and I missed any backlight feature in the remaining patch.

> +{
> +	struct s6d16d0 *s6 = panel_to_s6d16d0(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(s6->dev);
> +	int ret;
> +
> +	/* Exit sleep mode and power on */
> +	ret = mipi_dsi_dcs_set_display_off(dsi);
> +	if (ret) {
> +		dev_err(s6->dev, "failed to turn display off\n");
General comment:
dev_err => DRM_DEV_ERROR?
Should error message include the 'ret' value to make
it easier to track what went wrong?

> +		return ret;
> +	}
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret) {
> +		dev_err(s6->dev, "failed to enter sleep mode\n");
> +		return ret;
> +	}
> +
> +	/* Assert RESET */
> +	gpiod_set_value_cansleep(s6->reset_gpio, 1);
I mentioned in comment to binding doc that it should not
specify that reset is active high, but I can this this is
hardcoded in the driver, so maye the binding is justified.

> +	dsi->mode_flags =
> +		MIPI_DSI_CLOCK_NON_CONTINUOUS |
> +		MIPI_DSI_MODE_EOT_PACKET;
> +
> +	s6->supply = devm_regulator_get(dev, "vdd1");
> +	if (IS_ERR(s6->supply))
> +		return PTR_ERR(s6->supply);
This cannot be -EPROBE_DEFER?

> +						 GPIOD_OUT_HIGH);
> +	if (IS_ERR(s6->reset_gpio)) {
> +		ret = PTR_ERR(s6->reset_gpio);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to request GPIO: %d\n", ret);
> +		return ret;
> +	}
Like this..

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel