| Message ID | 20181008105852.24196-2-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] drm/panel: Add DT bindings for Samsung S6D16D0 | expand |
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");
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
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
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 > >
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
> > > + 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
Hi Andrzej, I was just looking at this (which confused me and was not fixed in v2): On Tue, Oct 9, 2018 at 8:46 PM Linus Walleij <linus.walleij@linaro.org> 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. Isn't the case such that the clock we use which will be =< hs_rate or lp_rate respectively, controls vrefresh? So actually, the sync cycles are constant, the clock comes from (usually) the best fit from the clock framework, and then vrefresh depends on what frequency we get from there, and it will be considerably lower in lp mode than in hs mode? It feels a bit silly to return two modes for the display that are going to be identical depending on whether hs or lp is used, but in some sense it would be the right thing, but I think then the display driver needs to be smart enough to identify that "OK now KMS is asking for HS mode" and "OK now KMA is asking for LP mode". Further, switching the display into the "LP mode" should probably have effects on the host other than just switching to the LP clock. I think due to the crudeness of the frameworks graphics of course comes out as it should but if userspace would do something creative like starting to inspect the vrefresh it gets and try to switch modes depending on that, I guess we have a bit of a problem. I guess currently userspace can't choose between HS or LP mode anyway, but I guess we should support that. I've changed this to clocking matching the HS mode right now but I feel a bit uncertain as to what the real fix is :/ Yours, Linus Walleij
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");
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