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

Message ID 20181008105852.24196-2-linus.walleij@linaro.org
State New
Headers show
Series
  • [1/2] drm/panel: Add DT bindings for Samsung S6D16D0
Related show

Commit Message

Linus Walleij Oct. 8, 2018, 10:58 a.m.
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).

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

Comments

Andrzej Hajda Oct. 9, 2018, 8:29 a.m. | #1
On 08.10.2018 12:58, 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).
>
> 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
> +	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>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct s6d16d0 {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	struct regulator *supply;
> +	struct gpio_desc *reset_gpio;
> +};
> +
> +/*
> + * The timings are not very helpful as the display is used in
> + * command mode.
> + */
> +static const struct drm_display_mode samsung_s6d16d0_mode = {
> +	.clock = 420160, /* HS 420160kHz, LP 19200kHz */
> +	.hdisplay = 864,
> +	.hsync_start = 864 + 154,
> +	.hsync_end = 864 + 154 + 16,
> +	.htotal = 864 + 154 + 16 + 32,
> +	.vdisplay = 480,
> +	.vsync_start = 480 + 1,
> +	.vsync_end = 480 + 1 + 1,
> +	.vtotal = 480 + 1 + 1 + 1,
> +	.vrefresh = 60,
> +};

htotal*vtotal*vrefresh should be equal to clock * 1000, even in command
mode, and they should be equal to TearingEffect signal frequency.

> +
> +static inline struct s6d16d0 *panel_to_s6d16d0(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct s6d16d0, panel);
> +}
> +
> +static int s6d16d0_unprepare(struct drm_panel *panel)
> +{
> +	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");
> +		return ret;
> +	}
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret) {
> +		dev_err(s6->dev, "failed to enter sleep mode\n");
> +		return ret;
> +	}

Usually display_off should be called from _disable callback and enter
sleep mode and power off from _unprepare callback, symmetrically on
power-on path.
Moreover usually there should be some delays between these commands to
allow panel to enter appropriate states.

