[PATCH 0/2] Add driver for Sharp Memory LCD

[PATCH 0/2] Add driver for Sharp Memory LCD

[Alex Lanzano]

This patch series add support for the monochrome Sharp Memory LCD
panels. This series is based off of the work done by Mehdi Djait.

References:
https://lore.kernel.org/dri-devel/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/
https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf

Alex Lanzano (2):
  dt-bindings: display: Add Sharp Memory LCD bindings
  drm/tiny: Add driver for Sharp Memory LCD

 .../bindings/display/sharp,sharp-memory.yaml  |  97 +++
 MAINTAINERS                                   |   8 +
 drivers/gpu/drm/tiny/Kconfig                  |  20 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/sharp-memory.c           | 741 ++++++++++++++++++
 include/dt-bindings/display/sharp-memory.h    |   9 +
 6 files changed, 876 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
 create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
 create mode 100644 include/dt-bindings/display/sharp-memory.h

-- 
2.45.2

[PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

Add device tree bindings for the monochrome Sharp Memory LCD

Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
---
 .../bindings/display/sharp,sharp-memory.yaml  | 97 +++++++++++++++++++
 include/dt-bindings/display/sharp-memory.h    |  9 ++
 2 files changed, 106 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
 create mode 100644 include/dt-bindings/display/sharp-memory.h

diff --git a/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
new file mode 100644
index 000000000000..a79edd97c857
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sharp,sharp-memory.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sharp Memory LCD panels
+
+maintainers:
+  - Alex Lanzano <lanzano.alex@gmail.com>
+
+description:
+  This binding is for the Sharp Memory LCD monochrome displays.
+
+properties:
+  compatible:
+    enum:
+      - sharp,ls010b7dh04
+      - sharp,ls011b7dh03
+      - sharp,ls012b7dd01
+      - sharp,ls013b7dh03
+      - sharp,ls013b7dh05
+      - sharp,ls018b7dh02
+      - sharp,ls027b7dh01
+      - sharp,ls027b7dh01a
+      - sharp,ls032b7dd02
+      - sharp,ls044q7dh01
+
+  reg:
+    maxItems: 1
+
+  spi-cs-high: true
+
+  spi-max-frequency:
+    maximum: 2000000
+
+  vcom-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      VCOM is a signal that prevents DC bias from being built up in
+      the panel resulting in pixels being forever stuck in one state.
+      vcom-mode can be set to one of three modes
+
+      SHARP_MEMORY_SOFTWARE_VCOM - This method uses a kthread to periodically send a
+      "maintain display" message to the display, toggling the vcom
+      bit on and off with each message
+
+      SHARP_MEMORY_EXTERNAL_VCOM - This method relies on an external clock to generate
+      the signal on the EXTCOMM pin
+
+      SHARP_MEMORY_PWM_VCOM - This method uses a pwm device to generate the signal
+      on the EXTCOMM pin
+
+    enum: [ 0, 1, 2 ]
+
+  enable-gpios:
+    maxItems: 1
+    description: Enables display
+
+  pwms:
+    maxItems: 1
+    description: External VCOM signal
+
+required:
+  - compatible
+  - reg
+  - spi-cs-high
+  - vcom-mode
+
+if:
+  properties:
+    vcom-mode:
+      # SHARP_MEMORY_PWM_VCOM
+      enum: [2]
+then:
+  required:
+    - pwms
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/display/sharp-memory.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            display@0{
+                    compatible = "sharp,ls013b7dh03";
+                    reg = <0>;
+                    spi-cs-high;
+                    spi-max-frequency = <1000000>;
+                    vcom-mode = <SHARP_MEMORY_SOFTWARE_VCOM>;
+            };
+    };
+...
diff --git a/include/dt-bindings/display/sharp-memory.h b/include/dt-bindings/display/sharp-memory.h
new file mode 100644
index 000000000000..dea14c7bd7ec
--- /dev/null
+++ b/include/dt-bindings/display/sharp-memory.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+#ifndef _DT_BINDINGS_SHARP_MEMORY
+#define _DT_BINDINGS_SHARP_MEMORY
+
+#define SHARP_MEMORY_SOFTWARE_VCOM	0
+#define SHARP_MEMORY_EXTERNAL_VCOM	1
+#define SHARP_MEMORY_PWM_VCOM		2
+
+#endif
-- 
2.45.2

