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