> +
> +	/* Assert RESET */
> +	gpiod_set_value_cansleep(s6->reset_gpio, 1);
> +	regulator_disable(s6->supply);
> +
> +	return 0;
> +}
> +
> +static int s6d16d0_prepare(struct drm_panel *panel)
> +{
> +	struct s6d16d0 *s6 = panel_to_s6d16d0(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(s6->dev);
> +	int ret;
> +
> +	ret = regulator_enable(s6->supply);
> +	if (ret) {
> +		dev_err(s6->dev, "failed to enable supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Assert RESET */
> +	gpiod_set_value_cansleep(s6->reset_gpio, 1);
> +	udelay(10);
> +	/* De-assert RESET */
> +	gpiod_set_value_cansleep(s6->reset_gpio, 0);
> +	msleep(120);
> +
> +	/* Enabe tearing mode: send TE (tearing effect) at VBLANK */
> +	ret = mipi_dsi_dcs_set_tear_on(dsi,
> +				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);

So the panel does not signal TE to DisplayController via dedicated
line/gpio. I guess then it is up to DSI-master to deliver this signal to
DisplayController? Am I right? What DSI-master driver is used with this
panel?

> +	if (ret) {
> +		dev_err(s6->dev, "failed to enble vblank TE\n");
> +		return ret;
> +	}
> +	/* Exit sleep mode and power on */
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret) {
> +		dev_err(s6->dev, "failed to exit sleep mode\n");
> +		return ret;
> +	}
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret) {
> +		dev_err(s6->dev, "failed to turn display on\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int s6d16d0_enable(struct drm_panel *panel)
> +{
> +	return 0;
> +}
> +
> +static int s6d16d0_disable(struct drm_panel *panel)
> +{
> +	return 0;
> +}
> +
> +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;
> +	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 const struct drm_panel_funcs s6d16d0_drm_funcs = {
> +	.disable = s6d16d0_disable,
> +	.unprepare = s6d16d0_unprepare,
> +	.prepare = s6d16d0_prepare,
> +	.enable = s6d16d0_enable,
> +	.get_modes = s6d16d0_get_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;
> +	mipi_dsi_set_drvdata(dsi, s6);
> +	s6->dev = dev;
> +
> +	dsi->lanes = 2;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	/*
> +	 * This display uses command mode so no MIPI_DSI_MODE_VIDEO
> +	 * or MIPI_DSI_MODE_VIDEO_SYNC_PULSE
> +	 *
> +	 * As we only send commands we do not need to be continously
> +	 * clocked.
> +	 */
> +	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 asserts RESET by default */
> +	s6->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						 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;
> +	}
> +
> +	drm_panel_init(&s6->panel);
> +	s6->panel.dev = dev;
> +	s6->panel.funcs = &s6d16d0_drm_funcs;
> +
> +	ret = drm_panel_add(&s6->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0)
> +		drm_panel_remove(&s6->panel);
> +
> +	return ret;
> +}
> +
> +static int s6d16d0_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct s6d16d0 *s6 = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&s6->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id s6d16d0_of_match[] = {
> +	{ .compatible = "samsung,s6d16d0" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, s6d16d0_of_match);
> +
> +static struct mipi_dsi_driver s6d16d0_driver = {
> +	.probe = s6d16d0_probe,
> +	.remove = s6d16d0_remove,
> +	.driver = {
> +		.name = "panel-samsung-s6d16d0",
> +		.of_match_table = s6d16d0_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(s6d16d0_driver);
> +
> +MODULE_AUTHOR("Linus Wallei <linus.walleij@linaro.org>");
> +MODULE_DESCRIPTION("MIPI-DSI s6d16d0 Panel Driver");
> +MODULE_LICENSE("GPL v2");
Linus Walleij Oct. 9, 2018, 6:46 p.m. | #2
On Tue, Oct 9, 2018 at 10:29 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 08.10.2018 12:58, Linus Walleij wrote:

> > +static const struct drm_display_mode samsung_s6d16d0_mode = {
> > +     .clock = 420160, /* HS 420160kHz, LP 19200kHz */
> > +     .hdisplay = 864,
> > +     .hsync_start = 864 + 154,
> > +     .hsync_end = 864 + 154 + 16,
> > +     .htotal = 864 + 154 + 16 + 32,
> > +     .vdisplay = 480,
> > +     .vsync_start = 480 + 1,
> > +     .vsync_end = 480 + 1 + 1,
> > +     .vtotal = 480 + 1 + 1 + 1,
> > +     .vrefresh = 60,
> > +};
>
> htotal*vtotal*vrefresh should be equal to clock * 1000, even in command
> mode, and they should be equal to TearingEffect signal frequency.

OK I had no clue. I'll look closer at this. I suspect the .clock is just
wrong and should instead be based on what you say.

Hm what about making a macro for that or something...
it seems the clock should simply be calculated from the
other values.

> > +static int s6d16d0_unprepare(struct drm_panel *panel)
> > +{
> > +     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");
> > +             return ret;
> > +     }
> > +     ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> > +     if (ret) {
> > +             dev_err(s6->dev, "failed to enter sleep mode\n");
> > +             return ret;
> > +     }
>
> Usually display_off should be called from _disable callback and enter
> sleep mode and power off from _unprepare callback, symmetrically on
> power-on path.

OK I try that.

> Moreover usually there should be some delays between these commands to
> allow panel to enter appropriate states.

There are none of these in the out-of-tree driver, and this is one of
those Samsung panels without any datasheet (known to me at least).
And it works...

> > +     /* Enabe tearing mode: send TE (tearing effect) at VBLANK */
> > +     ret = mipi_dsi_dcs_set_tear_on(dsi,
> > +                                    MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>
> So the panel does not signal TE to DisplayController via dedicated
> line/gpio.

Well there is a dedicated line, but in my case (at least) it is not
routed to a GPIO but to a dedicated line on the DSI host.

> I guess then it is up to DSI-master to deliver this signal to
> DisplayController? Am I right?

Yes this hardware has a combined display controller and three
DSI masters (also DPI output) in the same register range.
They are jotted together in a display complex.

> What DSI-master driver is used with this
> panel?

This:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/commit/?h=ux500-mcde

I am in the uncomfortable big upfront design phase that is so typical
for DRM drivers. Everything has to be built up from zero. :/

Yours,
Linus Walleij
Sam Ravnborg Oct. 9, 2018, 7:43 p.m. | #3
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
Andrzej Hajda Oct. 10, 2018, 6:37 a.m. | #4
On 09.10.2018 20:46, Linus Walleij wrote:
> On Tue, Oct 9, 2018 at 10:29 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 08.10.2018 12:58, Linus Walleij wrote:
>>> +static const struct drm_display_mode samsung_s6d16d0_mode = {
>>> +     .clock = 420160, /* HS 420160kHz, LP 19200kHz */
>>> +     .hdisplay = 864,
>>> +     .hsync_start = 864 + 154,
>>> +     .hsync_end = 864 + 154 + 16,
>>> +     .htotal = 864 + 154 + 16 + 32,
>>> +     .vdisplay = 480,
>>> +     .vsync_start = 480 + 1,
>>> +     .vsync_end = 480 + 1 + 1,
>>> +     .vtotal = 480 + 1 + 1 + 1,
>>> +     .vrefresh = 60,
>>> +};
>> htotal*vtotal*vrefresh should be equal to clock * 1000, even in command
>> mode, and they should be equal to TearingEffect signal frequency.
> OK I had no clue. I'll look closer at this. I suspect the .clock is just
> wrong and should instead be based on what you say.
>
> Hm what about making a macro for that or something...
> it seems the clock should simply be calculated from the
> other values.

In general .vrefresh is redundant, and it should be calculated from
clock and totals, otherwise rounding errors can be significant. On the
other side .vrefresh is much more readable than clock.
So maybe some initializer:
#define DRM_MODE_FROM_VM(hact, hfp, hsyn, hbp, vact, vfp, vsyn, vbp,
vrefresh) ...

would work, and will eliminate these pluses from current initializers.
Here vrefresh could be even float, as the initializer will evaluate and
convert to int in compile time.


>
>>> +static int s6d16d0_unprepare(struct drm_panel *panel)
>>> +{
>>> +     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");
>>> +             return ret;
>>> +     }
>>> +     ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
>>> +     if (ret) {
>>> +             dev_err(s6->dev, "failed to enter sleep mode\n");
>>> +             return ret;
>>> +     }
>> Usually display_off should be called from _disable callback and enter
>> sleep mode and power off from _unprepare callback, symmetrically on
>> power-on path.
> OK I try that.
>
>> Moreover usually there should be some delays between these commands to
>> allow panel to enter appropriate states.
> There are none of these in the out-of-tree driver, and this is one of
> those Samsung panels without any datasheet (known to me at least).
> And it works...

Yes, the panel is turned off so artifacts are hard to notice anyway. So
lets leave it as is.

Regards
Andrzej

>
>>> +     /* Enabe tearing mode: send TE (tearing effect) at VBLANK */
>>> +     ret = mipi_dsi_dcs_set_tear_on(dsi,
>>> +                                    MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>> So the panel does not signal TE to DisplayController via dedicated
>> line/gpio.
> Well there is a dedicated line, but in my case (at least) it is not
> routed to a GPIO but to a dedicated line on the DSI host.
>
>> I guess then it is up to DSI-master to deliver this signal to
>> DisplayController? Am I right?
> Yes this hardware has a combined display controller and three
> DSI masters (also DPI output) in the same register range.
> They are jotted together in a display complex.
>
>> What DSI-master driver is used with this
>> panel?
> This:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/commit/?h=ux500-mcde
>
> I am in the uncomfortable big upfront design phase that is so typical
> for DRM drivers. Everything has to be built up from zero. :/
>
> Yours,
> Linus Walleij
>
>
Linus Walleij Oct. 11, 2018, 9:03 a.m. | #5
Hi Sam,

thanks for your review!

I fixed most issues. Just some open small questions:

On Tue, Oct 9, 2018 at 9:43 PM Sam Ravnborg <sam@ravnborg.org> wrote:

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

OK I did those changes.

I feel a bit annoyed by DRM defining their own macros to
wrap all generic dev_* stuff from the kernel, like it is extra
special or something, but the subsystem can have it
the way it wants, I thin networking does the same.

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

What I am trying to achieve is that all drivers just state 1
for "asserted" whether that means active low or not.

Sadly the kernel and device tree bindings are full of bad
examples where people encode the physical high/low inside
the driver and I am only slowly cleaning this up.

I try to set a good example...


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

It can and then it bails out here and retries later.

I don't quite get the question, sorry.

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

-EPROBE_DEFER is only tested to not produce error prints
unnecessarily. In the regulator case, the regulator core already
displays an error.

Yours,
Linus Walleij
Sam Ravnborg Oct. 11, 2018, 11:37 a.m. | #6
> > > +     if (IS_ERR(s6->supply))
> > > +             return PTR_ERR(s6->supply);
> >
> > This cannot be -EPROBE_DEFER?
> 
> It can and then it bails out here and retries later.
> 
> I don't quite get the question, sorry.
> 
> > > +     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..
> 
> -EPROBE_DEFER is only tested to not produce error prints
> unnecessarily. In the regulator case, the regulator core already
> displays an error.
This was what I missed above (that we only check -EPROBE_DEFER to avoid printing).
So my question was just me confusing things.

Thanks for the explanation.

	Sam

Patch

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
+	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>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+
+struct s6d16d0 {
+	struct device *dev;
+	struct drm_panel panel;
+	struct regulator *supply;
+	struct gpio_desc *reset_gpio;
+};
+
+/*
+ * The timings are not very helpful as the display is used in
+ * command mode.
+ */
+static const struct drm_display_mode samsung_s6d16d0_mode = {
+	.clock = 420160, /* HS 420160kHz, LP 19200kHz */
+	.hdisplay = 864,
+	.hsync_start = 864 + 154,
+	.hsync_end = 864 + 154 + 16,
+	.htotal = 864 + 154 + 16 + 32,
+	.vdisplay = 480,
+	.vsync_start = 480 + 1,
+	.vsync_end = 480 + 1 + 1,
+	.vtotal = 480 + 1 + 1 + 1,
+	.vrefresh = 60,
+};
+
+static inline struct s6d16d0 *panel_to_s6d16d0(struct drm_panel *panel)
+{
+	return container_of(panel, struct s6d16d0, panel);
+}
+
+static int s6d16d0_unprepare(struct drm_panel *panel)
+{
+	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");
+		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);
+	regulator_disable(s6->supply);
+
+	return 0;
+}
+
+static int s6d16d0_prepare(struct drm_panel *panel)
+{
+	struct s6d16d0 *s6 = panel_to_s6d16d0(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(s6->dev);
+	int ret;
+
+	ret = regulator_enable(s6->supply);
+	if (ret) {
+		dev_err(s6->dev, "failed to enable supply: %d\n", ret);
+		return ret;
+	}
+
+	/* Assert RESET */
+	gpiod_set_value_cansleep(s6->reset_gpio, 1);
+	udelay(10);
+	/* De-assert RESET */
+	gpiod_set_value_cansleep(s6->reset_gpio, 0);
+	msleep(120);
+
+	/* Enabe tearing mode: send TE (tearing effect) at VBLANK */
+	ret = mipi_dsi_dcs_set_tear_on(dsi,
+				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+	if (ret) {
+		dev_err(s6->dev, "failed to enble vblank TE\n");
+		return ret;
+	}
+	/* Exit sleep mode and power on */
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret) {
+		dev_err(s6->dev, "failed to exit sleep mode\n");
+		return ret;
+	}
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret) {
+		dev_err(s6->dev, "failed to turn display on\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int s6d16d0_enable(struct drm_panel *panel)
+{
+	return 0;
+}
+
+static int s6d16d0_disable(struct drm_panel *panel)
+{
+	return 0;
+}
+
+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;
+	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 const struct drm_panel_funcs s6d16d0_drm_funcs = {
+	.disable = s6d16d0_disable,
+	.unprepare = s6d16d0_unprepare,
+	.prepare = s6d16d0_prepare,
+	.enable = s6d16d0_enable,
+	.get_modes = s6d16d0_get_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;
+	mipi_dsi_set_drvdata(dsi, s6);
+	s6->dev = dev;
+
+	dsi->lanes = 2;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	/*
+	 * This display uses command mode so no MIPI_DSI_MODE_VIDEO
+	 * or MIPI_DSI_MODE_VIDEO_SYNC_PULSE
+	 *
+	 * As we only send commands we do not need to be continously
+	 * clocked.
+	 */
+	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 asserts RESET by default */
+	s6->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						 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;
+	}
+
+	drm_panel_init(&s6->panel);
+	s6->panel.dev = dev;
+	s6->panel.funcs = &s6d16d0_drm_funcs;
+
+	ret = drm_panel_add(&s6->panel);
+	if (ret < 0)
+		return ret;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0)
+		drm_panel_remove(&s6->panel);
+
+	return ret;
+}
+
+static int s6d16d0_remove(struct mipi_dsi_device *dsi)
+{
+	struct s6d16d0 *s6 = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&s6->panel);
+
+	return 0;
+}
+
+static const struct of_device_id s6d16d0_of_match[] = {
+	{ .compatible = "samsung,s6d16d0" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, s6d16d0_of_match);
+
+static struct mipi_dsi_driver s6d16d0_driver = {
+	.probe = s6d16d0_probe,
+	.remove = s6d16d0_remove,
+	.driver = {
+		.name = "panel-samsung-s6d16d0",
+		.of_match_table = s6d16d0_of_match,
+	},
+};
+module_mipi_dsi_driver(s6d16d0_driver);
+
+MODULE_AUTHOR("Linus Wallei <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("MIPI-DSI s6d16d0 Panel Driver");
+MODULE_LICENSE("GPL v2");