[PATCH 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

Add support for the monochrome Sharp Memory LCDs.

Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
---
 MAINTAINERS                         |   8 +
 drivers/gpu/drm/tiny/Kconfig        |  20 +
 drivers/gpu/drm/tiny/Makefile       |   1 +
 drivers/gpu/drm/tiny/sharp-memory.c | 742 ++++++++++++++++++++++++++++
 4 files changed, 771 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 71b739b40921..d19893d25913 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7123,6 +7123,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
 F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
 
+DRM DRIVER FOR SHARP MEMORY LCD
+M:	Alex Lanzano <lanzano.alex@gmail.com>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
+F:	drivers/gpu/drm/tiny/sharp-memory.c
+F:	include/dt-bindings/display/sharp-memory.h
+
 DRM DRIVER FOR SITRONIX ST7586 PANELS
 M:	David Lechner <david@lechnology.com>
 S:	Maintained
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649bc1..bc386954faa2 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -186,6 +186,26 @@ config TINYDRM_REPAPER
 
 	  If M is selected the module will be called repaper.
 
+config TINYDRM_SHARP_MEMORY
+	tristate "DRM support for Sharp Memory LCD panels"
+	depends on DRM && SPI
+	select DRM_GEM_DMA_HELPER
+	select DRM_KMS_HELPER
+	help
+	  DRM Driver for the following Sharp Memory Panels:
+	  * 1.00" Sharp Memory LCD (LS010B7DH04)
+	  * 1.10" Sharp Memory LCD (LS011B7DH03)
+	  * 1.20" Sharp Memory LCD (LS012B7DD01)
+	  * 1.28" Sharp Memory LCD (LS013B7DH03)
+	  * 1.26" Sharp Memory LCD (LS013B7DH05)
+	  * 1.80" Sharp Memory LCD (LS018B7DH02)
+	  * 2.70" Sharp Memory LCD (LS027B7DH01)
+	  * 2.70" Sharp Memory LCD (LS027B7DH01A)
+	  * 3.20" Sharp Memory LCD (LS032B7DD02)
+	  * 4.40" Sharp Memory LCD (LS044Q7DH01)
+
+	  If M is selected the module will be called sharp_memory.
+
 config TINYDRM_ST7586
 	tristate "DRM support for Sitronix ST7586 display panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a044b..4aaf56f8707d 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -14,5 +14,6 @@ obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
 obj-$(CONFIG_TINYDRM_ILI9486)		+= ili9486.o
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
+obj-$(CONFIG_TINYDRM_SHARP_MEMORY)	+= sharp-memory.o
 obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
 obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
new file mode 100644
index 000000000000..5e61e348ce3a
--- /dev/null
+++ b/drivers/gpu/drm/tiny/sharp-memory.c
@@ -0,0 +1,742 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/spi/spi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/kthread.h>
+#include <linux/bitrev.h>
+#include <linux/pwm.h>
+#include <linux/mutex.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_dma_helper.h>
+#include <drm/drm_fbdev_dma.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_rect.h>
+#include <drm/drm_probe_helper.h>
+
+#include <dt-bindings/display/sharp-memory.h>
+
+#define SHARP_MODE_PERIOD 8
+#define SHARP_ADDR_PERIOD 8
+#define SHARP_DUMMY_PERIOD 8
+
+#define SHARP_MEMORY_DISPLAY_MAINTAIN_MODE 0
+#define SHARP_MEMORY_DISPLAY_UPDATE_MODE 1
+#define SHARP_MEMORY_DISPLAY_CLEAR_MODE 4
+
+enum sharp_memory_model {
+	LS010B7DH04 = 1,
+	LS011B7DH03,
+	LS012B7DD01,
+	LS013B7DH03,
+	LS013B7DH05,
+	LS018B7DH02,
+	LS027B7DH01,
+	LS027B7DH01A,
+	LS032B7DD02,
+	LS044Q7DH01,
+};
+
+struct sharp_memory_device {
+	struct drm_device drm;
+	struct spi_device *spi;
+
+	const struct drm_display_mode *mode;
+
+	struct drm_crtc crtc;
+	struct drm_plane plane;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+
+	struct gpio_desc *enable_gpio;
+
+	struct task_struct *sw_vcom_signal;
+	struct pwm_device *pwm_vcom_signal;
+
+	uint32_t vcom_mode;
+	uint8_t vcom;
+
+	uint32_t pitch;
+	uint32_t tx_buffer_size;
+	uint8_t *tx_buffer;
+	struct mutex tx_mutex;
+};
+
+static inline int sharp_memory_spi_write(struct spi_device *spi, void *buf, size_t len)
+{
+	/* Reverse the bit order */
+	for (uint8_t *b = buf; b < ((uint8_t *)buf) + len; ++b)
+		*b = bitrev8(*b);
+
+	return spi_write(spi, buf, len);
+}
+
+static inline struct sharp_memory_device *drm_to_sharp_memory_device(struct drm_device *drm)
+{
+	return container_of(drm, struct sharp_memory_device, drm);
+}
+
+DEFINE_DRM_GEM_DMA_FOPS(sharp_memory_fops);
+
+static const struct drm_driver sharp_memory_drm_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops			= &sharp_memory_fops,
+	DRM_GEM_DMA_DRIVER_OPS_VMAP,
+	.name			= "sharp_memory_display",
+	.desc			= "Sharp Display Memory LCD",
+	.date			= "20231129",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static inline void sharp_memory_set_tx_buffer_mode(uint8_t *buffer, uint8_t mode, uint8_t vcom)
+{
+
+	*buffer = mode | (vcom << 1);
+}
+
+static inline void sharp_memory_set_tx_buffer_addresses(uint8_t *buffer,
+							struct drm_rect clip,
+							uint32_t pitch)
+{
+	for (uint32_t line = 0; line < clip.y2; ++line)
+		buffer[line * pitch] = line + 1;
+
+}
+
+static void sharp_memory_set_tx_buffer_data(uint8_t *buffer,
+					    struct drm_framebuffer *fb,
+					    struct drm_rect clip,
+					    uint32_t pitch,
+					    struct drm_format_conv_state *fmtcnv_state)
+{
+	int ret;
+	struct iosys_map dst, vmap;
+	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		return;
+
+
+	iosys_map_set_vaddr(&dst, buffer);
+	iosys_map_set_vaddr(&vmap, dma_obj->vaddr);
+
+	drm_fb_xrgb8888_to_mono(&dst, &pitch, &vmap, fb, &clip, fmtcnv_state);
+
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+}
+
+static int sharp_memory_update_display(struct sharp_memory_device *smd,
+				       struct drm_framebuffer *fb,
+				       struct drm_rect clip,
+				       struct drm_format_conv_state *fmtcnv_state)
+{
+	int ret;
+	uint32_t pitch = smd->pitch;
+	uint8_t vcom = smd->vcom;
+	uint8_t *tx_buffer = smd->tx_buffer;
+	uint32_t tx_buffer_size = smd->tx_buffer_size;
+
+	mutex_lock(&smd->tx_mutex);
+
+	/* Populate the transmit buffer with frame data */
+	sharp_memory_set_tx_buffer_mode(&tx_buffer[0],
+					SHARP_MEMORY_DISPLAY_UPDATE_MODE, vcom);
+	sharp_memory_set_tx_buffer_addresses(&tx_buffer[1], clip, pitch);
+	sharp_memory_set_tx_buffer_data(&tx_buffer[2], fb, clip, pitch, fmtcnv_state);
+
+	ret = sharp_memory_spi_write(smd->spi, tx_buffer, tx_buffer_size);
+
+	mutex_unlock(&smd->tx_mutex);
+
+	return ret;
+}
+
+static int sharp_memory_maintain_display(struct sharp_memory_device *smd)
+{
+	int ret;
+	uint8_t vcom = smd->vcom;
+	uint8_t *tx_buffer = smd->tx_buffer;
+
+	mutex_lock(&smd->tx_mutex);
+
+	sharp_memory_set_tx_buffer_mode(&tx_buffer[0], SHARP_MEMORY_DISPLAY_MAINTAIN_MODE, vcom);
+	tx_buffer[1] = 0; // Write dummy data
+	ret = sharp_memory_spi_write(smd->spi, tx_buffer, 2);
+
+	mutex_unlock(&smd->tx_mutex);
+
+	return ret;
+}
+
+static int sharp_memory_clear_display(struct sharp_memory_device *smd)
+{
+	int ret;
+	uint8_t vcom = smd->vcom;
+	uint8_t *tx_buffer = smd->tx_buffer;
+
+	mutex_lock(&smd->tx_mutex);
+
+	sharp_memory_set_tx_buffer_mode(&tx_buffer[0], SHARP_MEMORY_DISPLAY_CLEAR_MODE, vcom);
+	tx_buffer[1] = 0; // write dummy data
+	ret = sharp_memory_spi_write(smd->spi, tx_buffer, 2);
+
+	mutex_unlock(&smd->tx_mutex);
+
+	return ret;
+}
+
+static void sharp_memory_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect,
+				  struct drm_format_conv_state *fmtconv_state)
+{
+	struct drm_rect clip;
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(fb->dev);
+
+	/* Always update a full line regardless of what is dirty */
+	clip.x1 = 0;
+	clip.x2 = fb->width;
+	clip.y1 = rect->y1;
+	clip.y2 = rect->y2;
+
+	sharp_memory_update_display(smd, fb, clip, fmtconv_state);
+}
+
+static int sharp_memory_plane_atomic_check(struct drm_plane *plane,
+					   struct drm_atomic_state *state)
+{
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct sharp_memory_device *smd;
+	struct drm_crtc_state *crtc_state;
+
+	smd = container_of(plane, struct sharp_memory_device, plane);
+	crtc_state = drm_atomic_get_new_crtc_state(state, &smd->crtc);
+
+	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
+
+static void sharp_memory_plane_atomic_update(struct drm_plane *plane,
+					     struct drm_atomic_state *state)
+{
+
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
+	struct sharp_memory_device *smd;
+	struct drm_rect rect;
+
+	smd = container_of(plane, struct sharp_memory_device, plane);
+	if (!smd->crtc.state->active)
+		return;
+
+
+	if (drm_atomic_helper_damage_merged(old_state, plane_state, &rect))
+		sharp_memory_fb_dirty(plane_state->fb, &rect, &fmtcnv_state);
+
+	drm_format_conv_state_release(&fmtcnv_state);
+}
+
+static const struct drm_plane_helper_funcs sharp_memory_plane_helper_funcs = {
+	.prepare_fb = drm_gem_plane_helper_prepare_fb,
+	.atomic_check = sharp_memory_plane_atomic_check,
+	.atomic_update = sharp_memory_plane_atomic_update,
+};
+
+static bool sharp_memory_format_mod_supported(struct drm_plane *plane,
+					      uint32_t format,
+					      uint64_t modifier)
+{
+	return modifier == DRM_FORMAT_MOD_LINEAR;
+}
+
+static const struct drm_plane_funcs sharp_memory_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	.reset = drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+	.format_mod_supported = sharp_memory_format_mod_supported,
+};
+
+static enum drm_mode_status sharp_memory_crtc_mode_valid(struct drm_crtc *crtc,
+							 const struct drm_display_mode *mode)
+{
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, smd->mode);
+}
+
+static int sharp_memory_crtc_check(struct drm_crtc *crtc,
+				   struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	int ret;
+
+	if (!crtc_state->enable)
+		goto out;
+
+	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+	if (ret)
+		return ret;
+
+out:
+	return drm_atomic_add_affected_planes(state, crtc);
+}
+
+static int sharp_memory_sw_vcom_signal_thread(void *data)
+{
+	struct sharp_memory_device *smd = data;
+
+	while (!kthread_should_stop()) {
+		smd->vcom ^= 1; // Toggle vcom
+		sharp_memory_maintain_display(smd);
+		msleep(1000);
+	}
+
+	return 0;
+}
+
+static void sharp_memory_crtc_enable(struct drm_crtc *crtc,
+				     struct drm_atomic_state *state)
+{
+	struct pwm_state pwm_state;
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
+
+	sharp_memory_clear_display(smd);
+
+	if (smd->enable_gpio)
+		gpiod_set_value(smd->enable_gpio, 1);
+
+
+	switch (smd->vcom_mode) {
+	case SHARP_MEMORY_SOFTWARE_VCOM:
+		smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
+						  smd, "sw_vcom_signal");
+		break;
+
+	case SHARP_MEMORY_EXTERNAL_VCOM:
+		break;
+
+	case SHARP_MEMORY_PWM_VCOM:
+		pwm_get_state(smd->pwm_vcom_signal, &pwm_state);
+		pwm_state.period =    1000000000;
+		pwm_state.duty_cycle = 100000000;
+		pwm_state.enabled = true;
+		pwm_apply_state(smd->pwm_vcom_signal, &pwm_state);
+		break;
+	}
+}
+
+static void sharp_memory_crtc_disable(struct drm_crtc *crtc,
+				      struct drm_atomic_state *state)
+{
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
+
+	sharp_memory_clear_display(smd);
+
+	if (smd->enable_gpio)
+		gpiod_set_value(smd->enable_gpio, 0);
+
+
+	switch (smd->vcom_mode) {
+	case SHARP_MEMORY_SOFTWARE_VCOM:
+		kthread_stop(smd->sw_vcom_signal);
+		break;
+
+	case SHARP_MEMORY_EXTERNAL_VCOM:
+		break;
+
+	case SHARP_MEMORY_PWM_VCOM:
+		pwm_disable(smd->pwm_vcom_signal);
+		break;
+	}
+}
+
+static const struct drm_crtc_helper_funcs sharp_memory_crtc_helper_funcs = {
+	.mode_valid = sharp_memory_crtc_mode_valid,
+	.atomic_check = sharp_memory_crtc_check,
+	.atomic_enable = sharp_memory_crtc_enable,
+	.atomic_disable = sharp_memory_crtc_disable,
+};
+
+static const struct drm_crtc_funcs sharp_memory_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs sharp_memory_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static int sharp_memory_connector_get_modes(struct drm_connector *connector)
+{
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(connector->dev);
+
+	return drm_connector_helper_get_modes_fixed(connector, smd->mode);
+}
+
+static const struct drm_connector_helper_funcs sharp_memory_connector_hfuncs = {
+	.get_modes = sharp_memory_connector_get_modes,
+};
+
+static const struct drm_connector_funcs sharp_memory_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+
+};
+
+static const struct drm_mode_config_funcs sharp_memory_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct spi_device_id sharp_memory_ids[] = {
+	{"ls010b7dh04", LS010B7DH04},
+	{"ls011b7dh03", LS011B7DH03},
+	{"ls012b7dd01", LS012B7DD01},
+	{"ls013b7dh03", LS013B7DH03},
+	{"ls013b7dh05", LS013B7DH05},
+	{"ls018b7dh02", LS018B7DH02},
+	{"ls027b7dh01", LS027B7DH01},
+	{"ls027b7dh01a", LS027B7DH01A},
+	{"ls032b7dd02", LS032B7DD02},
+	{"ls044q7dh01", LS044Q7DH01},
+	{},
+};
+MODULE_DEVICE_TABLE(spi, sharp_memory_ids);
+
+static const struct of_device_id sharp_memory_of_match[] = {
+	{.compatible = "sharp,ls010b7dh04"},
+	{.compatible = "sharp,ls011b7dh03"},
+	{.compatible = "sharp,ls012b7dd01"},
+	{.compatible = "sharp,ls013b7dh03"},
+	{.compatible = "sharp,ls013b7dh05"},
+	{.compatible = "sharp,ls018b7dh02"},
+	{.compatible = "sharp,ls027b7dh01"},
+	{.compatible = "sharp,ls027b7dh01a"},
+	{.compatible = "sharp,ls032b7dd02"},
+	{.compatible = "sharp,ls044q7dh01"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, sharp_memory_of_match);
+
+static const struct drm_display_mode sharp_memory_ls010b7dh04_mode = {
+	DRM_SIMPLE_MODE(128, 128, 18, 18),
+};
+
+static const struct drm_display_mode sharp_memory_ls011b7dh03_mode = {
+	DRM_SIMPLE_MODE(160, 68, 25, 10),
+};
+
+static const struct drm_display_mode sharp_memory_ls012b7dd01_mode = {
+	DRM_SIMPLE_MODE(184, 38, 29, 6),
+};
+
+static const struct drm_display_mode sharp_memory_ls013b7dh03_mode = {
+	DRM_SIMPLE_MODE(128, 128, 23, 23),
+};
+
+static const struct drm_display_mode sharp_memory_ls013b7dh05_mode = {
+	DRM_SIMPLE_MODE(144, 168, 20, 24),
+};
+
+static const struct drm_display_mode sharp_memory_ls018b7dh02_mode = {
+	DRM_SIMPLE_MODE(230, 303, 27, 36),
+};
+
+static const struct drm_display_mode sharp_memory_ls027b7dh01_mode = {
+	DRM_SIMPLE_MODE(400, 240, 58, 35),
+};
+
+static const struct drm_display_mode sharp_memory_ls032b7dd02_mode = {
+	DRM_SIMPLE_MODE(336, 536, 42, 68),
+};
+
+static const struct drm_display_mode sharp_memory_ls044q7dh01_mode = {
+	DRM_SIMPLE_MODE(320, 240, 89, 67),
+};
+
+static const uint32_t sharp_memory_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+static inline enum sharp_memory_model sharp_memory_model_from_device_id(struct spi_device *spi)
+{
+	const struct spi_device_id *spi_id = spi_get_device_id(spi);
+
+	return (enum sharp_memory_model)spi_id->driver_data;
+}
+
+static int sharp_memory_pipe_init(struct drm_device *dev,
+				  struct sharp_memory_device *smd,
+				  const uint32_t *formats, unsigned int format_count,
+				  const uint64_t *format_modifiers)
+{
+	int ret;
+	struct drm_encoder *encoder = &smd->encoder;
+	struct drm_plane *plane = &smd->plane;
+	struct drm_crtc *crtc = &smd->crtc;
+	struct drm_connector *connector = &smd->connector;
+
+	drm_plane_helper_add(plane, &sharp_memory_plane_helper_funcs);
+	ret = drm_universal_plane_init(dev, plane, 0,
+				       &sharp_memory_plane_funcs,
+				       formats, format_count,
+				       format_modifiers,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+
+
+	drm_crtc_helper_add(crtc, &sharp_memory_crtc_helper_funcs);
+	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
+					&sharp_memory_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+
+
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+	ret = drm_encoder_init(dev, encoder, &sharp_memory_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ret;
+
+
+	ret = drm_connector_init(&smd->drm, &smd->connector,
+				 &sharp_memory_connector_funcs,
+				 DRM_MODE_CONNECTOR_SPI);
+	if (ret)
+		return ret;
+
+
+	drm_connector_helper_add(&smd->connector,
+				 &sharp_memory_connector_hfuncs);
+
+	return drm_connector_attach_encoder(connector, encoder);
+}
+
+static int sharp_memory_init_pwm_vcom_signal(struct sharp_memory_device *smd)
+{
+	struct pwm_state state;
+	struct device *dev = &smd->spi->dev;
+
+	smd->pwm_vcom_signal = devm_pwm_get(dev, NULL);
+	if (IS_ERR(smd->pwm_vcom_signal)) {
+		dev_err(dev, "Could not get pwm device\n");
+		return PTR_ERR(smd->pwm_vcom_signal);
+	}
+
+	pwm_init_state(smd->pwm_vcom_signal, &state);
+	state.enabled = false;
+	pwm_apply_state(smd->pwm_vcom_signal, &state);
+
+	return 0;
+}
+
+static int sharp_memory_probe(struct spi_device *spi)
+{
+	int ret;
+	struct device *dev;
+	struct sharp_memory_device *smd;
+	enum sharp_memory_model model;
+	struct drm_device *drm;
+
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n");
+
+	dev = &spi->dev;
+	if (!dev->coherent_dma_mask) {
+		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to set dma mask\n");
+	}
+
+	smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver,
+				 struct sharp_memory_device, drm);
+
+	spi_set_drvdata(spi, smd);
+
+	smd->spi = spi;
+	drm = &smd->drm;
+	ret = drmm_mode_config_init(drm);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
+
+
+	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
+	if (smd->enable_gpio == NULL)
+		dev_warn(&spi->dev, "Enable gpio not defined\n");
+
+
+	/*
+	 * VCOM is a signal that prevents DC bias from being built up in
+	 * the panel resulting in pixels being forever stuck in one state.
+	 *
+	 * This driver supports three different methods to generate this
+	 * signal depending on EXTMODE pin:
+	 *
+	 * SOFTWARE_VCOM (EXTMODE = L) - This method uses a kthread to
+	 * periodically send a "maintain display" message to the display,
+	 * toggling the vcom bit on and off with each message
+	 *
+	 * EXTERNAL_VCOM (EXTMODE = H) - This method relies on an external
+	 * clock to generate the signal on the EXTCOMM pin
+	 *
+	 * PWM_VCOM (EXTMODE = H) - This method uses a pwm device to generate
+	 * the signal on the EXTCOMM pin
+	 *
+	 */
+	smd->vcom = 0;
+	if (device_property_read_u32(&spi->dev, "vcom-mode", &smd->vcom_mode))
+		return dev_err_probe(dev, ret, "Unable to find vcom-mode node in device tree\n");
+
+
+	switch (smd->vcom_mode) {
+	case SHARP_MEMORY_SOFTWARE_VCOM:
+	case SHARP_MEMORY_EXTERNAL_VCOM:
+		/* No init is needed for these two modes */
+		break;
+
+	case SHARP_MEMORY_PWM_VCOM:
+		ret = sharp_memory_init_pwm_vcom_signal(smd);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to initialize external COM signal\n");
+
+		break;
+
+	default:
+		return dev_err_probe(dev, -EINVAL, "Invalid mode specified for vcom-mode\n");
+	}
+
+	drm->mode_config.funcs = &sharp_memory_mode_config_funcs;
+
+	/* Set the DRM display mode depending on panel model */
+	model = sharp_memory_model_from_device_id(spi);
+	switch (model) {
+	case LS013B7DH03:
+		smd->mode = &sharp_memory_ls013b7dh03_mode;
+		break;
+
+	case LS010B7DH04:
+		smd->mode = &sharp_memory_ls010b7dh04_mode;
+		break;
+
+	case LS011B7DH03:
+		smd->mode = &sharp_memory_ls011b7dh03_mode;
+		break;
+
+	case LS012B7DD01:
+		smd->mode = &sharp_memory_ls012b7dd01_mode;
+		break;
+
+	case LS013B7DH05:
+		smd->mode = &sharp_memory_ls013b7dh05_mode;
+		break;
+
+	case LS018B7DH02:
+		smd->mode = &sharp_memory_ls018b7dh02_mode;
+		break;
+
+	case LS027B7DH01:
+		fallthrough;
+	case LS027B7DH01A:
+		smd->mode = &sharp_memory_ls027b7dh01_mode;
+		break;
+
+	case LS032B7DD02:
+		smd->mode = &sharp_memory_ls032b7dd02_mode;
+		break;
+
+	case LS044Q7DH01:
+		smd->mode = &sharp_memory_ls044q7dh01_mode;
+		break;
+
+	default:
+		return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n");
+	}
+
+	smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8;
+	smd->tx_buffer_size = (SHARP_MODE_PERIOD +
+			       (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) *
+			       smd->mode->vdisplay) / 8;
+
+	smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL);
+
+	mutex_init(&smd->tx_mutex);
+
+	drm->mode_config.min_width = smd->mode->hdisplay;
+	drm->mode_config.max_width = smd->mode->hdisplay;
+	drm->mode_config.min_height = smd->mode->vdisplay;
+	drm->mode_config.max_height = smd->mode->vdisplay;
+
+	ret = sharp_memory_pipe_init(drm, smd,
+				     sharp_memory_formats,
+				     ARRAY_SIZE(sharp_memory_formats),
+				     NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n");
+
+
+	drm_plane_enable_fb_damage_clips(&smd->plane);
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register drm device.\n");
+
+	drm_fbdev_dma_setup(drm, 0);
+
+	return 0;
+}
+
+static void sharp_memory_remove(struct spi_device *spi)
+{
+	struct sharp_memory_device *smd = spi_get_drvdata(spi);
+
+	drm_dev_unplug(&smd->drm);
+	drm_atomic_helper_shutdown(&smd->drm);
+}
+
+static struct spi_driver sharp_memory_spi_driver = {
+	.driver = {
+		.name = "sharp_memory",
+		.of_match_table = sharp_memory_of_match,
+	},
+	.probe = sharp_memory_probe,
+	.remove = sharp_memory_remove,
+	.id_table = sharp_memory_ids,
+};
+module_spi_driver(sharp_memory_spi_driver);
+
+
+MODULE_AUTHOR("Alex Lanzano <lanzano.alex@gmail.com>");
+MODULE_DESCRIPTION("SPI Protocol driver for the sharp_memory display");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Christophe JAILLET]

Le 25/07/2024 à 02:47, Alex Lanzano a écrit :
> Add support for the monochrome Sharp Memory LCDs.
> 
> Signed-off-by: Alex Lanzano <lanzano.alex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   MAINTAINERS                         |   8 +
>   drivers/gpu/drm/tiny/Kconfig        |  20 +
>   drivers/gpu/drm/tiny/Makefile       |   1 +
>   drivers/gpu/drm/tiny/sharp-memory.c | 742 ++++++++++++++++++++++++++++
>   4 files changed, 771 insertions(+)
>   create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> 

Hi,

below a few comments, mostly cosmetic and 2 more interesting things 
related to error handling at the end.

Hope it helps.

CJ

...

> diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
> new file mode 100644
> index 000000000000..5e61e348ce3a
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/sharp-memory.c
> @@ -0,0 +1,742 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +#include <linux/kernel.h>

Is it really needed?

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>
> +#include <linux/bitrev.h>
> +#include <linux/pwm.h>
> +#include <linux/mutex.h>

Nitpick: usually, alphabetical order is preferred.

> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_dma_helper.h>

...

> +static inline void sharp_memory_set_tx_buffer_addresses(uint8_t *buffer,
> +							struct drm_rect clip,
> +							uint32_t pitch)
> +{
> +	for (uint32_t line = 0; line < clip.y2; ++line)
> +		buffer[line * pitch] = line + 1;
> +

Nitpick: No need for an empty line.

> +}
> +
> +static void sharp_memory_set_tx_buffer_data(uint8_t *buffer,
> +					    struct drm_framebuffer *fb,
> +					    struct drm_rect clip,
> +					    uint32_t pitch,
> +					    struct drm_format_conv_state *fmtcnv_state)
> +{
> +	int ret;
> +	struct iosys_map dst, vmap;
> +	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		return;
> +
> +

Nitpick: No need for 2 newlines.

> +	iosys_map_set_vaddr(&dst, buffer);
> +	iosys_map_set_vaddr(&vmap, dma_obj->vaddr);
> +
> +	drm_fb_xrgb8888_to_mono(&dst, &pitch, &vmap, fb, &clip, fmtcnv_state);
> +
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +}

...

> +static void sharp_memory_plane_atomic_update(struct drm_plane *plane,
> +					     struct drm_atomic_state *state)
> +{
> +

Nitpick: No need for an empty line.

> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
> +	struct sharp_memory_device *smd;
> +	struct drm_rect rect;
> +
> +	smd = container_of(plane, struct sharp_memory_device, plane);
> +	if (!smd->crtc.state->active)
> +		return;
> +
> +
> +	if (drm_atomic_helper_damage_merged(old_state, plane_state, &rect))
> +		sharp_memory_fb_dirty(plane_state->fb, &rect, &fmtcnv_state);
> +
> +	drm_format_conv_state_release(&fmtcnv_state);
> +}

...

> +static void sharp_memory_crtc_enable(struct drm_crtc *crtc,
> +				     struct drm_atomic_state *state)
> +{
> +	struct pwm_state pwm_state;
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> +	sharp_memory_clear_display(smd);
> +
> +	if (smd->enable_gpio)
> +		gpiod_set_value(smd->enable_gpio, 1);
> +
> +

Nitpick: No need for 2 newlines.

> +	switch (smd->vcom_mode) {
> +	case SHARP_MEMORY_SOFTWARE_VCOM:
> +		smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
> +						  smd, "sw_vcom_signal");
> +		break;
> +
> +	case SHARP_MEMORY_EXTERNAL_VCOM:
> +		break;
> +
> +	case SHARP_MEMORY_PWM_VCOM:
> +		pwm_get_state(smd->pwm_vcom_signal, &pwm_state);
> +		pwm_state.period =    1000000000;
> +		pwm_state.duty_cycle = 100000000;
> +		pwm_state.enabled = true;
> +		pwm_apply_state(smd->pwm_vcom_signal, &pwm_state);
> +		break;
> +	}
> +}
> +
> +static void sharp_memory_crtc_disable(struct drm_crtc *crtc,
> +				      struct drm_atomic_state *state)
> +{
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> +	sharp_memory_clear_display(smd);
> +
> +	if (smd->enable_gpio)
> +		gpiod_set_value(smd->enable_gpio, 0);
> +
> +

Nitpick: No need for 2 newlines.

> +	switch (smd->vcom_mode) {
> +	case SHARP_MEMORY_SOFTWARE_VCOM:
> +		kthread_stop(smd->sw_vcom_signal);
> +		break;
> +
> +	case SHARP_MEMORY_EXTERNAL_VCOM:
> +		break;
> +
> +	case SHARP_MEMORY_PWM_VCOM:
> +		pwm_disable(smd->pwm_vcom_signal);
> +		break;
> +	}
> +}

