Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

2023-04-22 Thread Artur Weber
Hi,

thank you for the review.

On 20/04/2023 09:35, Linus Walleij wrote:
>> +static int s6d7aa0_on(struct s6d7aa0 *ctx)
>> +{
>> +   struct mipi_dsi_device *dsi = ctx->dsi;
>> +   struct device *dev = &dsi->dev;
>> +   int ret;
>> +
>> +   dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> 
> (...)
> 
>> +static int s6d7aa0_off(struct s6d7aa0 *ctx)
>> +{
>> +   struct mipi_dsi_device *dsi = ctx->dsi;
>> +   struct device *dev = &dsi->dev;
>> +   int ret;
>> +
>> +   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> 
> I haven't seen this mode flag MIPI_DSI_MODE_LPM set and
> masked in other DSI panel drivers! Is this something we should
> fix everywhere then? Or even something the core should be
> doing?

These bits were included in a driver for a similar panel with the same
controller in an MSM8916 close-to-mainline kernel fork[1]; that driver
was generated with lmdpdg[2], which adds the LPM mode flag automatically
based on some downstream DTS property. In this case, I left it in, since
it didn't seem to break anything... but I just re-tested without it and
it seems that it might've fixed some odd issues I'd get sometimes when
going out of sleep mode. I'll get rid of it in the next version.

(I based my panel driver off that driver; now that I think about it, it
might be worth mentioning somewhere in the copyright notice...?)

Best regards
Artur Weber

[1]
https://github.com/msm8916-mainline/linux/blob/msm8916/6.3-rc7/drivers/gpu/drm/panel/msm8916-generated/panel-samsung-s6d7aa0-lsl080al03.c
[2]
https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator


Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

2023-04-20 Thread Linus Walleij
Hi Artur,

thanks for your patch!

On Sun, Apr 16, 2023 at 3:16 PM Artur Weber  wrote:

> Initial driver for S6D7AA0-controlled panels, currently only for the
> LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family of tablets.
>
> It should be possible to extend this driver to work with other panels
> using this IC.
>
> Signed-off-by: Artur Weber 
> ---
> Changed in v2:
>  - Removed unused panel_name property from desc struct

Overall this driver looks very good. I could merge it once the DT bindings
are ACKed by the DT maintainers and some minor stuff fixed.

Some comments below:

> +/* Manufacturer command set */
> +#define CMD_BL_CTL 0xc3
> +#define CMD_OTP_RELOAD 0xd0
> +#define CMD_PASSWD10xf0
> +#define CMD_PASSWD20xf1
> +#define CMD_PASSWD30xfc

Some drivers prefix these commands with "MCS" such as
MCS_BL_CTL.

MCS = Manufacturer Command Set (I think)

Some just name the identifers after the panel such as
s6d27a1 which has S6D27A1_RESCTL etc.

CMD seems a bit general to me and may be mistaken for
the actual DCS commands.

> +struct s6d7aa0 {
> +   struct drm_panel panel;
> +   struct mipi_dsi_device *dsi;
> +   struct gpio_desc *reset_gpio;
> +   struct regulator *enable_supply;
> +   const struct s6d7aa0_panel_desc *desc;
> +   bool prepared;

Skip this state variable, the core keeps track of whether the
panel is enabled or not.

> +static void s6d7aa0_reset(struct s6d7aa0 *ctx)
> +{
> +   gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +   msleep(50);

This first de-assertion is unnecessary isn't it?

The reset line will just be asserted longer if it is already asserted.

> +   gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +   msleep(50);
> +   gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +   msleep(50);
> +}

(...)

> +static int s6d7aa0_on(struct s6d7aa0 *ctx)
> +{
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +   struct device *dev = &dsi->dev;
> +   int ret;
> +
> +   dsi->mode_flags |= MIPI_DSI_MODE_LPM;

(...)

> +static int s6d7aa0_off(struct s6d7aa0 *ctx)
> +{
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +   struct device *dev = &dsi->dev;
> +   int ret;
> +
> +   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

I haven't seen this mode flag MIPI_DSI_MODE_LPM set and
masked in other DSI panel drivers! Is this something we should
fix everywhere then? Or even something the core should be
doing?

Yours,
Linus Walleij


[PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

2023-04-17 Thread Artur Weber
Initial driver for S6D7AA0-controlled panels, currently only for the
LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family of tablets.

It should be possible to extend this driver to work with other panels
using this IC.

Signed-off-by: Artur Weber 
---
Changed in v2:
 - Removed unused panel_name property from desc struct
---
 drivers/gpu/drm/panel/Kconfig |   7 +
 drivers/gpu/drm/panel/Makefile|   1 +
 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c | 395 ++
 3 files changed, 403 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 871c..2ce9d1a45625 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -532,6 +532,13 @@ config DRM_PANEL_SAMSUNG_S6D27A1
  This panel can be found in Samsung Galaxy Ace 2
  GT-I8160 mobile phone.
 
+config DRM_PANEL_SAMSUNG_S6D7AA0
+   tristate "Samsung S6D7AA0 MIPI-DSI video mode panel controller"
+   depends on OF
+   depends on BACKLIGHT_CLASS_DEVICE
+   select DRM_MIPI_DSI
+   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 c05aa9e23907..193f3067865d 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_DB7430) += 
panel-samsung-db7430.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_S6D27A1) += panel-samsung-s6d27a1.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D7AA0) += panel-samsung-s6d7aa0.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_S6E63M0) += panel-samsung-s6e63m0.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c 
b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
new file mode 100644
index ..408e4082de74
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Samsung S6D7AA0 MIPI-DSI TFT LCD controller drm_panel driver.
+ *
+ * Copyright (C) 2022 Artur Weber 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+/* Manufacturer command set */
+#define CMD_BL_CTL 0xc3
+#define CMD_OTP_RELOAD 0xd0
+#define CMD_PASSWD10xf0
+#define CMD_PASSWD20xf1
+#define CMD_PASSWD30xfc
+
+struct s6d7aa0 {
+   struct drm_panel panel;
+   struct mipi_dsi_device *dsi;
+   struct gpio_desc *reset_gpio;
+   struct regulator *enable_supply;
+   const struct s6d7aa0_panel_desc *desc;
+   bool prepared;
+};
+
+struct s6d7aa0_panel_desc {
+   int (*init_func)(struct s6d7aa0 *ctx);
+   const struct drm_display_mode drm_mode;
+   unsigned long mode_flags;
+   u32 bus_flags;
+   bool use_passwd3;
+};
+
+static inline struct s6d7aa0 *panel_to_s6d7aa0(struct drm_panel *panel)
+{
+   return container_of(panel, struct s6d7aa0, panel);
+}
+
+static void s6d7aa0_reset(struct s6d7aa0 *ctx)
+{
+   gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+   msleep(50);
+   gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+   msleep(50);
+   gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+   msleep(50);
+}
+
+static int s6d7aa0_lock(struct s6d7aa0 *ctx, bool lock)
+{
+   struct mipi_dsi_device *dsi = ctx->dsi;
+
+   if (lock) {
+   mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD1, 0xa5, 0xa5);
+   mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD2, 0xa5, 0xa5);
+   if (ctx->desc->use_passwd3)
+   mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD3, 0x5a, 0x5a);
+   } else {
+   mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD1, 0x5a, 0x5a);
+   mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD2, 0x5a, 0x5a);
+   if (ctx->desc->use_passwd3)
+   mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD3, 0xa5, 0xa5);
+   }
+
+   return 0;
+}
+
+static int s6d7aa0_bl_ctl_on(struct s6d7aa0 *ctx, bool enable)
+{
+   struct mipi_dsi_device *dsi = ctx->dsi;
+
+   if (enable)
+   mipi_dsi_dcs_write_seq(dsi, CMD_BL_CTL, 0x40, 0x00, 0x28);
+   else
+   mipi_dsi_dcs_write_seq(dsi, CMD_BL_CTL, 0x40, 0x00, 0x20);
+
+   return 0;
+}
+
+static int s6d7aa0_on(struct s6d7aa0 *ctx)
+{
+   struct mipi_dsi_device *dsi = ctx->dsi;
+   struct device *dev = &dsi->dev;
+   int ret;
+
+   dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+   s6d7aa0_reset(ctx);
+
+   ret = ctx->desc->init_func(ctx);
+   if (ret < 0) {
+