...

> +static const struct drm_connector_funcs sharp_memory_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +

Nitpick: No need for an empty line.

> +};
> +
> +static const struct drm_mode_config_funcs sharp_memory_mode_config_funcs = {
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};

...

> +static int sharp_memory_pipe_init(struct drm_device *dev,
> +				  struct sharp_memory_device *smd,
> +				  const uint32_t *formats, unsigned int format_count,
> +				  const uint64_t *format_modifiers)
> +{
> +	int ret;
> +	struct drm_encoder *encoder = &smd->encoder;
> +	struct drm_plane *plane = &smd->plane;
> +	struct drm_crtc *crtc = &smd->crtc;
> +	struct drm_connector *connector = &smd->connector;
> +
> +	drm_plane_helper_add(plane, &sharp_memory_plane_helper_funcs);
> +	ret = drm_universal_plane_init(dev, plane, 0,
> +				       &sharp_memory_plane_funcs,
> +				       formats, format_count,
> +				       format_modifiers,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret)
> +		return ret;
> +
> +
> +	drm_crtc_helper_add(crtc, &sharp_memory_crtc_helper_funcs);
> +	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
> +					&sharp_memory_crtc_funcs, NULL);
> +	if (ret)
> +		return ret;
> +
> +

Nitpick: No need for 2 newlines. (here and 2 other time just below)

> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +	ret = drm_encoder_init(dev, encoder, &sharp_memory_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret)
> +		return ret;
> +
> +
> +	ret = drm_connector_init(&smd->drm, &smd->connector,
> +				 &sharp_memory_connector_funcs,
> +				 DRM_MODE_CONNECTOR_SPI);
> +	if (ret)
> +		return ret;
> +
> +
> +	drm_connector_helper_add(&smd->connector,
> +				 &sharp_memory_connector_hfuncs);
> +
> +	return drm_connector_attach_encoder(connector, encoder);
> +}
> +
> +static int sharp_memory_init_pwm_vcom_signal(struct sharp_memory_device *smd)
> +{
> +	struct pwm_state state;
> +	struct device *dev = &smd->spi->dev;
> +
> +	smd->pwm_vcom_signal = devm_pwm_get(dev, NULL);
> +	if (IS_ERR(smd->pwm_vcom_signal)) {
> +		dev_err(dev, "Could not get pwm device\n");
> +		return PTR_ERR(smd->pwm_vcom_signal);

Called from probe() opnly,so could be return  dev_err_probe().

> +	}
> +
> +	pwm_init_state(smd->pwm_vcom_signal, &state);
> +	state.enabled = false;
> +	pwm_apply_state(smd->pwm_vcom_signal, &state);
> +
> +	return 0;
> +}
> +
> +static int sharp_memory_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct device *dev;
> +	struct sharp_memory_device *smd;
> +	enum sharp_memory_model model;
> +	struct drm_device *drm;
> +
> +	ret = spi_setup(spi);
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n");
> +
> +	dev = &spi->dev;
> +	if (!dev->coherent_dma_mask) {
> +		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to set dma mask\n");
> +	}
> +
> +	smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver,
> +				 struct sharp_memory_device, drm);
> +
> +	spi_set_drvdata(spi, smd);
> +
> +	smd->spi = spi;
> +	drm = &smd->drm;
> +	ret = drmm_mode_config_init(drm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
> +
> +

Nitpick: No need for 2 newlines.

> +	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> +	if (smd->enable_gpio == NULL)
> +		dev_warn(&spi->dev, "Enable gpio not defined\n");
> +
> +

Nitpick: No need for 2 newlines.

> +	/*
> +	 * VCOM is a signal that prevents DC bias from being built up in
> +	 * the panel resulting in pixels being forever stuck in one state.
> +	 *
> +	 * This driver supports three different methods to generate this
> +	 * signal depending on EXTMODE pin:
> +	 *
> +	 * SOFTWARE_VCOM (EXTMODE = L) - This method uses a kthread to
> +	 * periodically send a "maintain display" message to the display,
> +	 * toggling the vcom bit on and off with each message
> +	 *
> +	 * EXTERNAL_VCOM (EXTMODE = H) - This method relies on an external
> +	 * clock to generate the signal on the EXTCOMM pin
> +	 *
> +	 * PWM_VCOM (EXTMODE = H) - This method uses a pwm device to generate
> +	 * the signal on the EXTCOMM pin
> +	 *
> +	 */
> +	smd->vcom = 0;
> +	if (device_property_read_u32(&spi->dev, "vcom-mode", &smd->vcom_mode))
> +		return dev_err_probe(dev, ret, "Unable to find vcom-mode node in device tree\n");

ret is 0 at this point, so we return 'sucess'.
Is it what is expected?

> +
> +

Nitpick: No need for 2 newlines.

> +	switch (smd->vcom_mode) {
> +	case SHARP_MEMORY_SOFTWARE_VCOM:
> +	case SHARP_MEMORY_EXTERNAL_VCOM:
> +		/* No init is needed for these two modes */
> +		break;
> +
> +	case SHARP_MEMORY_PWM_VCOM:
> +		ret = sharp_memory_init_pwm_vcom_signal(smd);

...

> +	smd->tx_buffer_size = (SHARP_MODE_PERIOD +
> +			       (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) *
> +			       smd->mode->vdisplay) / 8;
> +
> +	smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL);

Error handling?

> +
> +	mutex_init(&smd->tx_mutex);
> +
> +	drm->mode_config.min_width = smd->mode->hdisplay;
> +	drm->mode_config.max_width = smd->mode->hdisplay;
> +	drm->mode_config.min_height = smd->mode->vdisplay;
> +	drm->mode_config.max_height = smd->mode->vdisplay;

...

CJ

Re: [PATCH 0/2] Add driver for Sharp Memory LCD

[Thomas Petazzoni]

Hello Alex,

On Wed, 24 Jul 2024 20:47:01 -0400
Alex Lanzano <lanzano.alex@gmail.com> wrote:

> This patch series add support for the monochrome Sharp Memory LCD
> panels. This series is based off of the work done by Mehdi Djait.

Thanks for resuming the effort on this patch series! Since this patch
series is clearly heavily based on Mehdi's work, wouldn't it make sense
to preserve Mehdi's authorship for the patches?

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Krzysztof Kozlowski]

On 25/07/2024 02:47, Alex Lanzano wrote:
> Add support for the monochrome Sharp Memory LCDs.
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> ---
>  MAINTAINERS                         |   8 +
>  drivers/gpu/drm/tiny/Kconfig        |  20 +
>  drivers/gpu/drm/tiny/Makefile       |   1 +
>  drivers/gpu/drm/tiny/sharp-memory.c | 742 ++++++++++++++++++++++++++++
>  4 files changed, 771 insertions(+)
>  create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 71b739b40921..d19893d25913 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7123,6 +7123,14 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
>  F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
>  
> +DRM DRIVER FOR SHARP MEMORY LCD
> +M:	Alex Lanzano <lanzano.alex@gmail.com>
> +S:	Maintained
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +F:	Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> +F:	drivers/gpu/drm/tiny/sharp-memory.c
> +F:	include/dt-bindings/display/sharp-memory.h
> +
>  DRM DRIVER FOR SITRONIX ST7586 PANELS
>  M:	David Lechner <david@lechnology.com>
>  S:	Maintained
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index f6889f649bc1..bc386954faa2 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -186,6 +186,26 @@ config TINYDRM_REPAPER
>  
>  	  If M is selected the module will be called repaper.
>  
> +config TINYDRM_SHARP_MEMORY
> +	tristate "DRM support for Sharp Memory LCD panels"
> +	depends on DRM && SPI
> +	select DRM_GEM_DMA_HELPER
> +	select DRM_KMS_HELPER
> +	help
> +	  DRM Driver for the following Sharp Memory Panels:
> +	  * 1.00" Sharp Memory LCD (LS010B7DH04)
> +	  * 1.10" Sharp Memory LCD (LS011B7DH03)
> +	  * 1.20" Sharp Memory LCD (LS012B7DD01)
> +	  * 1.28" Sharp Memory LCD (LS013B7DH03)
> +	  * 1.26" Sharp Memory LCD (LS013B7DH05)
> +	  * 1.80" Sharp Memory LCD (LS018B7DH02)
> +	  * 2.70" Sharp Memory LCD (LS027B7DH01)
> +	  * 2.70" Sharp Memory LCD (LS027B7DH01A)
> +	  * 3.20" Sharp Memory LCD (LS032B7DD02)
> +	  * 4.40" Sharp Memory LCD (LS044Q7DH01)
> +
> +	  If M is selected the module will be called sharp_memory.
> +
>  config TINYDRM_ST7586
>  	tristate "DRM support for Sitronix ST7586 display panels"
>  	depends on DRM && SPI
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 76dde89a044b..4aaf56f8707d 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -14,5 +14,6 @@ obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
>  obj-$(CONFIG_TINYDRM_ILI9486)		+= ili9486.o
>  obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
>  obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
> +obj-$(CONFIG_TINYDRM_SHARP_MEMORY)	+= sharp-memory.o
>  obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
>  obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
> diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
> new file mode 100644
> index 000000000000..5e61e348ce3a
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/sharp-memory.c
> @@ -0,0 +1,742 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>
> +#include <linux/bitrev.h>
> +#include <linux/pwm.h>
> +#include <linux/mutex.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_dma_helper.h>
> +#include <drm/drm_fbdev_dma.h>
> +#include <drm/drm_format_helper.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_rect.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include <dt-bindings/display/sharp-memory.h>
> +
> +#define SHARP_MODE_PERIOD 8
> +#define SHARP_ADDR_PERIOD 8
> +#define SHARP_DUMMY_PERIOD 8
> +
> +#define SHARP_MEMORY_DISPLAY_MAINTAIN_MODE 0
> +#define SHARP_MEMORY_DISPLAY_UPDATE_MODE 1
> +#define SHARP_MEMORY_DISPLAY_CLEAR_MODE 4
> +
> +enum sharp_memory_model {
> +	LS010B7DH04 = 1,
> +	LS011B7DH03,
> +	LS012B7DD01,
> +	LS013B7DH03,
> +	LS013B7DH05,
> +	LS018B7DH02,
> +	LS027B7DH01,
> +	LS027B7DH01A,
> +	LS032B7DD02,
> +	LS044Q7DH01,
> +};
> +
> +struct sharp_memory_device {
> +	struct drm_device drm;
> +	struct spi_device *spi;
> +
> +	const struct drm_display_mode *mode;
> +
> +	struct drm_crtc crtc;
> +	struct drm_plane plane;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
> +
> +	struct gpio_desc *enable_gpio;
> +
> +	struct task_struct *sw_vcom_signal;
> +	struct pwm_device *pwm_vcom_signal;
> +
> +	uint32_t vcom_mode;
> +	uint8_t vcom;
> +
> +	uint32_t pitch;
> +	uint32_t tx_buffer_size;
> +	uint8_t *tx_buffer;
> +	struct mutex tx_mutex;
> +};
> +
> +static inline int sharp_memory_spi_write(struct spi_device *spi, void *buf, size_t len)
> +{
> +	/* Reverse the bit order */
> +	for (uint8_t *b = buf; b < ((uint8_t *)buf) + len; ++b)
> +		*b = bitrev8(*b);
> +
> +	return spi_write(spi, buf, len);
> +}
> +
> +static inline struct sharp_memory_device *drm_to_sharp_memory_device(struct drm_device *drm)
> +{
> +	return container_of(drm, struct sharp_memory_device, drm);
> +}
> +
> +DEFINE_DRM_GEM_DMA_FOPS(sharp_memory_fops);
> +
> +static const struct drm_driver sharp_memory_drm_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops			= &sharp_memory_fops,
> +	DRM_GEM_DMA_DRIVER_OPS_VMAP,
> +	.name			= "sharp_memory_display",
> +	.desc			= "Sharp Display Memory LCD",
> +	.date			= "20231129",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static inline void sharp_memory_set_tx_buffer_mode(uint8_t *buffer, uint8_t mode, uint8_t vcom)
> +{
> +
> +	*buffer = mode | (vcom << 1);
> +}
> +
> +static inline void sharp_memory_set_tx_buffer_addresses(uint8_t *buffer,
> +							struct drm_rect clip,
> +							uint32_t pitch)
> +{
> +	for (uint32_t line = 0; line < clip.y2; ++line)
> +		buffer[line * pitch] = line + 1;
> +
> +}
> +
> +static void sharp_memory_set_tx_buffer_data(uint8_t *buffer,
> +					    struct drm_framebuffer *fb,
> +					    struct drm_rect clip,
> +					    uint32_t pitch,
> +					    struct drm_format_conv_state *fmtcnv_state)
> +{
> +	int ret;
> +	struct iosys_map dst, vmap;
> +	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		return;
> +
> +
> +	iosys_map_set_vaddr(&dst, buffer);
> +	iosys_map_set_vaddr(&vmap, dma_obj->vaddr);
> +
> +	drm_fb_xrgb8888_to_mono(&dst, &pitch, &vmap, fb, &clip, fmtcnv_state);
> +
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +}
> +
> +static int sharp_memory_update_display(struct sharp_memory_device *smd,
> +				       struct drm_framebuffer *fb,
> +				       struct drm_rect clip,
> +				       struct drm_format_conv_state *fmtcnv_state)
> +{
> +	int ret;
> +	uint32_t pitch = smd->pitch;
> +	uint8_t vcom = smd->vcom;
> +	uint8_t *tx_buffer = smd->tx_buffer;
> +	uint32_t tx_buffer_size = smd->tx_buffer_size;
> +
> +	mutex_lock(&smd->tx_mutex);
> +
> +	/* Populate the transmit buffer with frame data */
> +	sharp_memory_set_tx_buffer_mode(&tx_buffer[0],
> +					SHARP_MEMORY_DISPLAY_UPDATE_MODE, vcom);
> +	sharp_memory_set_tx_buffer_addresses(&tx_buffer[1], clip, pitch);
> +	sharp_memory_set_tx_buffer_data(&tx_buffer[2], fb, clip, pitch, fmtcnv_state);
> +
> +	ret = sharp_memory_spi_write(smd->spi, tx_buffer, tx_buffer_size);
> +
> +	mutex_unlock(&smd->tx_mutex);
> +
> +	return ret;
> +}
> +
> +static int sharp_memory_maintain_display(struct sharp_memory_device *smd)
> +{
> +	int ret;
> +	uint8_t vcom = smd->vcom;
> +	uint8_t *tx_buffer = smd->tx_buffer;
> +
> +	mutex_lock(&smd->tx_mutex);
> +
> +	sharp_memory_set_tx_buffer_mode(&tx_buffer[0], SHARP_MEMORY_DISPLAY_MAINTAIN_MODE, vcom);
> +	tx_buffer[1] = 0; // Write dummy data
> +	ret = sharp_memory_spi_write(smd->spi, tx_buffer, 2);
> +
> +	mutex_unlock(&smd->tx_mutex);
> +
> +	return ret;
> +}
> +
> +static int sharp_memory_clear_display(struct sharp_memory_device *smd)
> +{
> +	int ret;
> +	uint8_t vcom = smd->vcom;
> +	uint8_t *tx_buffer = smd->tx_buffer;
> +
> +	mutex_lock(&smd->tx_mutex);
> +
> +	sharp_memory_set_tx_buffer_mode(&tx_buffer[0], SHARP_MEMORY_DISPLAY_CLEAR_MODE, vcom);
> +	tx_buffer[1] = 0; // write dummy data
> +	ret = sharp_memory_spi_write(smd->spi, tx_buffer, 2);
> +
> +	mutex_unlock(&smd->tx_mutex);
> +
> +	return ret;
> +}
> +
> +static void sharp_memory_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect,
> +				  struct drm_format_conv_state *fmtconv_state)
> +{
> +	struct drm_rect clip;
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(fb->dev);
> +
> +	/* Always update a full line regardless of what is dirty */
> +	clip.x1 = 0;
> +	clip.x2 = fb->width;
> +	clip.y1 = rect->y1;
> +	clip.y2 = rect->y2;
> +
> +	sharp_memory_update_display(smd, fb, clip, fmtconv_state);
> +}
> +
> +static int sharp_memory_plane_atomic_check(struct drm_plane *plane,
> +					   struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct sharp_memory_device *smd;
> +	struct drm_crtc_state *crtc_state;
> +
> +	smd = container_of(plane, struct sharp_memory_device, plane);
> +	crtc_state = drm_atomic_get_new_crtc_state(state, &smd->crtc);
> +
> +	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +						   DRM_PLANE_NO_SCALING,
> +						   DRM_PLANE_NO_SCALING,
> +						   false, false);
> +}
> +
> +static void sharp_memory_plane_atomic_update(struct drm_plane *plane,
> +					     struct drm_atomic_state *state)
> +{
> +
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
> +	struct sharp_memory_device *smd;
> +	struct drm_rect rect;
> +
> +	smd = container_of(plane, struct sharp_memory_device, plane);
> +	if (!smd->crtc.state->active)
> +		return;
> +
> +
> +	if (drm_atomic_helper_damage_merged(old_state, plane_state, &rect))
> +		sharp_memory_fb_dirty(plane_state->fb, &rect, &fmtcnv_state);
> +
> +	drm_format_conv_state_release(&fmtcnv_state);
> +}
> +
> +static const struct drm_plane_helper_funcs sharp_memory_plane_helper_funcs = {
> +	.prepare_fb = drm_gem_plane_helper_prepare_fb,
> +	.atomic_check = sharp_memory_plane_atomic_check,
> +	.atomic_update = sharp_memory_plane_atomic_update,
> +};
> +
> +static bool sharp_memory_format_mod_supported(struct drm_plane *plane,
> +					      uint32_t format,
> +					      uint64_t modifier)
> +{
> +	return modifier == DRM_FORMAT_MOD_LINEAR;
> +}
> +
> +static const struct drm_plane_funcs sharp_memory_plane_funcs = {
> +	.update_plane = drm_atomic_helper_update_plane,
> +	.disable_plane = drm_atomic_helper_disable_plane,
> +	.destroy = drm_plane_cleanup,
> +	.reset = drm_atomic_helper_plane_reset,
> +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +	.format_mod_supported = sharp_memory_format_mod_supported,
> +};
> +
> +static enum drm_mode_status sharp_memory_crtc_mode_valid(struct drm_crtc *crtc,
> +							 const struct drm_display_mode *mode)
> +{
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> +	return drm_crtc_helper_mode_valid_fixed(crtc, mode, smd->mode);
> +}
> +
> +static int sharp_memory_crtc_check(struct drm_crtc *crtc,
> +				   struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	int ret;
> +
> +	if (!crtc_state->enable)
> +		goto out;
> +
> +	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +out:
> +	return drm_atomic_add_affected_planes(state, crtc);
> +}
> +
> +static int sharp_memory_sw_vcom_signal_thread(void *data)
> +{
> +	struct sharp_memory_device *smd = data;
> +
> +	while (!kthread_should_stop()) {
> +		smd->vcom ^= 1; // Toggle vcom
> +		sharp_memory_maintain_display(smd);
> +		msleep(1000);
> +	}
> +
> +	return 0;
> +}
> +
> +static void sharp_memory_crtc_enable(struct drm_crtc *crtc,
> +				     struct drm_atomic_state *state)
> +{
> +	struct pwm_state pwm_state;
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> +	sharp_memory_clear_display(smd);
> +
> +	if (smd->enable_gpio)
> +		gpiod_set_value(smd->enable_gpio, 1);
> +
> +
> +	switch (smd->vcom_mode) {
> +	case SHARP_MEMORY_SOFTWARE_VCOM:
> +		smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
> +						  smd, "sw_vcom_signal");
> +		break;
> +
> +	case SHARP_MEMORY_EXTERNAL_VCOM:
> +		break;
> +
> +	case SHARP_MEMORY_PWM_VCOM:
> +		pwm_get_state(smd->pwm_vcom_signal, &pwm_state);
> +		pwm_state.period =    1000000000;
> +		pwm_state.duty_cycle = 100000000;
> +		pwm_state.enabled = true;
> +		pwm_apply_state(smd->pwm_vcom_signal, &pwm_state);
> +		break;
> +	}
> +}
> +
> +static void sharp_memory_crtc_disable(struct drm_crtc *crtc,
> +				      struct drm_atomic_state *state)
> +{
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> +
> +	sharp_memory_clear_display(smd);
> +
> +	if (smd->enable_gpio)
> +		gpiod_set_value(smd->enable_gpio, 0);
> +
> +
> +	switch (smd->vcom_mode) {
> +	case SHARP_MEMORY_SOFTWARE_VCOM:
> +		kthread_stop(smd->sw_vcom_signal);
> +		break;
> +
> +	case SHARP_MEMORY_EXTERNAL_VCOM:
> +		break;
> +
> +	case SHARP_MEMORY_PWM_VCOM:
> +		pwm_disable(smd->pwm_vcom_signal);
> +		break;
> +	}
> +}
> +
> +static const struct drm_crtc_helper_funcs sharp_memory_crtc_helper_funcs = {
> +	.mode_valid = sharp_memory_crtc_mode_valid,
> +	.atomic_check = sharp_memory_crtc_check,
> +	.atomic_enable = sharp_memory_crtc_enable,
> +	.atomic_disable = sharp_memory_crtc_disable,
> +};
> +
> +static const struct drm_crtc_funcs sharp_memory_crtc_funcs = {
> +	.reset = drm_atomic_helper_crtc_reset,
> +	.destroy = drm_crtc_cleanup,
> +	.set_config = drm_atomic_helper_set_config,
> +	.page_flip = drm_atomic_helper_page_flip,
> +	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +};
> +
> +static const struct drm_encoder_funcs sharp_memory_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +static int sharp_memory_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct sharp_memory_device *smd = drm_to_sharp_memory_device(connector->dev);
> +
> +	return drm_connector_helper_get_modes_fixed(connector, smd->mode);
> +}
> +
> +static const struct drm_connector_helper_funcs sharp_memory_connector_hfuncs = {
> +	.get_modes = sharp_memory_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs sharp_memory_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +
> +};
> +
> +static const struct drm_mode_config_funcs sharp_memory_mode_config_funcs = {
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static const struct spi_device_id sharp_memory_ids[] = {
> +	{"ls010b7dh04", LS010B7DH04},
> +	{"ls011b7dh03", LS011B7DH03},
> +	{"ls012b7dd01", LS012B7DD01},
> +	{"ls013b7dh03", LS013B7DH03},
> +	{"ls013b7dh05", LS013B7DH05},
> +	{"ls018b7dh02", LS018B7DH02},
> +	{"ls027b7dh01", LS027B7DH01},
> +	{"ls027b7dh01a", LS027B7DH01A},
> +	{"ls032b7dd02", LS032B7DD02},
> +	{"ls044q7dh01", LS044Q7DH01},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(spi, sharp_memory_ids);
> +
> +static const struct of_device_id sharp_memory_of_match[] = {
> +	{.compatible = "sharp,ls010b7dh04"},

Both ID tables should be in sync. See not-so-recent IIO discussions and
commits.

> +	{.compatible = "sharp,ls011b7dh03"},
> +	{.compatible = "sharp,ls012b7dd01"},
> +	{.compatible = "sharp,ls013b7dh03"},
> +	{.compatible = "sharp,ls013b7dh05"},
> +	{.compatible = "sharp,ls018b7dh02"},
> +	{.compatible = "sharp,ls027b7dh01"},
> +	{.compatible = "sharp,ls027b7dh01a"},
> +	{.compatible = "sharp,ls032b7dd02"},
> +	{.compatible = "sharp,ls044q7dh01"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sharp_memory_of_match);
> +
> +static const struct drm_display_mode sharp_memory_ls010b7dh04_mode = {
> +	DRM_SIMPLE_MODE(128, 128, 18, 18),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls011b7dh03_mode = {
> +	DRM_SIMPLE_MODE(160, 68, 25, 10),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls012b7dd01_mode = {
> +	DRM_SIMPLE_MODE(184, 38, 29, 6),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls013b7dh03_mode = {
> +	DRM_SIMPLE_MODE(128, 128, 23, 23),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls013b7dh05_mode = {
> +	DRM_SIMPLE_MODE(144, 168, 20, 24),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls018b7dh02_mode = {
> +	DRM_SIMPLE_MODE(230, 303, 27, 36),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls027b7dh01_mode = {
> +	DRM_SIMPLE_MODE(400, 240, 58, 35),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls032b7dd02_mode = {
> +	DRM_SIMPLE_MODE(336, 536, 42, 68),
> +};
> +
> +static const struct drm_display_mode sharp_memory_ls044q7dh01_mode = {
> +	DRM_SIMPLE_MODE(320, 240, 89, 67),
> +};
> +
> +static const uint32_t sharp_memory_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static inline enum sharp_memory_model sharp_memory_model_from_device_id(struct spi_device *spi)
> +{
> +	const struct spi_device_id *spi_id = spi_get_device_id(spi);
> +
> +	return (enum sharp_memory_model)spi_id->driver_data;

No, use appropriate wrapper, do not re-implement spi_get_device_match_data.

> +}
> +
> +static int sharp_memory_pipe_init(struct drm_device *dev,
> +				  struct sharp_memory_device *smd,
> +				  const uint32_t *formats, unsigned int format_count,
> +				  const uint64_t *format_modifiers)
> +{



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Krzysztof Kozlowski]

On 25/07/2024 02:47, Alex Lanzano wrote:
> Add device tree bindings for the monochrome Sharp Memory LCD
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> ---
>  .../bindings/display/sharp,sharp-memory.yaml  | 97 +++++++++++++++++++
>  include/dt-bindings/display/sharp-memory.h    |  9 ++
>  2 files changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
>  create mode 100644 include/dt-bindings/display/sharp-memory.h
> 
> diff --git a/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> new file mode 100644
> index 000000000000..a79edd97c857
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml

Filename based on compatible, so e.g. sharp,ls010b7dh04.yaml.

> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sharp,sharp-memory.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sharp Memory LCD panels
> +
> +maintainers:
> +  - Alex Lanzano <lanzano.alex@gmail.com>
> +
> +description:
> +  This binding is for the Sharp Memory LCD monochrome displays.

Do not say that binding is a binding... instead describe hardware.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - sharp,ls010b7dh04
> +      - sharp,ls011b7dh03
> +      - sharp,ls012b7dd01
> +      - sharp,ls013b7dh03
> +      - sharp,ls013b7dh05
> +      - sharp,ls018b7dh02
> +      - sharp,ls027b7dh01
> +      - sharp,ls027b7dh01a
> +      - sharp,ls032b7dd02
> +      - sharp,ls044q7dh01
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cs-high: true

You can drop it.

> +
> +  spi-max-frequency:
> +    maximum: 2000000
> +
> +  vcom-mode:

Missing vendor prefix.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      VCOM is a signal that prevents DC bias from being built up in
> +      the panel resulting in pixels being forever stuck in one state.
> +      vcom-mode can be set to one of three modes
> +
> +      SHARP_MEMORY_SOFTWARE_VCOM - This method uses a kthread to periodically send a
> +      "maintain display" message to the display, toggling the vcom
> +      bit on and off with each message

You described Linux, this is not suitable for bindings.

> +
> +      SHARP_MEMORY_EXTERNAL_VCOM - This method relies on an external clock to generate
> +      the signal on the EXTCOMM pin
> +
> +      SHARP_MEMORY_PWM_VCOM - This method uses a pwm device to generate the signal
> +      on the EXTCOMM pin

I don't see why do you even need this property. Just choose the best
option based on available connections/pins.

> +
> +    enum: [ 0, 1, 2 ]

Here 0/1/2 but above something entirely else. Just use strings.

> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: Enables display

Drop description and maxItems. :true is enough

> +
> +  pwms:
> +    maxItems: 1
> +    description: External VCOM signal
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-cs-high
> +  - vcom-mode
> +

allOf:

and missing ref to spi peripheral props

> +if:
> +  properties:
> +    vcom-mode:
> +      # SHARP_MEMORY_PWM_VCOM
> +      enum: [2]
> +then:
> +  required:
> +    - pwms
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/display/sharp-memory.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;

Mess indentation.

Use 4 spaces for example indentation.

> +
> +            display@0{
> +                    compatible = "sharp,ls013b7dh03";
> +                    reg = <0>;
> +                    spi-cs-high;
> +                    spi-max-frequency = <1000000>;
> +                    vcom-mode = <SHARP_MEMORY_SOFTWARE_VCOM>;
> +            };
> +    };
> +...
> diff --git a/include/dt-bindings/display/sharp-memory.h b/include/dt-bindings/display/sharp-memory.h
> new file mode 100644
> index 000000000000..dea14c7bd7ec
> --- /dev/null
> +++ b/include/dt-bindings/display/sharp-memory.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +#ifndef _DT_BINDINGS_SHARP_MEMORY
> +#define _DT_BINDINGS_SHARP_MEMORY
> +
> +#define SHARP_MEMORY_SOFTWARE_VCOM	0
> +#define SHARP_MEMORY_EXTERNAL_VCOM	1
> +#define SHARP_MEMORY_PWM_VCOM		2

Nope, drop the binding.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Krzysztof Kozlowski]

On 25/07/2024 02:47, Alex Lanzano wrote:
> Add device tree bindings for the monochrome Sharp Memory LCD
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> ---
>  .../bindings/display/sharp,sharp-memory.yaml  | 97 +++++++++++++++++++
>  include/dt-bindings/display/sharp-memory.h    |  9 ++
>  2 files changed, 106 insertions(+)

BTW, look at your original binding:
https://lore.kernel.org/dri-devel/2a7c51b6e619c02ec175a5c219b0a0fd7a24499d.1701267411.git.mehdi.djait@bootlin.com/

Why did you decide to introduce mistakes? Eg. dropping all $refs?

And finally look at the comment:
https://lore.kernel.org/dri-devel/5dbdf7bd-cfa3-492b-a0a3-fdc323cf11f8@linaro.org/
You just ignored it completely....

Best regards,
Krzysztof

Re: [PATCH 0/2] Add driver for Sharp Memory LCD

[Alex Lanzano]

On Thu, Jul 25, 2024 at 07:45:32AM GMT, Thomas Petazzoni wrote:
> Hello Alex,
> 
> On Wed, 24 Jul 2024 20:47:01 -0400
> Alex Lanzano <lanzano.alex@gmail.com> wrote:
> 
> > This patch series add support for the monochrome Sharp Memory LCD
> > panels. This series is based off of the work done by Mehdi Djait.
> 
> Thanks for resuming the effort on this patch series! Since this patch
> series is clearly heavily based on Mehdi's work, wouldn't it make sense
> to preserve Mehdi's authorship for the patches?

Hi! Thanks for the review. 

What would be the best way to go about doing this? I'm guessing appending to
MODULE_AUTHOR and adding a Signed-of-by or Co-Developed-by?

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

Thank you for the review! I will address these comments in V2

On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote:
> On 25/07/2024 02:47, Alex Lanzano wrote:
> > Add device tree bindings for the monochrome Sharp Memory LCD
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> > ---
> >  .../bindings/display/sharp,sharp-memory.yaml  | 97 +++++++++++++++++++
> >  include/dt-bindings/display/sharp-memory.h    |  9 ++
> >  2 files changed, 106 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> >  create mode 100644 include/dt-bindings/display/sharp-memory.h
> > 
> > diff --git a/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> > new file mode 100644
> > index 000000000000..a79edd97c857
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> 
> Filename based on compatible, so e.g. sharp,ls010b7dh04.yaml.
> 
> > @@ -0,0 +1,97 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/sharp,sharp-memory.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sharp Memory LCD panels
> > +
> > +maintainers:
> > +  - Alex Lanzano <lanzano.alex@gmail.com>
> > +
> > +description:
> > +  This binding is for the Sharp Memory LCD monochrome displays.
> 
> Do not say that binding is a binding... instead describe hardware.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sharp,ls010b7dh04
> > +      - sharp,ls011b7dh03
> > +      - sharp,ls012b7dd01
> > +      - sharp,ls013b7dh03
> > +      - sharp,ls013b7dh05
> > +      - sharp,ls018b7dh02
> > +      - sharp,ls027b7dh01
> > +      - sharp,ls027b7dh01a
> > +      - sharp,ls032b7dd02
> > +      - sharp,ls044q7dh01
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cs-high: true
> 
> You can drop it.
> 
> > +
> > +  spi-max-frequency:
> > +    maximum: 2000000
> > +
> > +  vcom-mode:
> 
> Missing vendor prefix.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      VCOM is a signal that prevents DC bias from being built up in
> > +      the panel resulting in pixels being forever stuck in one state.
> > +      vcom-mode can be set to one of three modes
> > +
> > +      SHARP_MEMORY_SOFTWARE_VCOM - This method uses a kthread to periodically send a
> > +      "maintain display" message to the display, toggling the vcom
> > +      bit on and off with each message
> 
> You described Linux, this is not suitable for bindings.
> 
> > +
> > +      SHARP_MEMORY_EXTERNAL_VCOM - This method relies on an external clock to generate
> > +      the signal on the EXTCOMM pin
> > +
> > +      SHARP_MEMORY_PWM_VCOM - This method uses a pwm device to generate the signal
> > +      on the EXTCOMM pin
> 
> I don't see why do you even need this property. Just choose the best
> option based on available connections/pins.
> 

I wanted to cover most of the hardware configurations I've seen with these
displays. This property simplifies the driver implementation and allows the user
to explicitly state how the VCOM signal will be generated on their platform.

> > +
> > +    enum: [ 0, 1, 2 ]
> 
> Here 0/1/2 but above something entirely else. Just use strings.
> 
> > +
> > +  enable-gpios:
> > +    maxItems: 1
> > +    description: Enables display
> 
> Drop description and maxItems. :true is enough
> 
> > +
> > +  pwms:
> > +    maxItems: 1
> > +    description: External VCOM signal
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - spi-cs-high
> > +  - vcom-mode
> > +
> 
> allOf:
> 
> and missing ref to spi peripheral props
> 
> > +if:
> > +  properties:
> > +    vcom-mode:
> > +      # SHARP_MEMORY_PWM_VCOM
> > +      enum: [2]
> > +then:
> > +  required:
> > +    - pwms
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/display/sharp-memory.h>
> > +
> > +    spi {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> 
> Mess indentation.
> 
> Use 4 spaces for example indentation.
> 
> > +
> > +            display@0{
> > +                    compatible = "sharp,ls013b7dh03";
> > +                    reg = <0>;
> > +                    spi-cs-high;
> > +                    spi-max-frequency = <1000000>;
> > +                    vcom-mode = <SHARP_MEMORY_SOFTWARE_VCOM>;
> > +            };
> > +    };
> > +...
> > diff --git a/include/dt-bindings/display/sharp-memory.h b/include/dt-bindings/display/sharp-memory.h
> > new file mode 100644
> > index 000000000000..dea14c7bd7ec
> > --- /dev/null
> > +++ b/include/dt-bindings/display/sharp-memory.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +#ifndef _DT_BINDINGS_SHARP_MEMORY
> > +#define _DT_BINDINGS_SHARP_MEMORY
> > +
> > +#define SHARP_MEMORY_SOFTWARE_VCOM	0
> > +#define SHARP_MEMORY_EXTERNAL_VCOM	1
> > +#define SHARP_MEMORY_PWM_VCOM		2
> 
> Nope, drop the binding.
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
Alex

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

On Thu, Jul 25, 2024 at 08:18:44AM GMT, Krzysztof Kozlowski wrote:
> On 25/07/2024 02:47, Alex Lanzano wrote:
> > Add device tree bindings for the monochrome Sharp Memory LCD
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> > ---
> >  .../bindings/display/sharp,sharp-memory.yaml  | 97 +++++++++++++++++++
> >  include/dt-bindings/display/sharp-memory.h    |  9 ++
> >  2 files changed, 106 insertions(+)
> 
> BTW, look at your original binding:
> https://lore.kernel.org/dri-devel/2a7c51b6e619c02ec175a5c219b0a0fd7a24499d.1701267411.git.mehdi.djait@bootlin.com/
> 
> Why did you decide to introduce mistakes? Eg. dropping all $refs?
> 
> And finally look at the comment:
> https://lore.kernel.org/dri-devel/5dbdf7bd-cfa3-492b-a0a3-fdc323cf11f8@linaro.org/
> You just ignored it completely....
> 
> Best regards,
> Krzysztof
> 

Will fix in V2.

Best regards,
Alex

Re: [PATCH 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

On Thu, Jul 25, 2024 at 08:08:57AM GMT, Krzysztof Kozlowski wrote:
> On 25/07/2024 02:47, Alex Lanzano wrote:
> > +static const struct spi_device_id sharp_memory_ids[] = {
> > +	{"ls010b7dh04", LS010B7DH04},
> > +	{"ls011b7dh03", LS011B7DH03},
> > +	{"ls012b7dd01", LS012B7DD01},
> > +	{"ls013b7dh03", LS013B7DH03},
> > +	{"ls013b7dh05", LS013B7DH05},
> > +	{"ls018b7dh02", LS018B7DH02},
> > +	{"ls027b7dh01", LS027B7DH01},
> > +	{"ls027b7dh01a", LS027B7DH01A},
> > +	{"ls032b7dd02", LS032B7DD02},
> > +	{"ls044q7dh01", LS044Q7DH01},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(spi, sharp_memory_ids);
> > +
> > +static const struct of_device_id sharp_memory_of_match[] = {
> > +	{.compatible = "sharp,ls010b7dh04"},
> 
> Both ID tables should be in sync. See not-so-recent IIO discussions and
> commits.
> 
> > +	{.compatible = "sharp,ls011b7dh03"},
> > +	{.compatible = "sharp,ls012b7dd01"},
> > +	{.compatible = "sharp,ls013b7dh03"},
> > +	{.compatible = "sharp,ls013b7dh05"},
> > +	{.compatible = "sharp,ls018b7dh02"},
> > +	{.compatible = "sharp,ls027b7dh01"},
> > +	{.compatible = "sharp,ls027b7dh01a"},
> > +	{.compatible = "sharp,ls032b7dd02"},
> > +	{.compatible = "sharp,ls044q7dh01"},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, sharp_memory_of_match);

I'm having some trouble finding the discussions and commits you're referring to.
When you say the tables should be in sync are you referring to the ordering of
entries in the tables? Like at index x of both tables should define model y?

Best regards,
Alex

Re: [PATCH 0/2] Add driver for Sharp Memory LCD

[Thomas Petazzoni]

Hello,

On Thu, 25 Jul 2024 20:07:01 -0400
Alex Lanzano <lanzano.alex@gmail.com> wrote:

> What would be the best way to go about doing this? I'm guessing appending to
> MODULE_AUTHOR and adding a Signed-of-by or Co-Developed-by?

Depends on how much you've changed things compared to Mehdi's version.
If you've mostly kept what Mehdi did, then I guess you should keep him
as the author of the commits (i.e the Author: field of the git commit +
the first Signed-off-by), and add your Signed-off-by underneath.

If you've made very significant changes to the point where the code
isn't really Mehdi's code anymore, but just inspired, then you should
set yourself as the author (Author: field of the git commit + the first
Signed-off-by), and credit Mehdi via a Co-developed-by: tag.

In any case, thanks a lot for this pushing this work, much appreciated!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com

Re: [PATCH 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Krzysztof Kozlowski]

On 26/07/2024 03:25, Alex Lanzano wrote:
> On Thu, Jul 25, 2024 at 08:08:57AM GMT, Krzysztof Kozlowski wrote:
>> On 25/07/2024 02:47, Alex Lanzano wrote:
>>> +static const struct spi_device_id sharp_memory_ids[] = {
>>> +	{"ls010b7dh04", LS010B7DH04},
>>> +	{"ls011b7dh03", LS011B7DH03},
>>> +	{"ls012b7dd01", LS012B7DD01},
>>> +	{"ls013b7dh03", LS013B7DH03},
>>> +	{"ls013b7dh05", LS013B7DH05},
>>> +	{"ls018b7dh02", LS018B7DH02},
>>> +	{"ls027b7dh01", LS027B7DH01},
>>> +	{"ls027b7dh01a", LS027B7DH01A},
>>> +	{"ls032b7dd02", LS032B7DD02},
>>> +	{"ls044q7dh01", LS044Q7DH01},
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, sharp_memory_ids);
>>> +
>>> +static const struct of_device_id sharp_memory_of_match[] = {
>>> +	{.compatible = "sharp,ls010b7dh04"},
>>
>> Both ID tables should be in sync. See not-so-recent IIO discussions and
>> commits.
>>
>>> +	{.compatible = "sharp,ls011b7dh03"},
>>> +	{.compatible = "sharp,ls012b7dd01"},
>>> +	{.compatible = "sharp,ls013b7dh03"},
>>> +	{.compatible = "sharp,ls013b7dh05"},
>>> +	{.compatible = "sharp,ls018b7dh02"},
>>> +	{.compatible = "sharp,ls027b7dh01"},
>>> +	{.compatible = "sharp,ls027b7dh01a"},
>>> +	{.compatible = "sharp,ls032b7dd02"},
>>> +	{.compatible = "sharp,ls044q7dh01"},
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sharp_memory_of_match);
> 
> I'm having some trouble finding the discussions and commits you're referring to.
> When you say the tables should be in sync are you referring to the ordering of
> entries in the tables? Like at index x of both tables should define model y?

No, the match data.

62d3fb9dcc091ccdf25eb3b716e90e07e3ed861f

Best regards,
Krzysztof

[PATCH v2 0/2] Add driver for Sharp Memory LCD

[Alex Lanzano]

This patch series add support for the monochrome Sharp Memory LCD
panels. This series is based off of the work done by Mehdi Djait.

References:
https://lore.kernel.org/dri-devel/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/
https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf

Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
Changes in v2:
- Credited Mehdi Djait in commit messages
- Renamed sharp,sharp-memory.yaml to sharp,ls010b7dh04.yaml
- Using strings instead of int for vcom-mode in dt-binding
- Fixed indentation of binding example
- Removed binding header
- Removed extra whitespace in sharp-memory.c
- Fixed error handling in sharp-memory.c
- Added match data to of_device_id table to be in-sync with spi_device_id table
- Replaced redundant function with spi_get_device_match_data
- Sorted header files in sharp-memory.c
---
Alex Lanzano (2):
  dt-bindings: display: Add Sharp Memory LCD bindings
  drm/tiny: Add driver for Sharp Memory LCD

 .../bindings/display/sharp,ls010b7dh04.yaml   |  93 +++
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/tiny/Kconfig                  |  20 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/sharp-memory.c           | 727 ++++++++++++++++++
 5 files changed, 848 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
 create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c

-- 
2.45.2

[PATCH v2 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

Add device tree bindings for the monochrome Sharp Memory LCD

Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
 .../bindings/display/sharp,ls010b7dh04.yaml   | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml

diff --git a/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
new file mode 100644
index 000000000000..79bde7bf0d7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sharp,ls010b7dh04.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sharp Memory LCD panels
+
+maintainers:
+  - Alex Lanzano <lanzano.alex@gmail.com>
+
+description:
+  Sharp Memory LCDs are a series of monochrome displays that operate over
+  a SPI bus when the chip select is high. The displays require a signal (VCOM)
+  to be generated to prevent DC bias build up resulting in pixels being
+  unable to change. Three modes can be used to provide the VCOM signal
+  ("software", "external", "pwm").
+
+properties:
+  compatible:
+    enum:
+      - sharp,ls010b7dh04
+      - sharp,ls011b7dh03
+      - sharp,ls012b7dd01
+      - sharp,ls013b7dh03
+      - sharp,ls013b7dh05
+      - sharp,ls018b7dh02
+      - sharp,ls027b7dh01
+      - sharp,ls027b7dh01a
+      - sharp,ls032b7dd02
+      - sharp,ls044q7dh01
+
+  reg:
+    maxItems: 1
+
+  spi-cs-high: true
+
+  spi-max-frequency:
+    maximum: 2000000
+
+  sharp,vcom-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: |
+      software - This mode relies on a software operation to send a
+      "maintain display" message to the display, toggling the vcom
+      bit on and off with each message
+
+      external - This mode relies on an external clock to generate
+      the signal on the EXTCOMM pin
+
+      pwm - This mode relies on a pwm device to generate the signal
+      on the EXTCOMM pin
+
+  enable-gpios: true
+
+  pwms:
+    maxItems: 1
+    description: External VCOM signal
+
+required:
+  - compatible
+  - reg
+  - spi-cs-high
+  - sharp,vcom-mode
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  - if:
+      properties:
+        sharp,vcom-mode:
+          const: pwm
+    then:
+      required:
+        - pwms
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        display@0{
+            compatible = "sharp,ls013b7dh03";
+            reg = <0>;
+            spi-cs-high;
+            spi-max-frequency = <1000000>;
+            sharp,vcom-mode = "software";
+        };
+    };
+...
-- 
2.45.2

[PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

Add support for the monochrome Sharp Memory LCDs.

Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
 MAINTAINERS                         |   7 +
 drivers/gpu/drm/tiny/Kconfig        |  20 +
 drivers/gpu/drm/tiny/Makefile       |   1 +
 drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++
 4 files changed, 754 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 71b739b40921..b5b08247a994 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7123,6 +7123,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
 F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
 
+DRM DRIVER FOR SHARP MEMORY LCD
+M:	Alex Lanzano <lanzano.alex@gmail.com>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
+F:	drivers/gpu/drm/tiny/sharp-memory.c
+
 DRM DRIVER FOR SITRONIX ST7586 PANELS
 M:	David Lechner <david@lechnology.com>
 S:	Maintained
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649bc1..bc386954faa2 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -186,6 +186,26 @@ config TINYDRM_REPAPER
 
 	  If M is selected the module will be called repaper.
 
+config TINYDRM_SHARP_MEMORY
+	tristate "DRM support for Sharp Memory LCD panels"
+	depends on DRM && SPI
+	select DRM_GEM_DMA_HELPER
+	select DRM_KMS_HELPER
+	help
+	  DRM Driver for the following Sharp Memory Panels:
+	  * 1.00" Sharp Memory LCD (LS010B7DH04)
+	  * 1.10" Sharp Memory LCD (LS011B7DH03)
+	  * 1.20" Sharp Memory LCD (LS012B7DD01)
+	  * 1.28" Sharp Memory LCD (LS013B7DH03)
+	  * 1.26" Sharp Memory LCD (LS013B7DH05)
+	  * 1.80" Sharp Memory LCD (LS018B7DH02)
+	  * 2.70" Sharp Memory LCD (LS027B7DH01)
+	  * 2.70" Sharp Memory LCD (LS027B7DH01A)
+	  * 3.20" Sharp Memory LCD (LS032B7DD02)
+	  * 4.40" Sharp Memory LCD (LS044Q7DH01)
+
+	  If M is selected the module will be called sharp_memory.
+
 config TINYDRM_ST7586
 	tristate "DRM support for Sitronix ST7586 display panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a044b..4aaf56f8707d 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -14,5 +14,6 @@ obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
 obj-$(CONFIG_TINYDRM_ILI9486)		+= ili9486.o
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
+obj-$(CONFIG_TINYDRM_SHARP_MEMORY)	+= sharp-memory.o
 obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
 obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
new file mode 100644
index 000000000000..facff2b955ec
--- /dev/null
+++ b/drivers/gpu/drm/tiny/sharp-memory.c
@@ -0,0 +1,726 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_dma_helper.h>
+#include <drm/drm_fbdev_dma.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_rect.h>
+#include <linux/bitrev.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kthread.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pwm.h>
+#include <linux/spi/spi.h>
+
+#define SHARP_MODE_PERIOD 8
+#define SHARP_ADDR_PERIOD 8
+#define SHARP_DUMMY_PERIOD 8
+
+#define SHARP_MEMORY_DISPLAY_MAINTAIN_MODE 0
+#define SHARP_MEMORY_DISPLAY_UPDATE_MODE 1
+#define SHARP_MEMORY_DISPLAY_CLEAR_MODE 4
+
+enum sharp_memory_model {
+	LS010B7DH04,
+	LS011B7DH03,
+	LS012B7DD01,
+	LS013B7DH03,
+	LS013B7DH05,
+	LS018B7DH02,
+	LS027B7DH01,
+	LS027B7DH01A,
+	LS032B7DD02,
+	LS044Q7DH01,
+};
+
+enum sharp_memory_vcom_mode {
+	SHARP_MEMORY_SOFTWARE_VCOM,
+	SHARP_MEMORY_EXTERNAL_VCOM,
+	SHARP_MEMORY_PWM_VCOM
+};
+
+struct sharp_memory_device {
+	struct drm_device drm;
+	struct spi_device *spi;
+
+	const struct drm_display_mode *mode;
+
+	struct drm_crtc crtc;
+	struct drm_plane plane;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+
+	struct gpio_desc *enable_gpio;
+
+	struct task_struct *sw_vcom_signal;
+	struct pwm_device *pwm_vcom_signal;
+
+	enum sharp_memory_vcom_mode vcom_mode;
+	uint8_t vcom;
+
+	uint32_t pitch;
+	uint32_t tx_buffer_size;
+	uint8_t *tx_buffer;
+	struct mutex tx_mutex;
+};
+
+static inline int sharp_memory_spi_write(struct spi_device *spi, void *buf, size_t len)
+{
+	/* Reverse the bit order */
+	for (uint8_t *b = buf; b < ((uint8_t *)buf) + len; ++b)
+		*b = bitrev8(*b);
+
+	return spi_write(spi, buf, len);
+}
+
+static inline struct sharp_memory_device *drm_to_sharp_memory_device(struct drm_device *drm)
+{
+	return container_of(drm, struct sharp_memory_device, drm);
+}
+
+DEFINE_DRM_GEM_DMA_FOPS(sharp_memory_fops);
+
+static const struct drm_driver sharp_memory_drm_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops			= &sharp_memory_fops,
+	DRM_GEM_DMA_DRIVER_OPS_VMAP,
+	.name			= "sharp_memory_display",
+	.desc			= "Sharp Display Memory LCD",
+	.date			= "20231129",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static inline void sharp_memory_set_tx_buffer_mode(uint8_t *buffer, uint8_t mode, uint8_t vcom)
+{
+
+	*buffer = mode | (vcom << 1);
+}
+
+static inline void sharp_memory_set_tx_buffer_addresses(uint8_t *buffer,
+							struct drm_rect clip,
+							uint32_t pitch)
+{
+	for (uint32_t line = 0; line < clip.y2; ++line)
+		buffer[line * pitch] = line + 1;
+
+}
+
+static void sharp_memory_set_tx_buffer_data(uint8_t *buffer,
+					    struct drm_framebuffer *fb,
+					    struct drm_rect clip,
+					    uint32_t pitch,
+					    struct drm_format_conv_state *fmtcnv_state)
+{
+	int ret;
+	struct iosys_map dst, vmap;
+	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		return;
+
+	iosys_map_set_vaddr(&dst, buffer);
+	iosys_map_set_vaddr(&vmap, dma_obj->vaddr);
+
+	drm_fb_xrgb8888_to_mono(&dst, &pitch, &vmap, fb, &clip, fmtcnv_state);
+
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+}
+
+static int sharp_memory_update_display(struct sharp_memory_device *smd,
+				       struct drm_framebuffer *fb,
+				       struct drm_rect clip,
+				       struct drm_format_conv_state *fmtcnv_state)
+{
+	int ret;
+	uint32_t pitch = smd->pitch;
+	uint8_t vcom = smd->vcom;
+	uint8_t *tx_buffer = smd->tx_buffer;
+	uint32_t tx_buffer_size = smd->tx_buffer_size;
+
+	mutex_lock(&smd->tx_mutex);
+
+	/* Populate the transmit buffer with frame data */
+	sharp_memory_set_tx_buffer_mode(&tx_buffer[0],
+					SHARP_MEMORY_DISPLAY_UPDATE_MODE, vcom);
+	sharp_memory_set_tx_buffer_addresses(&tx_buffer[1], clip, pitch);
+	sharp_memory_set_tx_buffer_data(&tx_buffer[2], fb, clip, pitch, fmtcnv_state);
+
+	ret = sharp_memory_spi_write(smd->spi, tx_buffer, tx_buffer_size);
+
+	mutex_unlock(&smd->tx_mutex);
+
+	return ret;
+}
+
+static int sharp_memory_maintain_display(struct sharp_memory_device *smd)
+{
+	int ret;
+	uint8_t vcom = smd->vcom;
+	uint8_t *tx_buffer = smd->tx_buffer;
+
+	mutex_lock(&smd->tx_mutex);
+
+	sharp_memory_set_tx_buffer_mode(&tx_buffer[0], SHARP_MEMORY_DISPLAY_MAINTAIN_MODE, vcom);
+	tx_buffer[1] = 0; // Write dummy data
+	ret = sharp_memory_spi_write(smd->spi, tx_buffer, 2);
+
+	mutex_unlock(&smd->tx_mutex);
+
+	return ret;
+}
+
+static int sharp_memory_clear_display(struct sharp_memory_device *smd)
+{
+	int ret;
+	uint8_t vcom = smd->vcom;
+	uint8_t *tx_buffer = smd->tx_buffer;
+
+	mutex_lock(&smd->tx_mutex);
+
+	sharp_memory_set_tx_buffer_mode(&tx_buffer[0], SHARP_MEMORY_DISPLAY_CLEAR_MODE, vcom);
+	tx_buffer[1] = 0; // write dummy data
+	ret = sharp_memory_spi_write(smd->spi, tx_buffer, 2);
+
+	mutex_unlock(&smd->tx_mutex);
+
+	return ret;
+}
+
+static void sharp_memory_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect,
+				  struct drm_format_conv_state *fmtconv_state)
+{
+	struct drm_rect clip;
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(fb->dev);
+
+	/* Always update a full line regardless of what is dirty */
+	clip.x1 = 0;
+	clip.x2 = fb->width;
+	clip.y1 = rect->y1;
+	clip.y2 = rect->y2;
+
+	sharp_memory_update_display(smd, fb, clip, fmtconv_state);
+}
+
+static int sharp_memory_plane_atomic_check(struct drm_plane *plane,
+					   struct drm_atomic_state *state)
+{
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct sharp_memory_device *smd;
+	struct drm_crtc_state *crtc_state;
+
+	smd = container_of(plane, struct sharp_memory_device, plane);
+	crtc_state = drm_atomic_get_new_crtc_state(state, &smd->crtc);
+
+	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
+
+static void sharp_memory_plane_atomic_update(struct drm_plane *plane,
+					     struct drm_atomic_state *state)
+{
+
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
+	struct sharp_memory_device *smd;
+	struct drm_rect rect;
+
+	smd = container_of(plane, struct sharp_memory_device, plane);
+	if (!smd->crtc.state->active)
+		return;
+
+	if (drm_atomic_helper_damage_merged(old_state, plane_state, &rect))
+		sharp_memory_fb_dirty(plane_state->fb, &rect, &fmtcnv_state);
+
+	drm_format_conv_state_release(&fmtcnv_state);
+}
+
+static const struct drm_plane_helper_funcs sharp_memory_plane_helper_funcs = {
+	.prepare_fb = drm_gem_plane_helper_prepare_fb,
+	.atomic_check = sharp_memory_plane_atomic_check,
+	.atomic_update = sharp_memory_plane_atomic_update,
+};
+
+static bool sharp_memory_format_mod_supported(struct drm_plane *plane,
+					      uint32_t format,
+					      uint64_t modifier)
+{
+	return modifier == DRM_FORMAT_MOD_LINEAR;
+}
+
+static const struct drm_plane_funcs sharp_memory_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	.reset = drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+	.format_mod_supported = sharp_memory_format_mod_supported,
+};
+
+static enum drm_mode_status sharp_memory_crtc_mode_valid(struct drm_crtc *crtc,
+							 const struct drm_display_mode *mode)
+{
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, smd->mode);
+}
+
+static int sharp_memory_crtc_check(struct drm_crtc *crtc,
+				   struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	int ret;
+
+	if (!crtc_state->enable)
+		goto out;
+
+	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+	if (ret)
+		return ret;
+
+out:
+	return drm_atomic_add_affected_planes(state, crtc);
+}
+
+static int sharp_memory_sw_vcom_signal_thread(void *data)
+{
+	struct sharp_memory_device *smd = data;
+
+	while (!kthread_should_stop()) {
+		smd->vcom ^= 1; // Toggle vcom
+		sharp_memory_maintain_display(smd);
+		msleep(1000);
+	}
+
+	return 0;
+}
+
+static void sharp_memory_crtc_enable(struct drm_crtc *crtc,
+				     struct drm_atomic_state *state)
+{
+	struct pwm_state pwm_state;
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
+
+	sharp_memory_clear_display(smd);
+
+	if (smd->enable_gpio)
+		gpiod_set_value(smd->enable_gpio, 1);
+
+	switch (smd->vcom_mode) {
+	case SHARP_MEMORY_SOFTWARE_VCOM:
+		smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
+						  smd, "sw_vcom_signal");
+		break;
+
+	case SHARP_MEMORY_EXTERNAL_VCOM:
+		break;
+
+	case SHARP_MEMORY_PWM_VCOM:
+		pwm_get_state(smd->pwm_vcom_signal, &pwm_state);
+		pwm_state.period =    1000000000;
+		pwm_state.duty_cycle = 100000000;
+		pwm_state.enabled = true;
+		pwm_apply_state(smd->pwm_vcom_signal, &pwm_state);
+		break;
+	}
+}
+
+static void sharp_memory_crtc_disable(struct drm_crtc *crtc,
+				      struct drm_atomic_state *state)
+{
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
+
+	sharp_memory_clear_display(smd);
+
+	if (smd->enable_gpio)
+		gpiod_set_value(smd->enable_gpio, 0);
+
+	switch (smd->vcom_mode) {
+	case SHARP_MEMORY_SOFTWARE_VCOM:
+		kthread_stop(smd->sw_vcom_signal);
+		break;
+
+	case SHARP_MEMORY_EXTERNAL_VCOM:
+		break;
+
+	case SHARP_MEMORY_PWM_VCOM:
+		pwm_disable(smd->pwm_vcom_signal);
+		break;
+	}
+}
+
+static const struct drm_crtc_helper_funcs sharp_memory_crtc_helper_funcs = {
+	.mode_valid = sharp_memory_crtc_mode_valid,
+	.atomic_check = sharp_memory_crtc_check,
+	.atomic_enable = sharp_memory_crtc_enable,
+	.atomic_disable = sharp_memory_crtc_disable,
+};
+
+static const struct drm_crtc_funcs sharp_memory_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs sharp_memory_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static int sharp_memory_connector_get_modes(struct drm_connector *connector)
+{
+	struct sharp_memory_device *smd = drm_to_sharp_memory_device(connector->dev);
+
+	return drm_connector_helper_get_modes_fixed(connector, smd->mode);
+}
+
+static const struct drm_connector_helper_funcs sharp_memory_connector_hfuncs = {
+	.get_modes = sharp_memory_connector_get_modes,
+};
+
+static const struct drm_connector_funcs sharp_memory_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+
+};
+
+static const struct drm_mode_config_funcs sharp_memory_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct spi_device_id sharp_memory_ids[] = {
+	{"ls010b7dh04", LS010B7DH04},
+	{"ls011b7dh03", LS011B7DH03},
+	{"ls012b7dd01", LS012B7DD01},
+	{"ls013b7dh03", LS013B7DH03},
+	{"ls013b7dh05", LS013B7DH05},
+	{"ls018b7dh02", LS018B7DH02},
+	{"ls027b7dh01", LS027B7DH01},
+	{"ls027b7dh01a", LS027B7DH01A},
+	{"ls032b7dd02", LS032B7DD02},
+	{"ls044q7dh01", LS044Q7DH01},
+	{},
+};
+MODULE_DEVICE_TABLE(spi, sharp_memory_ids);
+
+static const struct of_device_id sharp_memory_of_match[] = {
+	{.compatible = "sharp,ls010b7dh04", (void *)LS010B7DH04},
+	{.compatible = "sharp,ls011b7dh03", (void *)LS011B7DH03},
+	{.compatible = "sharp,ls012b7dd01", (void *)LS012B7DD01},
+	{.compatible = "sharp,ls013b7dh03", (void *)LS013B7DH03},
+	{.compatible = "sharp,ls013b7dh05", (void *)LS013B7DH05},
+	{.compatible = "sharp,ls018b7dh02", (void *)LS018B7DH02},
+	{.compatible = "sharp,ls027b7dh01", (void *)LS027B7DH01},
+	{.compatible = "sharp,ls027b7dh01a", (void *)LS027B7DH01},
+	{.compatible = "sharp,ls032b7dd02", (void *)LS032B7DD02},
+	{.compatible = "sharp,ls044q7dh01", (void *)LS044Q7DH01},
+	{},
+};
+MODULE_DEVICE_TABLE(of, sharp_memory_of_match);
+
+static const struct drm_display_mode sharp_memory_ls010b7dh04_mode = {
+	DRM_SIMPLE_MODE(128, 128, 18, 18),
+};
+
+static const struct drm_display_mode sharp_memory_ls011b7dh03_mode = {
+	DRM_SIMPLE_MODE(160, 68, 25, 10),
+};
+
+static const struct drm_display_mode sharp_memory_ls012b7dd01_mode = {
+	DRM_SIMPLE_MODE(184, 38, 29, 6),
+};
+
+static const struct drm_display_mode sharp_memory_ls013b7dh03_mode = {
+	DRM_SIMPLE_MODE(128, 128, 23, 23),
+};
+
+static const struct drm_display_mode sharp_memory_ls013b7dh05_mode = {
+	DRM_SIMPLE_MODE(144, 168, 20, 24),
+};
+
+static const struct drm_display_mode sharp_memory_ls018b7dh02_mode = {
+	DRM_SIMPLE_MODE(230, 303, 27, 36),
+};
+
+static const struct drm_display_mode sharp_memory_ls027b7dh01_mode = {
+	DRM_SIMPLE_MODE(400, 240, 58, 35),
+};
+
+static const struct drm_display_mode sharp_memory_ls032b7dd02_mode = {
+	DRM_SIMPLE_MODE(336, 536, 42, 68),
+};
+
+static const struct drm_display_mode sharp_memory_ls044q7dh01_mode = {
+	DRM_SIMPLE_MODE(320, 240, 89, 67),
+};
+
+static const uint32_t sharp_memory_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+static int sharp_memory_pipe_init(struct drm_device *dev,
+				  struct sharp_memory_device *smd,
+				  const uint32_t *formats, unsigned int format_count,
+				  const uint64_t *format_modifiers)
+{
+	int ret;
+	struct drm_encoder *encoder = &smd->encoder;
+	struct drm_plane *plane = &smd->plane;
+	struct drm_crtc *crtc = &smd->crtc;
+	struct drm_connector *connector = &smd->connector;
+
+	drm_plane_helper_add(plane, &sharp_memory_plane_helper_funcs);
+	ret = drm_universal_plane_init(dev, plane, 0,
+				       &sharp_memory_plane_funcs,
+				       formats, format_count,
+				       format_modifiers,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+
+	drm_crtc_helper_add(crtc, &sharp_memory_crtc_helper_funcs);
+	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
+					&sharp_memory_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+	ret = drm_encoder_init(dev, encoder, &sharp_memory_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ret;
+
+	ret = drm_connector_init(&smd->drm, &smd->connector,
+				 &sharp_memory_connector_funcs,
+				 DRM_MODE_CONNECTOR_SPI);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(&smd->connector,
+				 &sharp_memory_connector_hfuncs);
+
+	return drm_connector_attach_encoder(connector, encoder);
+}
+
+static int sharp_memory_init_pwm_vcom_signal(struct sharp_memory_device *smd)
+{
+	struct pwm_state state;
+	struct device *dev = &smd->spi->dev;
+
+	smd->pwm_vcom_signal = devm_pwm_get(dev, NULL);
+	if (IS_ERR(smd->pwm_vcom_signal)) {
+		dev_err(dev, "Could not get pwm device\n");
+		return dev_err_probe(dev, -EINVAL, "Could not get pwm device\n");
+	}
+
+	pwm_init_state(smd->pwm_vcom_signal, &state);
+	state.enabled = false;
+	pwm_apply_state(smd->pwm_vcom_signal, &state);
+
+	return 0;
+}
+
+static int sharp_memory_probe(struct spi_device *spi)
+{
+	int ret;
+	struct device *dev;
+	struct sharp_memory_device *smd;
+	enum sharp_memory_model model;
+	struct drm_device *drm;
+	const char *vcom_mode_str;
+
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n");
+
+	dev = &spi->dev;
+	if (!dev->coherent_dma_mask) {
+		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to set dma mask\n");
+	}
+
+	smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver,
+				 struct sharp_memory_device, drm);
+
+	spi_set_drvdata(spi, smd);
+
+	smd->spi = spi;
+	drm = &smd->drm;
+	ret = drmm_mode_config_init(drm);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
+
+	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
+	if (smd->enable_gpio == NULL)
+		dev_warn(&spi->dev, "Enable gpio not defined\n");
+
+	/*
+	 * VCOM is a signal that prevents DC bias from being built up in
+	 * the panel resulting in pixels being forever stuck in one state.
+	 *
+	 * This driver supports three different methods to generate this
+	 * signal depending on EXTMODE pin:
+	 *
+	 * software (EXTMODE = L) - This mode uses a kthread to
+	 * periodically send a "maintain display" message to the display,
+	 * toggling the vcom bit on and off with each message
+	 *
+	 * external (EXTMODE = H) - This mode relies on an external
+	 * clock to generate the signal on the EXTCOMM pin
+	 *
+	 * pwm (EXTMODE = H) - This mode uses a pwm device to generate
+	 * the signal on the EXTCOMM pin
+	 *
+	 */
+	smd->vcom = 0;
+	if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str))
+		return dev_err_probe(dev, -EINVAL,
+				     "Unable to find vcom-mode node in device tree\n");
+
+	if (!strcmp("software", vcom_mode_str)) {
+		smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM;
+
+	} else if (!strcmp("external", vcom_mode_str)) {
+		smd->vcom_mode = SHARP_MEMORY_EXTERNAL_VCOM;
+
+	} else if (!strcmp("pwm", vcom_mode_str)) {
+		smd->vcom_mode = SHARP_MEMORY_PWM_VCOM;
+		ret = sharp_memory_init_pwm_vcom_signal(smd);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to initialize external COM signal\n");
+	} else {
+		return dev_err_probe(dev, -EINVAL, "Invalid value set for vcom-mode\n");
+	}
+
+	drm->mode_config.funcs = &sharp_memory_mode_config_funcs;
+
+	/* Set the DRM display mode depending on panel model */
+	model = (uintptr_t)spi_get_device_match_data(spi);
+	switch (model) {
+	case LS013B7DH03:
+		smd->mode = &sharp_memory_ls013b7dh03_mode;
+		break;
+
+	case LS010B7DH04:
+		smd->mode = &sharp_memory_ls010b7dh04_mode;
+		break;
+
+	case LS011B7DH03:
+		smd->mode = &sharp_memory_ls011b7dh03_mode;
+		break;
+
+	case LS012B7DD01:
+		smd->mode = &sharp_memory_ls012b7dd01_mode;
+		break;
+
+	case LS013B7DH05:
+		smd->mode = &sharp_memory_ls013b7dh05_mode;
+		break;
+
+	case LS018B7DH02:
+		smd->mode = &sharp_memory_ls018b7dh02_mode;
+		break;
+
+	case LS027B7DH01:
+		fallthrough;
+	case LS027B7DH01A:
+		smd->mode = &sharp_memory_ls027b7dh01_mode;
+		break;
+
+	case LS032B7DD02:
+		smd->mode = &sharp_memory_ls032b7dd02_mode;
+		break;
+
+	case LS044Q7DH01:
+		smd->mode = &sharp_memory_ls044q7dh01_mode;
+		break;
+
+	default:
+		return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n");
+	}
+
+	smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8;
+	smd->tx_buffer_size = (SHARP_MODE_PERIOD +
+			       (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) *
+			       smd->mode->vdisplay) / 8;
+
+	smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL);
+	if (!smd->tx_buffer)
+		return dev_err_probe(&spi->dev, -ENOMEM, "Unable to alloc tx buffer\n");
+
+	mutex_init(&smd->tx_mutex);
+
+	drm->mode_config.min_width = smd->mode->hdisplay;
+	drm->mode_config.max_width = smd->mode->hdisplay;
+	drm->mode_config.min_height = smd->mode->vdisplay;
+	drm->mode_config.max_height = smd->mode->vdisplay;
+
+	ret = sharp_memory_pipe_init(drm, smd,
+				     sharp_memory_formats,
+				     ARRAY_SIZE(sharp_memory_formats),
+				     NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n");
+
+	drm_plane_enable_fb_damage_clips(&smd->plane);
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register drm device.\n");
+
+	drm_fbdev_dma_setup(drm, 0);
+
+	return 0;
+}
+
+static void sharp_memory_remove(struct spi_device *spi)
+{
+	struct sharp_memory_device *smd = spi_get_drvdata(spi);
+
+	drm_dev_unplug(&smd->drm);
+	drm_atomic_helper_shutdown(&smd->drm);
+}
+
+static struct spi_driver sharp_memory_spi_driver = {
+	.driver = {
+		.name = "sharp_memory",
+		.of_match_table = sharp_memory_of_match,
+	},
+	.probe = sharp_memory_probe,
+	.remove = sharp_memory_remove,
+	.id_table = sharp_memory_ids,
+};
+module_spi_driver(sharp_memory_spi_driver);
+
+MODULE_AUTHOR("Alex Lanzano <lanzano.alex@gmail.com>");
+MODULE_DESCRIPTION("SPI Protocol driver for the sharp_memory display");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[kernel test robot]

Hi Alex,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on drm-misc/drm-misc-next linus/master v6.10 next-20240726]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Lanzano/dt-bindings-display-Add-Sharp-Memory-LCD-bindings/20240727-035313
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240726194456.1336484-3-lanzano.alex%40gmail.com
patch subject: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD
reproduce: (https://download.01.org/0day-ci/archive/20240727/202407270505.ADlarsms-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407270505.ADlarsms-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/userspace-api/netlink/index.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
   Warning: Documentation/userspace-api/netlink/specs.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/display/exynos/
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
   Using alabaster theme

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Christophe JAILLET]

Le 26/07/2024 à 21:44, Alex Lanzano a écrit :
> Add support for the monochrome Sharp Memory LCDs.
> 
> Signed-off-by: Alex Lanzano <lanzano.alex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Co-developed-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> ---
>   MAINTAINERS                         |   7 +
>   drivers/gpu/drm/tiny/Kconfig        |  20 +
>   drivers/gpu/drm/tiny/Makefile       |   1 +
>   drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++
>   4 files changed, 754 insertions(+)
>   create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> 

Hi,

below some other tiny comments, hoping it helps.

Also "./scripts/checkpatch.pl --strict" gives some hints, should some be 
of interest.

> +static int sharp_memory_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct device *dev;
> +	struct sharp_memory_device *smd;
> +	enum sharp_memory_model model;
> +	struct drm_device *drm;
> +	const char *vcom_mode_str;
> +
> +	ret = spi_setup(spi);
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n");
> +
> +	dev = &spi->dev;

6 times below, &spi->dev could be replaced by dev, to slightly simplify 
things.

> +	if (!dev->coherent_dma_mask) {
> +		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to set dma mask\n");
> +	}
> +
> +	smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver,
> +				 struct sharp_memory_device, drm);

Missing error handling.

> +
> +	spi_set_drvdata(spi, smd);
> +
> +	smd->spi = spi;
> +	drm = &smd->drm;
> +	ret = drmm_mode_config_init(drm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
> +
> +	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> +	if (smd->enable_gpio == NULL)

Nitpick: if (!smd->enable_gpio)

> +		dev_warn(&spi->dev, "Enable gpio not defined\n");
> +
> +	/*
> +	 * VCOM is a signal that prevents DC bias from being built up in
> +	 * the panel resulting in pixels being forever stuck in one state.
> +	 *
> +	 * This driver supports three different methods to generate this
> +	 * signal depending on EXTMODE pin:
> +	 *
> +	 * software (EXTMODE = L) - This mode uses a kthread to
> +	 * periodically send a "maintain display" message to the display,
> +	 * toggling the vcom bit on and off with each message
> +	 *
> +	 * external (EXTMODE = H) - This mode relies on an external
> +	 * clock to generate the signal on the EXTCOMM pin
> +	 *
> +	 * pwm (EXTMODE = H) - This mode uses a pwm device to generate
> +	 * the signal on the EXTCOMM pin
> +	 *
> +	 */
> +	smd->vcom = 0;

Nitpick: devm_drm_dev_alloc() already zeroes the allocated memory. So 
this is useless. Unless you think it gives some useful hint, it could be 
removed.

> +	if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Unable to find vcom-mode node in device tree\n");
> +
> +	if (!strcmp("software", vcom_mode_str)) {
> +		smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM;

...

> +	smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8;
> +	smd->tx_buffer_size = (SHARP_MODE_PERIOD +
> +			       (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) *
> +			       smd->mode->vdisplay) / 8;
> +
> +	smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL);
> +	if (!smd->tx_buffer)
> +		return dev_err_probe(&spi->dev, -ENOMEM, "Unable to alloc tx buffer\n");

There is no need to log a message for memory allocation failure.
return -ENOMEM; should be just fine IMHO.

> +
> +	mutex_init(&smd->tx_mutex);
> +
> +	drm->mode_config.min_width = smd->mode->hdisplay;
> +	drm->mode_config.max_width = smd->mode->hdisplay;
> +	drm->mode_config.min_height = smd->mode->vdisplay;
> +	drm->mode_config.max_height = smd->mode->vdisplay;
> +
> +	ret = sharp_memory_pipe_init(drm, smd,
> +				     sharp_memory_formats,

Nitpick: you could save 1 LoC if this is put at the end of the previous 
line.

> +				     ARRAY_SIZE(sharp_memory_formats),
> +				     NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n");
> +
> +	drm_plane_enable_fb_damage_clips(&smd->plane);
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register drm device.\n");
> +
> +	drm_fbdev_dma_setup(drm, 0);
> +
> +	return 0;
> +}

...

CJ

Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

On Sat, Jul 27, 2024 at 12:12:01AM GMT, Christophe JAILLET wrote:
> Le 26/07/2024 à 21:44, Alex Lanzano a écrit :
> > Add support for the monochrome Sharp Memory LCDs.
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Co-developed-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Mehdi Djait <mehdi.djait-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> > ---
> >   MAINTAINERS                         |   7 +
> >   drivers/gpu/drm/tiny/Kconfig        |  20 +
> >   drivers/gpu/drm/tiny/Makefile       |   1 +
> >   drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++
> >   4 files changed, 754 insertions(+)
> >   create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> > 
> 
> Hi,
> 
> below some other tiny comments, hoping it helps.
> 
> Also "./scripts/checkpatch.pl --strict" gives some hints, should some be of
> interest.
> 

Ah! Thank you. I'll be sure to use strict from now on

> > +static int sharp_memory_probe(struct spi_device *spi)
> > +{
> > +	int ret;
> > +	struct device *dev;
> > +	struct sharp_memory_device *smd;
> > +	enum sharp_memory_model model;
> > +	struct drm_device *drm;
> > +	const char *vcom_mode_str;
> > +
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return dev_err_probe(&spi->dev, ret, "Failed to setup spi device\n");
> > +
> > +	dev = &spi->dev;
> 
> 6 times below, &spi->dev could be replaced by dev, to slightly simplify
> things.
> 
> > +	if (!dev->coherent_dma_mask) {
> > +		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "Failed to set dma mask\n");
> > +	}
> > +
> > +	smd = devm_drm_dev_alloc(&spi->dev, &sharp_memory_drm_driver,
> > +				 struct sharp_memory_device, drm);
> 
> Missing error handling.
> 
> > +
> > +	spi_set_drvdata(spi, smd);
> > +
> > +	smd->spi = spi;
> > +	drm = &smd->drm;
> > +	ret = drmm_mode_config_init(drm);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
> > +
> > +	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> > +	if (smd->enable_gpio == NULL)
> 
> Nitpick: if (!smd->enable_gpio)
> 
> > +		dev_warn(&spi->dev, "Enable gpio not defined\n");
> > +
> > +	/*
> > +	 * VCOM is a signal that prevents DC bias from being built up in
> > +	 * the panel resulting in pixels being forever stuck in one state.
> > +	 *
> > +	 * This driver supports three different methods to generate this
> > +	 * signal depending on EXTMODE pin:
> > +	 *
> > +	 * software (EXTMODE = L) - This mode uses a kthread to
> > +	 * periodically send a "maintain display" message to the display,
> > +	 * toggling the vcom bit on and off with each message
> > +	 *
> > +	 * external (EXTMODE = H) - This mode relies on an external
> > +	 * clock to generate the signal on the EXTCOMM pin
> > +	 *
> > +	 * pwm (EXTMODE = H) - This mode uses a pwm device to generate
> > +	 * the signal on the EXTCOMM pin
> > +	 *
> > +	 */
> > +	smd->vcom = 0;
> 
> Nitpick: devm_drm_dev_alloc() already zeroes the allocated memory. So this
> is useless. Unless you think it gives some useful hint, it could be removed.
> 
> > +	if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str))
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Unable to find vcom-mode node in device tree\n");
> > +
> > +	if (!strcmp("software", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM;
> 
> ...
> 
> > +	smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8;
> > +	smd->tx_buffer_size = (SHARP_MODE_PERIOD +
> > +			       (SHARP_ADDR_PERIOD + (smd->mode->hdisplay) + SHARP_DUMMY_PERIOD) *
> > +			       smd->mode->vdisplay) / 8;
> > +
> > +	smd->tx_buffer = devm_kzalloc(&spi->dev, smd->tx_buffer_size, GFP_KERNEL);
> > +	if (!smd->tx_buffer)
> > +		return dev_err_probe(&spi->dev, -ENOMEM, "Unable to alloc tx buffer\n");
> 
> There is no need to log a message for memory allocation failure.
> return -ENOMEM; should be just fine IMHO.
> 
> > +
> > +	mutex_init(&smd->tx_mutex);
> > +
> > +	drm->mode_config.min_width = smd->mode->hdisplay;
> > +	drm->mode_config.max_width = smd->mode->hdisplay;
> > +	drm->mode_config.min_height = smd->mode->vdisplay;
> > +	drm->mode_config.max_height = smd->mode->vdisplay;
> > +
> > +	ret = sharp_memory_pipe_init(drm, smd,
> > +				     sharp_memory_formats,
> 
> Nitpick: you could save 1 LoC if this is put at the end of the previous
> line.
> 
> > +				     ARRAY_SIZE(sharp_memory_formats),
> > +				     NULL);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n");
> > +
> > +	drm_plane_enable_fb_damage_clips(&smd->plane);
> > +	drm_mode_config_reset(drm);
> > +
> > +	ret = drm_dev_register(drm, 0);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to register drm device.\n");
> > +
> > +	drm_fbdev_dma_setup(drm, 0);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> CJ
> 
> 

I appreciate the review! I'll get these addressed in v3

Best regards,
Alex

Re: [PATCH v2 0/2] Add driver for Sharp Memory LCD

[Krzysztof Kozlowski]

On 26/07/2024 21:44, Alex Lanzano wrote:
> This patch series add support for the monochrome Sharp Memory LCD
> panels. This series is based off of the work done by Mehdi Djait.
> 
> References:
> https://lore.kernel.org/dri-devel/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/
> https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Krzysztof Kozlowski]

On 26/07/2024 21:44, Alex Lanzano wrote:
> Add support for the monochrome Sharp Memory LCDs.
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> ---
>  MAINTAINERS                         |   7 +
>  drivers/gpu/drm/tiny/Kconfig        |  20 +
>  drivers/gpu/drm/tiny/Makefile       |   1 +
>  drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++
>  4 files changed, 754 insertions(+)
>  create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 71b739b40921..b5b08247a994 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7123,6 +7123,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
>  F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
>  
> +DRM DRIVER FOR SHARP MEMORY LCD
> +M:	Alex Lanzano <lanzano.alex@gmail.com>
> +S:	Maintained
> +T:	git git://anongit.freedesktop.org/drm/drm-misc


Do you have drm-misc commit rights? If not, drop it. There is no point
to put some other maintainer's tree in your entry. Git tree is already
present in the maintainer's entry who is going to apply the patches.


> +F:	Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml

If you rename the file in your patchset, you must rename it EVERYWHERE.


> +F:	drivers/gpu/drm/tiny/sharp-memory.c
> +
>  DRM DRIVER FOR SITRONIX ST7586 PANELS


...

> +	smd->spi = spi;
> +	drm = &smd->drm;
> +	ret = drmm_mode_config_init(drm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
> +
> +	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> +	if (smd->enable_gpio == NULL)
> +		dev_warn(&spi->dev, "Enable gpio not defined\n");
> +
> +	/*
> +	 * VCOM is a signal that prevents DC bias from being built up in
> +	 * the panel resulting in pixels being forever stuck in one state.
> +	 *
> +	 * This driver supports three different methods to generate this
> +	 * signal depending on EXTMODE pin:
> +	 *
> +	 * software (EXTMODE = L) - This mode uses a kthread to
> +	 * periodically send a "maintain display" message to the display,
> +	 * toggling the vcom bit on and off with each message
> +	 *
> +	 * external (EXTMODE = H) - This mode relies on an external
> +	 * clock to generate the signal on the EXTCOMM pin
> +	 *
> +	 * pwm (EXTMODE = H) - This mode uses a pwm device to generate
> +	 * the signal on the EXTCOMM pin
> +	 *
> +	 */
> +	smd->vcom = 0;
> +	if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Unable to find vcom-mode node in device tree\n");
> +
> +	if (!strcmp("software", vcom_mode_str)) {
> +		smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM;
> +
> +	} else if (!strcmp("external", vcom_mode_str)) {
> +		smd->vcom_mode = SHARP_MEMORY_EXTERNAL_VCOM;
> +
> +	} else if (!strcmp("pwm", vcom_mode_str)) {
> +		smd->vcom_mode = SHARP_MEMORY_PWM_VCOM;
> +		ret = sharp_memory_init_pwm_vcom_signal(smd);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to initialize external COM signal\n");
> +	} else {
> +		return dev_err_probe(dev, -EINVAL, "Invalid value set for vcom-mode\n");
> +	}
> +
> +	drm->mode_config.funcs = &sharp_memory_mode_config_funcs;
> +
> +	/* Set the DRM display mode depending on panel model */
> +	model = (uintptr_t)spi_get_device_match_data(spi);
> +	switch (model) {
> +	case LS013B7DH03:
> +		smd->mode = &sharp_memory_ls013b7dh03_mode;
> +		break;
> +
> +	case LS010B7DH04:
> +		smd->mode = &sharp_memory_ls010b7dh04_mode;
> +		break;
> +
> +	case LS011B7DH03:
> +		smd->mode = &sharp_memory_ls011b7dh03_mode;
> +		break;
> +
> +	case LS012B7DD01:
> +		smd->mode = &sharp_memory_ls012b7dd01_mode;
> +		break;
> +
> +	case LS013B7DH05:
> +		smd->mode = &sharp_memory_ls013b7dh05_mode;
> +		break;
> +
> +	case LS018B7DH02:
> +		smd->mode = &sharp_memory_ls018b7dh02_mode;
> +		break;
> +
> +	case LS027B7DH01:
> +		fallthrough;
> +	case LS027B7DH01A:
> +		smd->mode = &sharp_memory_ls027b7dh01_mode;
> +		break;
> +
> +	case LS032B7DD02:
> +		smd->mode = &sharp_memory_ls032b7dd02_mode;
> +		break;
> +
> +	case LS044Q7DH01:
> +		smd->mode = &sharp_memory_ls044q7dh01_mode;
> +		break;

This is over-complicated. Just store the mode in device match data.


> +
> +	default:
> +		return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n");
> +	}


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Krzysztof Kozlowski]

On 26/07/2024 21:44, Alex Lanzano wrote:
> Add device tree bindings for the monochrome Sharp Memory LCD
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>

The order of tags is incorrect. Who developed it first? Please read at
Submitting patches - it explained this case quite precisely.

> ---
>  .../bindings/display/sharp,ls010b7dh04.yaml   | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> new file mode 100644
> index 000000000000..79bde7bf0d7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sharp,ls010b7dh04.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sharp Memory LCD panels
> +
> +maintainers:
> +  - Alex Lanzano <lanzano.alex@gmail.com>
> +
> +description:
> +  Sharp Memory LCDs are a series of monochrome displays that operate over
> +  a SPI bus when the chip select is high. The displays require a signal (VCOM)
> +  to be generated to prevent DC bias build up resulting in pixels being
> +  unable to change. Three modes can be used to provide the VCOM signal
> +  ("software", "external", "pwm").
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sharp,ls010b7dh04
> +      - sharp,ls011b7dh03
> +      - sharp,ls012b7dd01
> +      - sharp,ls013b7dh03
> +      - sharp,ls013b7dh05
> +      - sharp,ls018b7dh02
> +      - sharp,ls027b7dh01
> +      - sharp,ls027b7dh01a
> +      - sharp,ls032b7dd02
> +      - sharp,ls044q7dh01
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cs-high: true

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

> +
> +  spi-max-frequency:
> +    maximum: 2000000
> +
> +  sharp,vcom-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: |
> +      software - This mode relies on a software operation to send a
> +      "maintain display" message to the display, toggling the vcom
> +      bit on and off with each message
> +
> +      external - This mode relies on an external clock to generate
> +      the signal on the EXTCOMM pin

External clock? Then you might be missing clocks property.
> +
> +      pwm - This mode relies on a pwm device to generate the signal
> +      on the EXTCOMM pin

That's an enum. Otherwise why "pony" would be a correct vcom-mode?

> +
> +  enable-gpios: true
> +
> +  pwms:
> +    maxItems: 1
> +    description: External VCOM signal
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-cs-high
> +  - sharp,vcom-mode
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        sharp,vcom-mode:
> +          const: pwm
> +    then:
> +      required:
> +        - pwms
> +
> +additionalProperties: false

Instead:
unevaluatedProperties: false

> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        display@0{

Missing space (see DTS coding style or any DTS)

> +            compatible = "sharp,ls013b7dh03";
> +            reg = <0>;
> +            spi-cs-high;
> +            spi-max-frequency = <1000000>;
> +            sharp,vcom-mode = "software";
> +        };
> +    };
> +...

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote:
> On 25/07/2024 02:47, Alex Lanzano wrote:
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sharp,ls010b7dh04
> > +      - sharp,ls011b7dh03
> > +      - sharp,ls012b7dd01
> > +      - sharp,ls013b7dh03
> > +      - sharp,ls013b7dh05
> > +      - sharp,ls018b7dh02
> > +      - sharp,ls027b7dh01
> > +      - sharp,ls027b7dh01a
> > +      - sharp,ls032b7dd02
> > +      - sharp,ls044q7dh01
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cs-high: true
> 
> You can drop it.
> 

This is a required property in order for the display to function correctly.
But I have no issues removing it if there's a better place to document it.

Best regards,
Alex

Re: [PATCH v2 0/2] Add driver for Sharp Memory LCD

[Alex Lanzano]

On Sat, Jul 27, 2024 at 11:00:08AM GMT, Krzysztof Kozlowski wrote:
> On 26/07/2024 21:44, Alex Lanzano wrote:
> > This patch series add support for the monochrome Sharp Memory LCD
> > panels. This series is based off of the work done by Mehdi Djait.
> > 
> > References:
> > https://lore.kernel.org/dri-devel/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/
> > https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> 
> Do not attach (thread) your patchsets to some other threads (unrelated
> or older versions). This buries them deep in the mailbox and might
> interfere with applying entire sets.
> 
> Best regards,
> Krzysztof
> 

Will do! Sorry about that

Best regards,
Alex

Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

On Sat, Jul 27, 2024 at 11:03:19AM GMT, Krzysztof Kozlowski wrote:
> On 26/07/2024 21:44, Alex Lanzano wrote:
> > Add support for the monochrome Sharp Memory LCDs.
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > ---
> >  MAINTAINERS                         |   7 +
> >  drivers/gpu/drm/tiny/Kconfig        |  20 +
> >  drivers/gpu/drm/tiny/Makefile       |   1 +
> >  drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++
> >  4 files changed, 754 insertions(+)
> >  create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 71b739b40921..b5b08247a994 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7123,6 +7123,13 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
> >  F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> >  
> > +DRM DRIVER FOR SHARP MEMORY LCD
> > +M:	Alex Lanzano <lanzano.alex@gmail.com>
> > +S:	Maintained
> > +T:	git git://anongit.freedesktop.org/drm/drm-misc
> 
> 
> Do you have drm-misc commit rights? If not, drop it. There is no point
> to put some other maintainer's tree in your entry. Git tree is already
> present in the maintainer's entry who is going to apply the patches.
> 
> 

Will remove.

> > +F:	Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> 
> If you rename the file in your patchset, you must rename it EVERYWHERE.
> 
> 

Will do.

> > +F:	drivers/gpu/drm/tiny/sharp-memory.c
> > +
> >  DRM DRIVER FOR SITRONIX ST7586 PANELS
> 
> 
> ...
> 

Oh! Did you need me to rename sharp-memory.c as well?

> > +	smd->spi = spi;
> > +	drm = &smd->drm;
> > +	ret = drmm_mode_config_init(drm);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
> > +
> > +	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> > +	if (smd->enable_gpio == NULL)
> > +		dev_warn(&spi->dev, "Enable gpio not defined\n");
> > +
> > +	/*
> > +	 * VCOM is a signal that prevents DC bias from being built up in
> > +	 * the panel resulting in pixels being forever stuck in one state.
> > +	 *
> > +	 * This driver supports three different methods to generate this
> > +	 * signal depending on EXTMODE pin:
> > +	 *
> > +	 * software (EXTMODE = L) - This mode uses a kthread to
> > +	 * periodically send a "maintain display" message to the display,
> > +	 * toggling the vcom bit on and off with each message
> > +	 *
> > +	 * external (EXTMODE = H) - This mode relies on an external
> > +	 * clock to generate the signal on the EXTCOMM pin
> > +	 *
> > +	 * pwm (EXTMODE = H) - This mode uses a pwm device to generate
> > +	 * the signal on the EXTCOMM pin
> > +	 *
> > +	 */
> > +	smd->vcom = 0;
> > +	if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str))
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Unable to find vcom-mode node in device tree\n");
> > +
> > +	if (!strcmp("software", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM;
> > +
> > +	} else if (!strcmp("external", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_EXTERNAL_VCOM;
> > +
> > +	} else if (!strcmp("pwm", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_PWM_VCOM;
> > +		ret = sharp_memory_init_pwm_vcom_signal(smd);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to initialize external COM signal\n");
> > +	} else {
> > +		return dev_err_probe(dev, -EINVAL, "Invalid value set for vcom-mode\n");
> > +	}
> > +
> > +	drm->mode_config.funcs = &sharp_memory_mode_config_funcs;
> > +
> > +	/* Set the DRM display mode depending on panel model */
> > +	model = (uintptr_t)spi_get_device_match_data(spi);
> > +	switch (model) {
> > +	case LS013B7DH03:
> > +		smd->mode = &sharp_memory_ls013b7dh03_mode;
> > +		break;
> > +
> > +	case LS010B7DH04:
> > +		smd->mode = &sharp_memory_ls010b7dh04_mode;
> > +		break;
> > +
> > +	case LS011B7DH03:
> > +		smd->mode = &sharp_memory_ls011b7dh03_mode;
> > +		break;
> > +
> > +	case LS012B7DD01:
> > +		smd->mode = &sharp_memory_ls012b7dd01_mode;
> > +		break;
> > +
> > +	case LS013B7DH05:
> > +		smd->mode = &sharp_memory_ls013b7dh05_mode;
> > +		break;
> > +
> > +	case LS018B7DH02:
> > +		smd->mode = &sharp_memory_ls018b7dh02_mode;
> > +		break;
> > +
> > +	case LS027B7DH01:
> > +		fallthrough;
> > +	case LS027B7DH01A:
> > +		smd->mode = &sharp_memory_ls027b7dh01_mode;
> > +		break;
> > +
> > +	case LS032B7DD02:
> > +		smd->mode = &sharp_memory_ls032b7dd02_mode;
> > +		break;
> > +
> > +	case LS044Q7DH01:
> > +		smd->mode = &sharp_memory_ls044q7dh01_mode;
> > +		break;
> 
> This is over-complicated. Just store the mode in device match data.
> 
> 

Will simplify in v3

> > +
> > +	default:
> > +		return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n");
> > +	}
> 
> 
> Best regards,
> Krzysztof
> 

Thank you for taking the time to review!

Best regards,
Alex

Re: [PATCH v2 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

On Sat, Jul 27, 2024 at 11:06:05AM GMT, Krzysztof Kozlowski wrote:
> On 26/07/2024 21:44, Alex Lanzano wrote:
> > Add device tree bindings for the monochrome Sharp Memory LCD
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> 
> The order of tags is incorrect. Who developed it first? Please read at
> Submitting patches - it explained this case quite precisely.
> 

Will fix!

> > ---
> >  .../bindings/display/sharp,ls010b7dh04.yaml   | 94 +++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> > new file mode 100644
> > index 000000000000..79bde7bf0d7d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> > @@ -0,0 +1,94 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/sharp,ls010b7dh04.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sharp Memory LCD panels
> > +
> > +maintainers:
> > +  - Alex Lanzano <lanzano.alex@gmail.com>
> > +
> > +description:
> > +  Sharp Memory LCDs are a series of monochrome displays that operate over
> > +  a SPI bus when the chip select is high. The displays require a signal (VCOM)
> > +  to be generated to prevent DC bias build up resulting in pixels being
> > +  unable to change. Three modes can be used to provide the VCOM signal
> > +  ("software", "external", "pwm").
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sharp,ls010b7dh04
> > +      - sharp,ls011b7dh03
> > +      - sharp,ls012b7dd01
> > +      - sharp,ls013b7dh03
> > +      - sharp,ls013b7dh05
> > +      - sharp,ls018b7dh02
> > +      - sharp,ls027b7dh01
> > +      - sharp,ls027b7dh01a
> > +      - sharp,ls032b7dd02
> > +      - sharp,ls044q7dh01
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cs-high: true
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>
> 

My apologies! I must've missed the comment on this line. I replied to this
on the v1 thread

> > +
> > +  spi-max-frequency:
> > +    maximum: 2000000
> > +
> > +  sharp,vcom-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: |
> > +      software - This mode relies on a software operation to send a
> > +      "maintain display" message to the display, toggling the vcom
> > +      bit on and off with each message
> > +
> > +      external - This mode relies on an external clock to generate
> > +      the signal on the EXTCOMM pin
> 
> External clock? Then you might be missing clocks property.

This is to handle the case where a clock IC or other signal generator IC is provided
on the display module itself. So I don't believe the clocks property would be 
needed.

> > +
> > +      pwm - This mode relies on a pwm device to generate the signal
> > +      on the EXTCOMM pin
> 
> That's an enum. Otherwise why "pony" would be a correct vcom-mode?
> 

Will fix!

> > +
> > +  enable-gpios: true
> > +
> > +  pwms:
> > +    maxItems: 1
> > +    description: External VCOM signal
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - spi-cs-high
> > +  - sharp,vcom-mode
> > +
> > +allOf:
> > +  - $ref: panel/panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        sharp,vcom-mode:
> > +          const: pwm
> > +    then:
> > +      required:
> > +        - pwms
> > +
> > +additionalProperties: false
> 
> Instead:
> unevaluatedProperties: false
> 

Will fix!

> > +
> > +examples:
> > +  - |
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        display@0{
> 
> Missing space (see DTS coding style or any DTS)
> 

Will Fix!

> > +            compatible = "sharp,ls013b7dh03";
> > +            reg = <0>;
> > +            spi-cs-high;
> > +            spi-max-frequency = <1000000>;
> > +            sharp,vcom-mode = "software";
> > +        };
> > +    };
> > +...
> 
> Best regards,
> Krzysztof
> 

Again, thank you for taking the time to review.

Best regards,
Alex

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Krzysztof Kozlowski]

On 27/07/2024 18:30, Alex Lanzano wrote:
> On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote:
>> On 25/07/2024 02:47, Alex Lanzano wrote:
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - sharp,ls010b7dh04
>>> +      - sharp,ls011b7dh03
>>> +      - sharp,ls012b7dd01
>>> +      - sharp,ls013b7dh03
>>> +      - sharp,ls013b7dh05
>>> +      - sharp,ls018b7dh02
>>> +      - sharp,ls027b7dh01
>>> +      - sharp,ls027b7dh01a
>>> +      - sharp,ls032b7dd02
>>> +      - sharp,ls044q7dh01
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  spi-cs-high: true
>>
>> You can drop it.
>>
> 
> This is a required property in order for the display to function correctly.
> But I have no issues removing it if there's a better place to document it.

The sharp LCD device or the board (e.g. via some inversion)?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

On Sun, Jul 28, 2024 at 10:40:23AM GMT, Krzysztof Kozlowski wrote:
> On 27/07/2024 18:30, Alex Lanzano wrote:
> > On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote:
> >> On 25/07/2024 02:47, Alex Lanzano wrote:
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - sharp,ls010b7dh04
> >>> +      - sharp,ls011b7dh03
> >>> +      - sharp,ls012b7dd01
> >>> +      - sharp,ls013b7dh03
> >>> +      - sharp,ls013b7dh05
> >>> +      - sharp,ls018b7dh02
> >>> +      - sharp,ls027b7dh01
> >>> +      - sharp,ls027b7dh01a
> >>> +      - sharp,ls032b7dd02
> >>> +      - sharp,ls044q7dh01
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  spi-cs-high: true
> >>
> >> You can drop it.
> >>
> > 
> > This is a required property in order for the display to function correctly.
> > But I have no issues removing it if there's a better place to document it.
> 
> The sharp LCD device or the board (e.g. via some inversion)?
> 

The sharp LCD device itself.

https://cdn.sparkfun.com/assets/d/e/8/9/7/LS013B7DH03_datasheet.pdf
Page 16 of the PDF below shows the timing chart for it if interested

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Krzysztof Kozlowski]

On 28/07/2024 14:56, Alex Lanzano wrote:
>>>>> +
>>>>> +  spi-cs-high: true
>>>>
>>>> You can drop it.
>>>>
>>>
>>> This is a required property in order for the display to function correctly.
>>> But I have no issues removing it if there's a better place to document it.
>>
>> The sharp LCD device or the board (e.g. via some inversion)?
>>
> 
> The sharp LCD device itself.
> 
> https://cdn.sparkfun.com/assets/d/e/8/9/7/LS013B7DH03_datasheet.pdf
> Page 16 of the PDF below shows the timing chart for it if interested

OK, so you enforce chip select as always high and then someone comes
with a board which has a inversion and expect the SoC to drive it low,
as usually? In such system this would not work, would it?

IOW, I think this is the first case where you require specific CS high
and this should make you think why...

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

On Sun, Jul 28, 2024 at 04:43:42PM GMT, Krzysztof Kozlowski wrote:
> On 28/07/2024 14:56, Alex Lanzano wrote:
> >>>>> +
> >>>>> +  spi-cs-high: true
> >>>>
> >>>> You can drop it.
> >>>>
> >>>
> >>> This is a required property in order for the display to function correctly.
> >>> But I have no issues removing it if there's a better place to document it.
> >>
> >> The sharp LCD device or the board (e.g. via some inversion)?
> >>
> > 
> > The sharp LCD device itself.
> > 
> > https://cdn.sparkfun.com/assets/d/e/8/9/7/LS013B7DH03_datasheet.pdf
> > Page 16 of the PDF below shows the timing chart for it if interested
> 
> OK, so you enforce chip select as always high and then someone comes
> with a board which has a inversion and expect the SoC to drive it low,
> as usually? In such system this would not work, would it?
> 
> IOW, I think this is the first case where you require specific CS high
> and this should make you think why...

Ahh I see what you mean! I'll remove it.

Best regard,
Alex