[PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
(https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:

    ./fbtest -f /dev/fb1
    Using drawops cfb32 (32 bpp packed pixels)
    Available visuals:
      Monochrome
      Grayscale 256
      Truecolor 8:8:8:0
    Using visops truecolor
    Running all tests
    test001: PASSED
    test002: PASSED
    test003: PASSED
    test004: PASSED
    test005: PASSED
    test006: PASSED
    test008: PASSED
    Screen size too small for this test
    test010: PASSED
    Benchmarking... 10x10 squares: 414.41 Mpixels/s
    Benchmarking... 20x20 squares: 858.31 Mpixels/s
    Benchmarking... 50x50 squares: 1586.33 Mpixels/s
    test012: PASSED
    Benchmarking... R5 circles: 234.68 Mpixels/s
    Benchmarking... R10 circles: 498.24 Mpixels/s
    Benchmarking... R25 circles: 942.34 Mpixels/s
    test013: PASSED

This is a v2 that addresses all the issues pointed in v1, thanks a lot
to everyone that gave me feedback and reviews. I tried to not miss any
comment, but there were a lot so forgive me if something is not there.

Patch #1 adds two new helpers, drm_fb_gray8_to_mono_reversed() to convert
from grayscale to monochrome and a drm_fb_xrgb8888_to_mono_reversed() to
convert from XR24 to monochrome. The latter internally use thes former.

Patch #2 adds the driver. The name ssd130x was used instead of ssd1307fb
to denote that this driver is not only for SSD1307, but also for other
displays from the same chip family.

Patch #3 just adds a MAINTAINERS entry for the DRM driver and patch #4
adds myself as a co-maintainer of the existing Device Tree binding for
ssd1307fb, since the same is shared between the fbdev and DRM drivers.

Best regards,
Javier

Changes in v2:
- Drop patch that was adding a DRM_MODE_CONNECTOR_I2C type.
- Invert order of backlight {en,dis}able and display {on,off} (Sam Ravnborg)
- Don't clear the screen and turn on display on probe (Sam Ravnborg)
- Use backlight_get_brightness() macro to get BL brightness (Sam Ravnborg)
- Use dev managed version of devm_backlight_device_register() (Sam Ravnborg)
- Use dev_name(dev) for backlight name instead of an array (Sam Ravnborg)
- Drop the .get_brightness callback since isn't needed  (Sam Ravnborg)
- Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg)
- Add Sam Ravnborg's acked-by tag to patch 3/4.
- Rename driver to ssd130x since supports a display family (Thomas Zimmermann)
- Drop the TINY prefix from the Kconfig symbol (Thomas Zimmermann)
- Sort the Kconfig symbol dependencies alphabetically (Thomas Zimmermann)
- Rename struct ssd130x_array to struct ssd130x_i2c_msg (Thomas Zimmermann)
- Rename struct ssd130x_i2c_msg .type member to .cmd (Thomas Zimmermann)
- Use sizeof(*foo) instead of sizeof(struct foo) (Thomas Zimmermann)
- Use struct_size() macro to calculate sizeof(*foo) + len (Thomas Zimmermann)
- Use kcalloc() instead of kmalloc_array() + memset() (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Reorganize code in probe to make it more legible (Thomas Zimmermann)
- ssd130x_write_cmd() uses varargs to simplify I2C code (Thomas Zimmermann)
- Move regulator/pwm init logic to display pipe enable callback.
- Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann)
- Add a drm_fb_gray8_to_mono_reversed_line() helper (Thomas Zimmermann)

Javier Martinez Canillas (4):
  drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()
  drm/tiny: Add driver for Solomon SSD130X OLED displays
  MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver
  dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

 .../bindings/display/solomon,ssd1307fb.yaml   |   1 +
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/drm_format_helper.c           |  80 ++
 drivers/gpu/drm/tiny/Kconfig                  |  12 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/ssd130x.c                | 971 ++++++++++++++++++
 include/drm/drm_format_helper.h               |   7 +
 7 files changed, 1079 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ssd130x.c

-- 
2.34.1

[PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
(https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:

    ./fbtest -f /dev/fb1
    Using drawops cfb32 (32 bpp packed pixels)
    Available visuals:
      Monochrome
      Grayscale 256
      Truecolor 8:8:8:0
    Using visops truecolor
    Running all tests
    test001: PASSED
    test002: PASSED
    test003: PASSED
    test004: PASSED
    test005: PASSED
    test006: PASSED
    test008: PASSED
    Screen size too small for this test
    test010: PASSED
    Benchmarking... 10x10 squares: 414.41 Mpixels/s
    Benchmarking... 20x20 squares: 858.31 Mpixels/s
    Benchmarking... 50x50 squares: 1586.33 Mpixels/s
    test012: PASSED
    Benchmarking... R5 circles: 234.68 Mpixels/s
    Benchmarking... R10 circles: 498.24 Mpixels/s
    Benchmarking... R25 circles: 942.34 Mpixels/s
    test013: PASSED

This is a v2 that addresses all the issues pointed in v1, thanks a lot
to everyone that gave me feedback and reviews. I tried to not miss any
comment, but there were a lot so forgive me if something is not there.

Patch #1 adds two new helpers, drm_fb_gray8_to_mono_reversed() to convert
from grayscale to monochrome and a drm_fb_xrgb8888_to_mono_reversed() to
convert from XR24 to monochrome. The latter internally use thes former.

Patch #2 adds the driver. The name ssd130x was used instead of ssd1307fb
to denote that this driver is not only for SSD1307, but also for other
displays from the same chip family.

Patch #3 just adds a MAINTAINERS entry for the DRM driver and patch #4
adds myself as a co-maintainer of the existing Device Tree binding for
ssd1307fb, since the same is shared between the fbdev and DRM drivers.

Best regards,
Javier

Changes in v2:
- Drop patch that was adding a DRM_MODE_CONNECTOR_I2C type.
- Invert order of backlight {en,dis}able and display {on,off} (Sam Ravnborg)
- Don't clear the screen and turn on display on probe (Sam Ravnborg)
- Use backlight_get_brightness() macro to get BL brightness (Sam Ravnborg)
- Use dev managed version of devm_backlight_device_register() (Sam Ravnborg)
- Use dev_name(dev) for backlight name instead of an array (Sam Ravnborg)
- Drop the .get_brightness callback since isn't needed  (Sam Ravnborg)
- Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg)
- Add Sam Ravnborg's acked-by tag to patch 3/4.
- Rename driver to ssd130x since supports a display family (Thomas Zimmermann)
- Drop the TINY prefix from the Kconfig symbol (Thomas Zimmermann)
- Sort the Kconfig symbol dependencies alphabetically (Thomas Zimmermann)
- Rename struct ssd130x_array to struct ssd130x_i2c_msg (Thomas Zimmermann)
- Rename struct ssd130x_i2c_msg .type member to .cmd (Thomas Zimmermann)
- Use sizeof(*foo) instead of sizeof(struct foo) (Thomas Zimmermann)
- Use struct_size() macro to calculate sizeof(*foo) + len (Thomas Zimmermann)
- Use kcalloc() instead of kmalloc_array() + memset() (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Reorganize code in probe to make it more legible (Thomas Zimmermann)
- ssd130x_write_cmd() uses varargs to simplify I2C code (Thomas Zimmermann)
- Move regulator/pwm init logic to display pipe enable callback.
- Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann)
- Add a drm_fb_gray8_to_mono_reversed_line() helper (Thomas Zimmermann)

Javier Martinez Canillas (4):
  drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()
  drm/tiny: Add driver for Solomon SSD130X OLED displays
  MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver
  dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

 .../bindings/display/solomon,ssd1307fb.yaml   |   1 +
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/drm_format_helper.c           |  80 ++
 drivers/gpu/drm/tiny/Kconfig                  |  12 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/ssd130x.c                | 971 ++++++++++++++++++
 include/drm/drm_format_helper.h               |   7 +
 7 files changed, 1079 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ssd130x.c

-- 
2.34.1

[PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Javier Martinez Canillas]

Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
drivers that control monochromatic panels, that only have 1 bit per pixel.

The drm_fb_gray8_to_mono_reversed() helper was based on the function that
does the same in the drivers/gpu/drm/tiny/repaper.c driver.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |  7 +++
 2 files changed, 87 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 0f28dd2bdd72..cdce4b7c25d9 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_fb_blit_toio);
+
+static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
+{
+	unsigned int xb, i;
+
+	for (xb = 0; xb < pixels / 8; xb++) {
+		u8 byte = 0x00;
+
+		for (i = 0; i < 8; i++) {
+			int x = xb * 8 + i;
+
+			byte >>= 1;
+			if (src[x] >> 7)
+				byte |= BIT(7);
+		}
+		*dst++ = byte;
+	}
+}
+
+/**
+ * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @src: 8-bit grayscale source buffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome or grayscale support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
+ * helper function to convert to the native format.
+ */
+void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				   const struct drm_rect *clip)
+{
+
+	size_t height = drm_rect_height(clip);
+	size_t width = drm_rect_width(clip);
+	unsigned int y;
+	const u8 *gray8 = src;
+	u8 *mono = dst;
+
+	if (!dst_pitch)
+		dst_pitch = width;
+
+	for (y = 0; y < height; y++) {
+		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
+		mono += (dst_pitch / 8);
+		gray8 += dst_pitch;
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @src: XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use this function to convert to the native format.
+ *
+ * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
+ * then the result is converted from grayscale to reversed monohrome.
+ */
+void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				      const struct drm_framebuffer *fb,
+				      const struct drm_rect *clip)
+{
+	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
+		return;
+
+	if (!dst_pitch)
+		dst_pitch = drm_rect_width(clip);
+
+	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
+	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index b30ed5de0a33..85e551a5cbe6 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 		     const void *vmap, const struct drm_framebuffer *fb,
 		     const struct drm_rect *rect);
 
+void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				   const struct drm_rect *clip);
+
+void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				      const struct drm_framebuffer *fb,
+				      const struct drm_rect *clip);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.34.1

[PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888, gray8}_to_mono_reversed()

[Javier Martinez Canillas]

Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
drivers that control monochromatic panels, that only have 1 bit per pixel.

The drm_fb_gray8_to_mono_reversed() helper was based on the function that
does the same in the drivers/gpu/drm/tiny/repaper.c driver.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |  7 +++
 2 files changed, 87 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 0f28dd2bdd72..cdce4b7c25d9 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_fb_blit_toio);
+
+static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
+{
+	unsigned int xb, i;
+
+	for (xb = 0; xb < pixels / 8; xb++) {
+		u8 byte = 0x00;
+
+		for (i = 0; i < 8; i++) {
+			int x = xb * 8 + i;
+
+			byte >>= 1;
+			if (src[x] >> 7)
+				byte |= BIT(7);
+		}
+		*dst++ = byte;
+	}
+}
+
+/**
+ * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @src: 8-bit grayscale source buffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome or grayscale support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
+ * helper function to convert to the native format.
+ */
+void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				   const struct drm_rect *clip)
+{
+
+	size_t height = drm_rect_height(clip);
+	size_t width = drm_rect_width(clip);
+	unsigned int y;
+	const u8 *gray8 = src;
+	u8 *mono = dst;
+
+	if (!dst_pitch)
+		dst_pitch = width;
+
+	for (y = 0; y < height; y++) {
+		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
+		mono += (dst_pitch / 8);
+		gray8 += dst_pitch;
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @src: XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use this function to convert to the native format.
+ *
+ * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
+ * then the result is converted from grayscale to reversed monohrome.
+ */
+void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				      const struct drm_framebuffer *fb,
+				      const struct drm_rect *clip)
+{
+	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
+		return;
+
+	if (!dst_pitch)
+		dst_pitch = drm_rect_width(clip);
+
+	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
+	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index b30ed5de0a33..85e551a5cbe6 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 		     const void *vmap, const struct drm_framebuffer *fb,
 		     const struct drm_rect *rect);
 
+void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				   const struct drm_rect *clip);
+
+void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
+				      const struct drm_framebuffer *fb,
+				      const struct drm_rect *clip);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.34.1

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5985 bytes --]

Hi

Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
> Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
> drivers that control monochromatic panels, that only have 1 bit per pixel.
> 
> The drm_fb_gray8_to_mono_reversed() helper was based on the function that
> does the same in the drivers/gpu/drm/tiny/repaper.c driver.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  7 +++
>   2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..cdce4b7c25d9 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
> +{
> +	unsigned int xb, i;
> +
> +	for (xb = 0; xb < pixels / 8; xb++) {

In practice, all mode widths are multiples of 8 because VGA mandated it. 
So it's ok-ish to assume this here. You should probably at least print a 
warning somewhere if (pixels % 8 != 0)


> +		u8 byte = 0x00;
> +
> +		for (i = 0; i < 8; i++) {
> +			int x = xb * 8 + i;
> +
> +			byte >>= 1;
> +			if (src[x] >> 7)
> +				byte |= BIT(7);
> +		}
> +		*dst++ = byte;
> +	}
> +}
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				   const struct drm_rect *clip)

There's a bug here. You want to pass in a drm_framebuffer as fourth 
argument.

> +{
> +
> +	size_t height = drm_rect_height(clip);
> +	size_t width = drm_rect_width(clip);
> +	unsigned int y;
> +	const u8 *gray8 = src;
> +	u8 *mono = dst;
> +
> +	if (!dst_pitch)
> +		dst_pitch = width;

The dst_pitch is given in bytes. You have to device by 8. Here would be 
a good place to warn if (width % 8 != 0).

> +
> +	for (y = 0; y < height; y++) {
> +		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
> +		mono += (dst_pitch / 8);

The dst_pitch is already given in bytes.

> +		gray8 += dst_pitch;

'gray8 += fb->pitches[0]' would be correct.

> +	}
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use this function to convert to the native format.
> + *
> + * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
> + * then the result is converted from grayscale to reversed monohrome.
> + */
> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				      const struct drm_framebuffer *fb,
> +				      const struct drm_rect *clip)
> +{
> +	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
> +		return;
> +
> +	if (!dst_pitch)
> +		dst_pitch = drm_rect_width(clip);
> +
> +	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
> +	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);

Converting from dst into dst can give incorrect results. At some point 
we probably want to add restrict qualifiers to these pointers, to help 
the compiler with optimizing.

A better approach here is to pull the per-line conversion from 
drm_fb_xrgb8888_to_gray8() into a separate helper and implement a 
line-by-line conversion here. something like this:

   drm_fb_xrgb8888_to_mono_reversed()
   {
     char *tmp = kmalloc(size of a single line of gray8)

     for (heigth) {
        drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
        drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);

        src += fb->pitches[0]
        dst += dst_pitch;
     }

     kfree(tmp);
   }

Best regards
Thomas

> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index b30ed5de0a33..85e551a5cbe6 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   		     const void *vmap, const struct drm_framebuffer *fb,
>   		     const struct drm_rect *rect);
>   
> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				   const struct drm_rect *clip);
> +
> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				      const struct drm_framebuffer *fb,
> +				      const struct drm_rect *clip);
> +
>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5985 bytes --]

Hi

Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
> Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
> drivers that control monochromatic panels, that only have 1 bit per pixel.
> 
> The drm_fb_gray8_to_mono_reversed() helper was based on the function that
> does the same in the drivers/gpu/drm/tiny/repaper.c driver.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  7 +++
>   2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..cdce4b7c25d9 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
> +{
> +	unsigned int xb, i;
> +
> +	for (xb = 0; xb < pixels / 8; xb++) {

In practice, all mode widths are multiples of 8 because VGA mandated it. 
So it's ok-ish to assume this here. You should probably at least print a 
warning somewhere if (pixels % 8 != 0)


> +		u8 byte = 0x00;
> +
> +		for (i = 0; i < 8; i++) {
> +			int x = xb * 8 + i;
> +
> +			byte >>= 1;
> +			if (src[x] >> 7)
> +				byte |= BIT(7);
> +		}
> +		*dst++ = byte;
> +	}
> +}
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				   const struct drm_rect *clip)

There's a bug here. You want to pass in a drm_framebuffer as fourth 
argument.

> +{
> +
> +	size_t height = drm_rect_height(clip);
> +	size_t width = drm_rect_width(clip);
> +	unsigned int y;
> +	const u8 *gray8 = src;
> +	u8 *mono = dst;
> +
> +	if (!dst_pitch)
> +		dst_pitch = width;

The dst_pitch is given in bytes. You have to device by 8. Here would be 
a good place to warn if (width % 8 != 0).

> +
> +	for (y = 0; y < height; y++) {
> +		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
> +		mono += (dst_pitch / 8);

The dst_pitch is already given in bytes.

> +		gray8 += dst_pitch;

'gray8 += fb->pitches[0]' would be correct.

> +	}
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use this function to convert to the native format.
> + *
> + * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
> + * then the result is converted from grayscale to reversed monohrome.
> + */
> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				      const struct drm_framebuffer *fb,
> +				      const struct drm_rect *clip)
> +{
> +	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
> +		return;
> +
> +	if (!dst_pitch)
> +		dst_pitch = drm_rect_width(clip);
> +
> +	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
> +	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);

Converting from dst into dst can give incorrect results. At some point 
we probably want to add restrict qualifiers to these pointers, to help 
the compiler with optimizing.

A better approach here is to pull the per-line conversion from 
drm_fb_xrgb8888_to_gray8() into a separate helper and implement a 
line-by-line conversion here. something like this:

   drm_fb_xrgb8888_to_mono_reversed()
   {
     char *tmp = kmalloc(size of a single line of gray8)

     for (heigth) {
        drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
        drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);

        src += fb->pitches[0]
        dst += dst_pitch;
     }

     kfree(tmp);
   }

Best regards
Thomas

> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index b30ed5de0a33..85e551a5cbe6 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   		     const void *vmap, const struct drm_framebuffer *fb,
>   		     const struct drm_rect *rect);
>   
> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				   const struct drm_rect *clip);
> +
> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> +				      const struct drm_framebuffer *fb,
> +				      const struct drm_rect *clip);
> +
>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3788 bytes --]


Am 04.02.22 um 16:52 schrieb Thomas Zimmermann:
[...]
>> +/**
>> + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed 
>> monochrome
>> + * @dst: reversed monochrome destination buffer
>> + * @dst_pitch: Number of bytes between two consecutive scanlines 
>> within dst
>> + * @src: XRGB8888 source buffer
>> + * @fb: DRM framebuffer
>> + * @clip: Clip rectangle area to copy
>> + *
>> + * DRM doesn't have native monochrome support.
>> + * Such drivers can announce the commonly supported XR24 format to 
>> userspace
>> + * and use this function to convert to the native format.
>> + *
>> + * This function uses drm_fb_xrgb8888_to_gray8() to convert to 
>> grayscale and
>> + * then the result is converted from grayscale to reversed monohrome.
>> + */
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int 
>> dst_pitch, const void *src,
>> +                      const struct drm_framebuffer *fb,
>> +                      const struct drm_rect *clip)
>> +{
>> +    if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>> +        return;
>> +
>> +    if (!dst_pitch)
>> +        dst_pitch = drm_rect_width(clip);
>> +
>> +    drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>> +    drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
> 
> Converting from dst into dst can give incorrect results. At some point 
> we probably want to add restrict qualifiers to these pointers, to help 
> the compiler with optimizing.
> 
> A better approach here is to pull the per-line conversion from 
> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a 
> line-by-line conversion here. something like this:
> 
>    drm_fb_xrgb8888_to_mono_reversed()
>    {
>      char *tmp = kmalloc(size of a single line of gray8)
> 
>      for (heigth) {
>         drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
>         drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);

Here, I meant 'drm_fb_gray8_to_mono_reversed_line()'

> 
>         src += fb->pitches[0]
>         dst += dst_pitch;
>      }
> 
>      kfree(tmp);
>    }

To elaborate, this is an example of what I meant by composable. In the 
future, we can generalize this function and hand-in 2 function pointers 
the do the conversion with tmp as intermediate buffer. That would work 
for any combination of formats that have a common intermediate format.

> 
> Best regards
> Thomas
> 
>> +}
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
>> diff --git a/include/drm/drm_format_helper.h 
>> b/include/drm/drm_format_helper.h
>> index b30ed5de0a33..85e551a5cbe6 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned 
>> int dst_pitch, uint32_t dst_for
>>                const void *vmap, const struct drm_framebuffer *fb,
>>                const struct drm_rect *rect);
>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, 
>> const void *src,
>> +                   const struct drm_rect *clip);
>> +
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int 
>> dst_pitch, const void *src,
>> +                      const struct drm_framebuffer *fb,
>> +                      const struct drm_rect *clip);
>> +
>>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for your feedback.

On 2/4/22 16:52, Thomas Zimmermann wrote:

[snip]

>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>> +{
>> +	unsigned int xb, i;
>> +
>> +	for (xb = 0; xb < pixels / 8; xb++) {
> 
> In practice, all mode widths are multiples of 8 because VGA mandated it. 
> So it's ok-ish to assume this here. You should probably at least print a 
> warning somewhere if (pixels % 8 != 0)
>

Agreed.
 
[snip]

>> + * DRM doesn't have native monochrome or grayscale support.
>> + * Such drivers can announce the commonly supported XR24 format to userspace
>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>> + * helper function to convert to the native format.
>> + */
>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>> +				   const struct drm_rect *clip)
> 
> There's a bug here. You want to pass in a drm_framebuffer as fourth 
> argument.
>
>> +{
>> +
>> +	size_t height = drm_rect_height(clip);
>> +	size_t width = drm_rect_width(clip);
>> +	unsigned int y;
>> +	const u8 *gray8 = src;
>> +	u8 *mono = dst;
>> +
>> +	if (!dst_pitch)
>> +		dst_pitch = width;
> 
> The dst_pitch is given in bytes. You have to device by 8. Here would be 
> a good place to warn if (width % 8 != 0).
>

Ok.
 
>> +
>> +	for (y = 0; y < height; y++) {
>> +		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
>> +		mono += (dst_pitch / 8);
> 
> The dst_pitch is already given in bytes.
>

Yes, I know but for reversed mono we want only 1/8 of the width since we
are converting from 8 bits per pixel greyscale to 1 bit per pixel mono.

Or am I misunderstanding what you meant ?

>> +		gray8 += dst_pitch;
> 
> 'gray8 += fb->pitches[0]' would be correct.
>

Ok.
 
[snip]

>> + */
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>> +				      const struct drm_framebuffer *fb,
>> +				      const struct drm_rect *clip)
>> +{
>> +	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>> +		return;
>> +
>> +	if (!dst_pitch)
>> +		dst_pitch = drm_rect_width(clip);
>> +
>> +	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>> +	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
> 
> Converting from dst into dst can give incorrect results. At some point 
> we probably want to add restrict qualifiers to these pointers, to help 
> the compiler with optimizing.
> 
> A better approach here is to pull the per-line conversion from 
> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a 
> line-by-line conversion here. something like this:
> 
>    drm_fb_xrgb8888_to_mono_reversed()
>    {
>      char *tmp = kmalloc(size of a single line of gray8)
> 
>      for (heigth) {
>         drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
>         drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
> 
>         src += fb->pitches[0]
>         dst += dst_pitch;
>      }
> 
>      kfree(tmp);
>    }
>

I see. Yes, that sounds a much better approach. I'll change it in v3.
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for your feedback.

On 2/4/22 16:52, Thomas Zimmermann wrote:

[snip]

>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>> +{
>> +	unsigned int xb, i;
>> +
>> +	for (xb = 0; xb < pixels / 8; xb++) {
> 
> In practice, all mode widths are multiples of 8 because VGA mandated it. 
> So it's ok-ish to assume this here. You should probably at least print a 
> warning somewhere if (pixels % 8 != 0)
>

Agreed.
 
[snip]

>> + * DRM doesn't have native monochrome or grayscale support.
>> + * Such drivers can announce the commonly supported XR24 format to userspace
>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>> + * helper function to convert to the native format.
>> + */
>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>> +				   const struct drm_rect *clip)
> 
> There's a bug here. You want to pass in a drm_framebuffer as fourth 
> argument.
>
>> +{
>> +
>> +	size_t height = drm_rect_height(clip);
>> +	size_t width = drm_rect_width(clip);
>> +	unsigned int y;
>> +	const u8 *gray8 = src;
>> +	u8 *mono = dst;
>> +
>> +	if (!dst_pitch)
>> +		dst_pitch = width;
> 
> The dst_pitch is given in bytes. You have to device by 8. Here would be 
> a good place to warn if (width % 8 != 0).
>

Ok.
 
>> +
>> +	for (y = 0; y < height; y++) {
>> +		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
>> +		mono += (dst_pitch / 8);
> 
> The dst_pitch is already given in bytes.
>

Yes, I know but for reversed mono we want only 1/8 of the width since we
are converting from 8 bits per pixel greyscale to 1 bit per pixel mono.

Or am I misunderstanding what you meant ?

>> +		gray8 += dst_pitch;
> 
> 'gray8 += fb->pitches[0]' would be correct.
>

Ok.
 
[snip]

>> + */
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>> +				      const struct drm_framebuffer *fb,
>> +				      const struct drm_rect *clip)
>> +{
>> +	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>> +		return;
>> +
>> +	if (!dst_pitch)
>> +		dst_pitch = drm_rect_width(clip);
>> +
>> +	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>> +	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
> 
> Converting from dst into dst can give incorrect results. At some point 
> we probably want to add restrict qualifiers to these pointers, to help 
> the compiler with optimizing.
> 
> A better approach here is to pull the per-line conversion from 
> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a 
> line-by-line conversion here. something like this:
> 
>    drm_fb_xrgb8888_to_mono_reversed()
>    {
>      char *tmp = kmalloc(size of a single line of gray8)
> 
>      for (heigth) {
>         drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
>         drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
> 
>         src += fb->pitches[0]
>         dst += dst_pitch;
>      }
> 
>      kfree(tmp);
>    }
>

I see. Yes, that sounds a much better approach. I'll change it in v3.
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4160 bytes --]

Hi

Am 04.02.22 um 20:31 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> Thanks a lot for your feedback.
> 
> On 2/4/22 16:52, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>>> +{
>>> +	unsigned int xb, i;
>>> +
>>> +	for (xb = 0; xb < pixels / 8; xb++) {
>>
>> In practice, all mode widths are multiples of 8 because VGA mandated it.
>> So it's ok-ish to assume this here. You should probably at least print a
>> warning somewhere if (pixels % 8 != 0)
>>
> 
> Agreed.

After sending the mail, I realized that some code copies only parts of 
the source around; specifically for damage handling. None of this is 
aligned to multiple of 8. So the copying could start and end in the 
middle of bytes. You'd need a pixel-offset value of some sort.

If you don't want to handle this now, maybe at least detect this case 
and put a warning somewhere.

>   
> [snip]
> 
>>> + * DRM doesn't have native monochrome or grayscale support.
>>> + * Such drivers can announce the commonly supported XR24 format to userspace
>>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>>> + * helper function to convert to the native format.
>>> + */
>>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>>> +				   const struct drm_rect *clip)
>>
>> There's a bug here. You want to pass in a drm_framebuffer as fourth
>> argument.
>>
>>> +{
>>> +
>>> +	size_t height = drm_rect_height(clip);
>>> +	size_t width = drm_rect_width(clip);
>>> +	unsigned int y;
>>> +	const u8 *gray8 = src;
>>> +	u8 *mono = dst;
>>> +
>>> +	if (!dst_pitch)
>>> +		dst_pitch = width;
>>
>> The dst_pitch is given in bytes. You have to device by 8. Here would be
>> a good place to warn if (width % 8 != 0).
>>
> 
> Ok.
>   
>>> +
>>> +	for (y = 0; y < height; y++) {
>>> +		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
>>> +		mono += (dst_pitch / 8);
>>
>> The dst_pitch is already given in bytes.
>>
> 
> Yes, I know but for reversed mono we want only 1/8 of the width since we
> are converting from 8 bits per pixel greyscale to 1 bit per pixel mono.
> 
> Or am I misunderstanding what you meant ?

I mean that if there are 80 pixel on a scanline, the value of dst_pitch 
is already 10. These pitch values are always given in bytes.

Best regards
Thomas

> 
>>> +		gray8 += dst_pitch;
>>
>> 'gray8 += fb->pitches[0]' would be correct.
>>
> 
> Ok.
>   
> [snip]
> 
>>> + */
>>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>>> +				      const struct drm_framebuffer *fb,
>>> +				      const struct drm_rect *clip)
>>> +{
>>> +	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>>> +		return;
>>> +
>>> +	if (!dst_pitch)
>>> +		dst_pitch = drm_rect_width(clip);
>>> +
>>> +	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>>> +	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
>>
>> Converting from dst into dst can give incorrect results. At some point
>> we probably want to add restrict qualifiers to these pointers, to help
>> the compiler with optimizing.
>>
>> A better approach here is to pull the per-line conversion from
>> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a
>> line-by-line conversion here. something like this:
>>
>>     drm_fb_xrgb8888_to_mono_reversed()
>>     {
>>       char *tmp = kmalloc(size of a single line of gray8)
>>
>>       for (heigth) {
>>          drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
>>          drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
>>
>>          src += fb->pitches[0]
>>          dst += dst_pitch;
>>       }
>>
>>       kfree(tmp);
>>     }
>>
> 
> I see. Yes, that sounds a much better approach. I'll change it in v3.
>   
> Best regards,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4160 bytes --]

Hi

Am 04.02.22 um 20:31 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> Thanks a lot for your feedback.
> 
> On 2/4/22 16:52, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>>> +{
>>> +	unsigned int xb, i;
>>> +
>>> +	for (xb = 0; xb < pixels / 8; xb++) {
>>
>> In practice, all mode widths are multiples of 8 because VGA mandated it.
>> So it's ok-ish to assume this here. You should probably at least print a
>> warning somewhere if (pixels % 8 != 0)
>>
> 
> Agreed.

After sending the mail, I realized that some code copies only parts of 
the source around; specifically for damage handling. None of this is 
aligned to multiple of 8. So the copying could start and end in the 
middle of bytes. You'd need a pixel-offset value of some sort.

If you don't want to handle this now, maybe at least detect this case 
and put a warning somewhere.

>   
> [snip]
> 
>>> + * DRM doesn't have native monochrome or grayscale support.
>>> + * Such drivers can announce the commonly supported XR24 format to userspace
>>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>>> + * helper function to convert to the native format.
>>> + */
>>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>>> +				   const struct drm_rect *clip)
>>
>> There's a bug here. You want to pass in a drm_framebuffer as fourth
>> argument.
>>
>>> +{
>>> +
>>> +	size_t height = drm_rect_height(clip);
>>> +	size_t width = drm_rect_width(clip);
>>> +	unsigned int y;
>>> +	const u8 *gray8 = src;
>>> +	u8 *mono = dst;
>>> +
>>> +	if (!dst_pitch)
>>> +		dst_pitch = width;
>>
>> The dst_pitch is given in bytes. You have to device by 8. Here would be
>> a good place to warn if (width % 8 != 0).
>>
> 
> Ok.
>   
>>> +
>>> +	for (y = 0; y < height; y++) {
>>> +		drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
>>> +		mono += (dst_pitch / 8);
>>
>> The dst_pitch is already given in bytes.
>>
> 
> Yes, I know but for reversed mono we want only 1/8 of the width since we
> are converting from 8 bits per pixel greyscale to 1 bit per pixel mono.
> 
> Or am I misunderstanding what you meant ?

I mean that if there are 80 pixel on a scanline, the value of dst_pitch 
is already 10. These pitch values are always given in bytes.

Best regards
Thomas

> 
>>> +		gray8 += dst_pitch;
>>
>> 'gray8 += fb->pitches[0]' would be correct.
>>
> 
> Ok.
>   
> [snip]
> 
>>> + */
>>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>>> +				      const struct drm_framebuffer *fb,
>>> +				      const struct drm_rect *clip)
>>> +{
>>> +	if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>>> +		return;
>>> +
>>> +	if (!dst_pitch)
>>> +		dst_pitch = drm_rect_width(clip);
>>> +
>>> +	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>>> +	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
>>
>> Converting from dst into dst can give incorrect results. At some point
>> we probably want to add restrict qualifiers to these pointers, to help
>> the compiler with optimizing.
>>
>> A better approach here is to pull the per-line conversion from
>> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a
>> line-by-line conversion here. something like this:
>>
>>     drm_fb_xrgb8888_to_mono_reversed()
>>     {
>>       char *tmp = kmalloc(size of a single line of gray8)
>>
>>       for (heigth) {
>>          drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
>>          drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
>>
>>          src += fb->pitches[0]
>>          dst += dst_pitch;
>>       }
>>
>>       kfree(tmp);
>>     }
>>
> 
> I see. Yes, that sounds a much better approach. I'll change it in v3.
>   
> Best regards,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed()

[Ilia Mirkin]

On Fri, Feb 4, 2022 at 10:53 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
> > Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
> > drivers that control monochromatic panels, that only have 1 bit per pixel.
> >
> > The drm_fb_gray8_to_mono_reversed() helper was based on the function that
> > does the same in the drivers/gpu/drm/tiny/repaper.c driver.
> >
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> >
> > (no changes since v1)
> >
> >   drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
> >   include/drm/drm_format_helper.h     |  7 +++
> >   2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index 0f28dd2bdd72..cdce4b7c25d9 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> >       return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(drm_fb_blit_toio);
> > +
> > +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
> > +{
> > +     unsigned int xb, i;
> > +
> > +     for (xb = 0; xb < pixels / 8; xb++) {
>
> In practice, all mode widths are multiples of 8 because VGA mandated it.
> So it's ok-ish to assume this here. You should probably at least print a
> warning somewhere if (pixels % 8 != 0)

Not sure if it's relevant, but 1366x768 was a fairly popular laptop
resolution. There's even a dedicated drm_mode_fixup_1366x768 in
drm_edid.c. (Would it have killed them to add 2 more horizontal
pixels? Apparently.)

Cheers,

  -ilia

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888, gray8}_to_mono_reversed()

[Ilia Mirkin]

On Fri, Feb 4, 2022 at 10:53 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
> > Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
> > drivers that control monochromatic panels, that only have 1 bit per pixel.
> >
> > The drm_fb_gray8_to_mono_reversed() helper was based on the function that
> > does the same in the drivers/gpu/drm/tiny/repaper.c driver.
> >
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> >
> > (no changes since v1)
> >
> >   drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
> >   include/drm/drm_format_helper.h     |  7 +++
> >   2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index 0f28dd2bdd72..cdce4b7c25d9 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> >       return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(drm_fb_blit_toio);
> > +
> > +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
> > +{
> > +     unsigned int xb, i;
> > +
> > +     for (xb = 0; xb < pixels / 8; xb++) {
>
> In practice, all mode widths are multiples of 8 because VGA mandated it.
> So it's ok-ish to assume this here. You should probably at least print a
> warning somewhere if (pixels % 8 != 0)

Not sure if it's relevant, but 1366x768 was a fairly popular laptop
resolution. There's even a dedicated drm_mode_fixup_1366x768 in
drm_edid.c. (Would it have killed them to add 2 more horizontal
pixels? Apparently.)

Cheers,

  -ilia

Re: [PATCH v2 1/4] drm/format-helper: Add drm_fb_{xrgb8888, gray8}_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2204 bytes --]

Hi

Am 04.02.22 um 22:02 schrieb Ilia Mirkin:
> On Fri, Feb 4, 2022 at 10:53 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
>>> Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
>>> drivers that control monochromatic panels, that only have 1 bit per pixel.
>>>
>>> The drm_fb_gray8_to_mono_reversed() helper was based on the function that
>>> does the same in the drivers/gpu/drm/tiny/repaper.c driver.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>    drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
>>>    include/drm/drm_format_helper.h     |  7 +++
>>>    2 files changed, 87 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>>> index 0f28dd2bdd72..cdce4b7c25d9 100644
>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>> @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>>        return -EINVAL;
>>>    }
>>>    EXPORT_SYMBOL(drm_fb_blit_toio);
>>> +
>>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>>> +{
>>> +     unsigned int xb, i;
>>> +
>>> +     for (xb = 0; xb < pixels / 8; xb++) {
>>
>> In practice, all mode widths are multiples of 8 because VGA mandated it.
>> So it's ok-ish to assume this here. You should probably at least print a
>> warning somewhere if (pixels % 8 != 0)
> 
> Not sure if it's relevant, but 1366x768 was a fairly popular laptop
> resolution. There's even a dedicated drm_mode_fixup_1366x768 in
> drm_edid.c. (Would it have killed them to add 2 more horizontal
> pixels? Apparently.)

D'oh!

Do you know how the text console looks in this mode? Fonts still expect 
a multiple of 8.

Best regards
Thomas

> 
> Cheers,
> 
>    -ilia

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas]

Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
controllers that can be programmed via an I2C interface. This is a port
of the ssd1307fb driver that already supports these devices.

A Device Tree binding is not added because the DRM driver is compatible
with the existing binding for the ssd1307fb driver.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/gpu/drm/tiny/Kconfig   |  12 +
 drivers/gpu/drm/tiny/Makefile  |   1 +
 drivers/gpu/drm/tiny/ssd130x.c | 971 +++++++++++++++++++++++++++++++++
 3 files changed, 984 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ssd130x.c

diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 712e0004e96e..1b6f5aa41d69 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -67,6 +67,18 @@ config DRM_SIMPLEDRM
 	  On x86 BIOS or UEFI systems, you should also select SYSFB_SIMPLEFB
 	  to use UEFI and VESA framebuffers.
 
+config DRM_SSD130X
+	tristate "DRM support for Solomon SSD130X OLED displays"
+	depends on DRM && I2C
+	select BACKLIGHT_CLASS_DEVICE
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	help
+	  DRM driver for the SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
+	  OLED controllers that can be programmed via an I2C interface.
+
+	  If M is selected the module will be called ssd130x.
+
 config TINYDRM_HX8357D
 	tristate "DRM support for HX8357D display panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 5d5505d40e7b..18c3557dcb71 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
 obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
 obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
+obj-$(CONFIG_DRM_SSD130X)		+= ssd130x.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9163)		+= ili9163.o
 obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
diff --git a/drivers/gpu/drm/tiny/ssd130x.c b/drivers/gpu/drm/tiny/ssd130x.c
new file mode 100644
index 000000000000..b348768529dc
--- /dev/null
+++ b/drivers/gpu/drm/tiny/ssd130x.c
@@ -0,0 +1,971 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DRM driver for Solomon SSD130X OLED displays
+ *
+ * Copyright 2022 Red Hat Inc.
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons
+ *
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_rect.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#define DRIVER_NAME	"ssd130x"
+#define DRIVER_DESC	"DRM driver for Solomon SSD130X OLED displays"
+#define DRIVER_DATE	"20220131"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+#define SSD130X_DATA				0x40
+#define SSD130X_COMMAND			0x80
+
+#define SSD130X_SET_ADDRESS_MODE		0x20
+#define SSD130X_SET_ADDRESS_MODE_HORIZONTAL	(0x00)
+#define SSD130X_SET_ADDRESS_MODE_VERTICAL	(0x01)
+#define SSD130X_SET_ADDRESS_MODE_PAGE		(0x02)
+#define SSD130X_SET_COL_RANGE			0x21
+#define SSD130X_SET_PAGE_RANGE			0x22
+#define SSD130X_CONTRAST			0x81
+#define SSD130X_SET_LOOKUP_TABLE		0x91
+#define SSD130X_CHARGE_PUMP			0x8d
+#define SSD130X_SEG_REMAP_ON			0xa1
+#define SSD130X_DISPLAY_OFF			0xae
+#define SSD130X_SET_MULTIPLEX_RATIO		0xa8
+#define SSD130X_DISPLAY_ON			0xaf
+#define SSD130X_START_PAGE_ADDRESS		0xb0
+#define SSD130X_SET_DISPLAY_OFFSET		0xd3
+#define SSD130X_SET_CLOCK_FREQ			0xd5
+#define SSD130X_SET_AREA_COLOR_MODE		0xd8
+#define SSD130X_SET_PRECHARGE_PERIOD		0xd9
+#define SSD130X_SET_COM_PINS_CONFIG		0xda
+#define SSD130X_SET_VCOMH			0xdb
+
+#define MAX_CONTRAST 255
+
+struct ssd130x_deviceinfo {
+	u32 default_vcomh;
+	u32 default_dclk_div;
+	u32 default_dclk_frq;
+	int need_pwm;
+	int need_chargepump;
+};
+
+struct ssd130x_device {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	struct drm_display_mode mode;
+	struct drm_connector connector;
+	struct i2c_client *client;
+
+	const struct ssd130x_deviceinfo *device_info;
+
+	unsigned area_color_enable : 1;
+	unsigned com_invdir : 1;
+	unsigned com_lrremap : 1;
+	unsigned com_seq : 1;
+	unsigned lookup_table_set : 1;
+	unsigned low_power : 1;
+	unsigned seg_remap : 1;
+	u32 com_offset;
+	u32 contrast;
+	u32 dclk_div;
+	u32 dclk_frq;
+	u32 height;
+	u8 lookup_table[4];
+	u32 page_offset;
+	u32 col_offset;
+	u32 prechargep1;
+	u32 prechargep2;
+
+	struct backlight_device *bl_dev;
+	struct pwm_device *pwm;
+	struct gpio_desc *reset;
+	struct regulator *vbat_reg;
+	u32 vcomh;
+	u32 width;
+	/* Cached address ranges */
+	u8 col_start;
+	u8 col_end;
+	u8 page_start;
+	u8 page_end;
+};
+
+struct ssd130x_i2c_msg {
+	u8	cmd;
+	u8	data[];
+};
+
+static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
+{
+	return container_of(drm, struct ssd130x_device, drm);
+}
+
+static struct ssd130x_i2c_msg *ssd130x_alloc_msg(u32 len, u8 cmd)
+{
+	struct ssd130x_i2c_msg *msg;
+
+	msg = kzalloc(struct_size(msg, data, len), GFP_KERNEL);
+	if (!msg)
+		return NULL;
+
+	msg->cmd = cmd;
+
+	return msg;
+}
+
+static int ssd130x_write_msg(struct i2c_client *client,
+			     struct ssd130x_i2c_msg *msg, u32 len)
+{
+	int ret;
+
+	len += sizeof(*msg);
+
+	ret = i2c_master_send(client, (u8 *)msg, len);
+	if (ret != len) {
+		dev_err(&client->dev, "Couldn't send I2C command.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static inline int ssd130x_write_value(struct i2c_client *client, u8 value)
+{
+	struct ssd130x_i2c_msg *msg;
+	int ret;
+
+	msg = ssd130x_alloc_msg(1, SSD130X_COMMAND);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->data[0] = value;
+
+	ret = ssd130x_write_msg(client, msg, 1);
+	kfree(msg);
+
+	return ret;
+}
+
+static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
+				    /* u8 cmd, u8 value, ... */...)
+{
+	va_list ap;
+	u8 value;
+	int ret;
+
+	va_start(ap, count);
+
+	while (count--) {
+		value = va_arg(ap, int);
+		ret = ssd130x_write_value(client, (u8)value);
+		if (ret)
+			goto out_end;
+	}
+
+out_end:
+	va_end(ap);
+
+	return ret;
+}
+
+static int ssd130x_set_col_range(struct ssd130x_device *ssd130x,
+				 u8 col_start, u8 cols)
+{
+	u8 col_end = col_start + cols - 1;
+	int ret;
+
+	if (col_start == ssd130x->col_start && col_end == ssd130x->col_end)
+		return 0;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 3, SSD130X_SET_COL_RANGE,
+				col_start, col_end);
+	if (ret < 0)
+		return ret;
+
+	ssd130x->col_start = col_start;
+	ssd130x->col_end = col_end;
+	return 0;
+}
+
+static int ssd130x_set_page_range(struct ssd130x_device *ssd130x,
+				  u8 page_start, u8 pages)
+{
+	u8 page_end = page_start + pages - 1;
+	int ret;
+
+	if (page_start == ssd130x->page_start && page_end == ssd130x->page_end)
+		return 0;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 3, SSD130X_SET_PAGE_RANGE,
+				page_start, page_end);
+	if (ret < 0)
+		return ret;
+
+	ssd130x->page_start = page_start;
+	ssd130x->page_end = page_end;
+	return 0;
+}
+
+static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+	struct pwm_state pwmstate;
+
+	ssd130x->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(ssd130x->pwm)) {
+		dev_err(dev, "Could not get PWM from device tree!\n");
+		return PTR_ERR(ssd130x->pwm);
+	}
+
+	pwm_init_state(ssd130x->pwm, &pwmstate);
+	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
+	pwm_apply_state(ssd130x->pwm, &pwmstate);
+
+	/* Enable the PWM */
+	pwm_enable(ssd130x->pwm);
+
+	dev_dbg(dev, "Using PWM%d with a %lluns period.\n",
+		ssd130x->pwm->pwm, pwm_get_period(ssd130x->pwm));
+
+	return 0;
+}
+
+static void ssd130x_reset(struct ssd130x_device *ssd130x)
+{
+	/* Reset the screen */
+	gpiod_set_value_cansleep(ssd130x->reset, 1);
+	udelay(4);
+	gpiod_set_value_cansleep(ssd130x->reset, 0);
+	udelay(4);
+}
+
+static int ssd130x_poweron(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+	int ret;
+
+	if (ssd130x->reset)
+		ssd130x_reset(ssd130x);
+
+	if (ssd130x->vbat_reg) {
+		ret = regulator_enable(ssd130x->vbat_reg);
+		if (ret) {
+			dev_err(dev, "Failed to enable VBAT: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (ssd130x->device_info->need_pwm) {
+		ret = ssd130x_pwm_enable(ssd130x);
+		if (ret) {
+			dev_err(dev, "Failed to enable PWM: %d\n", ret);
+			if (ssd130x->vbat_reg)
+				regulator_disable(ssd130x->vbat_reg);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ssd130x_poweroff(struct ssd130x_device *ssd130x)
+{
+	if (ssd130x->device_info->need_pwm) {
+		pwm_disable(ssd130x->pwm);
+		pwm_put(ssd130x->pwm);
+	}
+
+	if (ssd130x->vbat_reg)
+		regulator_disable(ssd130x->vbat_reg);
+}
+
+static int ssd130x_init(struct ssd130x_device *ssd130x)
+{
+	u32 precharge, dclk, com_invdir, compins, chargepump;
+	int ret;
+
+	/* Set initial contrast */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
+	/* Set segment re-map */
+	if (ssd130x->seg_remap) {
+		ret = ssd130x_write_cmd(ssd130x->client, 1, SSD130X_SEG_REMAP_ON);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set COM direction */
+	com_invdir = 0xc0 | ssd130x->com_invdir << 3;
+	ret = ssd130x_write_cmd(ssd130x->client,  1, com_invdir);
+	if (ret < 0)
+		return ret;
+
+	/* Set multiplex ratio value */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_MULTIPLEX_RATIO,
+				ssd130x->height - 1);
+	if (ret < 0)
+		return ret;
+
+	/* set display offset value */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_DISPLAY_OFFSET,
+				ssd130x->com_offset);
+	if (ret < 0)
+		return ret;
+
+	/* Set clock frequency */
+	dclk = ((ssd130x->dclk_div - 1) & 0xf) | (ssd130x->dclk_frq & 0xf) << 4;
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_CLOCK_FREQ, dclk);
+	if (ret < 0)
+		return ret;
+
+	/* Set Area Color Mode ON/OFF & Low Power Display Mode */
+	if (ssd130x->area_color_enable || ssd130x->low_power) {
+		u32 mode = ((ssd130x->area_color_enable ? 0x30 : 0) |
+			    (ssd130x->low_power ? 5 : 0));
+
+		ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_AREA_COLOR_MODE, mode);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set precharge period in number of ticks from the internal clock */
+	precharge = (ssd130x->prechargep1 & 0xf) | (ssd130x->prechargep2 & 0xf) << 4;
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_PRECHARGE_PERIOD, precharge);
+	if (ret < 0)
+		return ret;
+
+	/* Set COM pins configuration */
+	compins = 0x02 | !ssd130x->com_seq << 4 | ssd130x->com_lrremap << 5;
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_COM_PINS_CONFIG, compins);
+	if (ret < 0)
+		return ret;
+
+
+	/* Set VCOMH */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_VCOMH, ssd130x->vcomh);
+	if (ret < 0)
+		return ret;
+
+	/* Turn on the DC-DC Charge Pump */
+	chargepump = BIT(4) | (ssd130x->device_info->need_chargepump ? BIT(2) : 0);
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CHARGE_PUMP, chargepump);
+	if (ret < 0)
+		return ret;
+
+	/* Set lookup table */
+	if (ssd130x->lookup_table_set) {
+		int i;
+
+		ret = ssd130x_write_cmd(ssd130x->client, 1, SSD130X_SET_LOOKUP_TABLE);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {
+			u8 val = ssd130x->lookup_table[i];
+
+			if (val < 31 || val > 63)
+				dev_warn(&ssd130x->client->dev,
+					 "lookup table index %d value out of range 31 <= %d <= 63\n",
+					 i, val);
+			ret = ssd130x_write_cmd(ssd130x->client, 1, val);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	/* Switch to horizontal addressing mode */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
+				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int ssd130x_update_display(struct ssd130x_device *ssd130x, u8 *buf,
+				  u32 width, u32 height)
+{
+	struct ssd130x_i2c_msg *msg;
+	unsigned int line_length = DIV_ROUND_UP(width, 8);
+	unsigned int pages = DIV_ROUND_UP(height, 8);
+	u32 array_idx = 0;
+	int ret, i, j, k;
+
+	msg = ssd130x_alloc_msg(width * pages, SSD130X_DATA);
+	if (!msg)
+		return -ENOMEM;
+
+	/*
+	 * The screen is divided in pages, each having a height of 8
+	 * pixels, and the width of the screen. When sending a byte of
+	 * data to the controller, it gives the 8 bits for the current
+	 * column. I.e, the first byte are the 8 bits of the first
+	 * column, then the 8 bits for the second column, etc.
+	 *
+	 *
+	 * Representation of the screen, assuming it is 5 bits
+	 * wide. Each letter-number combination is a bit that controls
+	 * one pixel.
+	 *
+	 * A0 A1 A2 A3 A4
+	 * B0 B1 B2 B3 B4
+	 * C0 C1 C2 C3 C4
+	 * D0 D1 D2 D3 D4
+	 * E0 E1 E2 E3 E4
+	 * F0 F1 F2 F3 F4
+	 * G0 G1 G2 G3 G4
+	 * H0 H1 H2 H3 H4
+	 *
+	 * If you want to update this screen, you need to send 5 bytes:
+	 *  (1) A0 B0 C0 D0 E0 F0 G0 H0
+	 *  (2) A1 B1 C1 D1 E1 F1 G1 H1
+	 *  (3) A2 B2 C2 D2 E2 F2 G2 H2
+	 *  (4) A3 B3 C3 D3 E3 F3 G3 H3
+	 *  (5) A4 B4 C4 D4 E4 F4 G4 H4
+	 */
+
+	ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset, width);
+	if (ret < 0)
+		goto out_free;
+
+	ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset / 8, pages);
+	if (ret < 0)
+		goto out_free;
+
+	for (i = 0; i < pages; i++) {
+		int m = 8;
+
+		/* Last page may be partial */
+		if (8 * (i + 1) > ssd130x->height)
+			m = ssd130x->height % 8;
+		for (j = 0; j < width; j++) {
+			u8 data = 0;
+
+			for (k = 0; k < m; k++) {
+				u8 byte = buf[(8 * i + k) * line_length +
+					       j / 8];
+				u8 bit = (byte >> (j % 8)) & 1;
+
+				data |= bit << k;
+			}
+			msg->data[array_idx++] = data;
+		}
+	}
+
+	ret = ssd130x_write_msg(ssd130x->client, msg, width * pages);
+
+out_free:
+	kfree(msg);
+	return ret;
+}
+
+static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, u32 width, u32 height)
+{
+	u8 *buf = NULL;
+
+	buf = kcalloc(width, height, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ssd130x_update_display(ssd130x, buf, width, height);
+
+	kfree(buf);
+}
+
+static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
+				struct drm_rect *rect)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+	int ret = 0;
+	u8 *buf = NULL;
+
+	buf = kcalloc(fb->width, fb->height, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
+
+	ssd130x_update_display(ssd130x, buf, fb->width, fb->height);
+
+	kfree(buf);
+
+	return ret;
+}
+
+static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					   const struct drm_display_mode *mode)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+
+	if (mode->hdisplay != ssd130x->mode.hdisplay &&
+	    mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_SIZE;
+	else if (mode->hdisplay != ssd130x->mode.hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_HEIGHT;
+
+	return MODE_OK;
+}
+
+static void ssd130x_display_pipe_enable(struct drm_simple_display_pipe *pipe,
+					struct drm_crtc_state *crtc_state,
+					struct drm_plane_state *plane_state)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx, ret;
+
+	ret = ssd130x_poweron(ssd130x);
+	if (ret)
+		return;
+
+	ret = ssd130x_init(ssd130x);
+	if (ret)
+		goto poweroff;
+
+	if (!drm_dev_enter(drm, &idx))
+		goto poweroff;
+
+	ssd130x_clear_screen(ssd130x, ssd130x->width, ssd130x->height);
+
+	ssd130x_write_cmd(ssd130x->client, 1, SSD130X_DISPLAY_ON);
+
+	backlight_enable(ssd130x->bl_dev);
+
+	drm_dev_exit(idx);
+
+	return;
+poweroff:
+	ssd130x_poweroff(ssd130x);
+}
+
+static void ssd130x_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd130x_clear_screen(ssd130x, ssd130x->width, ssd130x->height);
+
+	backlight_disable(ssd130x->bl_dev);
+
+	ssd130x_write_cmd(ssd130x->client, 1, SSD130X_DISPLAY_OFF);
+
+	ssd130x_poweroff(ssd130x);
+
+	drm_dev_exit(idx);
+}
+
+static void ssd130x_display_pipe_update(struct drm_simple_display_pipe *pipe,
+					struct drm_plane_state *old_plane_state)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_plane_state *plane_state = pipe->plane.state;
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_device *drm = &ssd130x->drm;
+	struct drm_rect src_clip, dst_clip;
+	int idx;
+
+	if (!fb)
+		return;
+
+	if (!pipe->crtc.state->active)
+		return;
+
+	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
+		return;
+
+	dst_clip = plane_state->dst;
+	if (!drm_rect_intersect(&dst_clip, &src_clip))
+		return;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
+
+	drm_dev_exit(idx);
+}
+
+static const struct drm_simple_display_pipe_funcs ssd130x_pipe_funcs = {
+	.mode_valid = ssd130x_display_pipe_mode_valid,
+	.enable = ssd130x_display_pipe_enable,
+	.disable = ssd130x_display_pipe_disable,
+	.update = ssd130x_display_pipe_update,
+	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+};
+
+static int ssd130x_connector_get_modes(struct drm_connector *connector)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(connector->dev);
+	struct drm_display_mode *mode = &ssd130x->mode;
+	struct device *dev = &ssd130x->client->dev;
+
+	mode = drm_mode_duplicate(connector->dev, &ssd130x->mode);
+	if (!mode) {
+		dev_err(dev, "Failed to duplicated mode\n");
+		return 0;
+	}
+
+	drm_mode_probed_add(connector, mode);
+	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
+
+	return 1;
+}
+
+static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {
+	.get_modes = ssd130x_connector_get_modes,
+};
+
+static const struct drm_connector_funcs ssd130x_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 ssd130x_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 uint32_t ssd130x_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+DEFINE_DRM_GEM_FOPS(ssd130x_fops);
+
+static const struct drm_driver ssd130x_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.date			= DRIVER_DATE,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &ssd130x_fops,
+};
+
+static int ssd130x_update_bl(struct backlight_device *bdev)
+{
+	struct ssd130x_device *ssd130x = bl_get_data(bdev);
+	int brightness = backlight_get_brightness(bdev);
+	int ret;
+
+	ssd130x->contrast = brightness;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 1, SSD130X_CONTRAST);
+	if (ret < 0)
+		return ret;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 1, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static const struct backlight_ops ssd130xfb_bl_ops = {
+	.update_status	= ssd130x_update_bl,
+};
+
+static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+
+	if (device_property_read_u32(dev, "solomon,width", &ssd130x->width))
+		ssd130x->width = 96;
+
+	if (device_property_read_u32(dev, "solomon,height", &ssd130x->height))
+		ssd130x->height = 16;
+
+	if (device_property_read_u32(dev, "solomon,page-offset", &ssd130x->page_offset))
+		ssd130x->page_offset = 1;
+
+	if (device_property_read_u32(dev, "solomon,col-offset", &ssd130x->col_offset))
+		ssd130x->col_offset = 0;
+
+	if (device_property_read_u32(dev, "solomon,com-offset", &ssd130x->com_offset))
+		ssd130x->com_offset = 0;
+
+	if (device_property_read_u32(dev, "solomon,prechargep1", &ssd130x->prechargep1))
+		ssd130x->prechargep1 = 2;
+
+	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
+		ssd130x->prechargep2 = 2;
+
+	if (!device_property_read_u8_array(dev, "solomon,lookup-table",
+					   ssd130x->lookup_table,
+					   ARRAY_SIZE(ssd130x->lookup_table)))
+		ssd130x->lookup_table_set = 1;
+
+	ssd130x->seg_remap = !device_property_read_bool(dev, "solomon,segment-no-remap");
+	ssd130x->com_seq = device_property_read_bool(dev, "solomon,com-seq");
+	ssd130x->com_lrremap = device_property_read_bool(dev, "solomon,com-lrremap");
+	ssd130x->com_invdir = device_property_read_bool(dev, "solomon,com-invdir");
+	ssd130x->area_color_enable =
+		device_property_read_bool(dev, "solomon,area-color-enable");
+	ssd130x->low_power = device_property_read_bool(dev, "solomon,low-power");
+
+	ssd130x->contrast = 127;
+	ssd130x->vcomh = ssd130x->device_info->default_vcomh;
+
+	/* Setup display timing */
+	if (device_property_read_u32(dev, "solomon,dclk-div", &ssd130x->dclk_div))
+		ssd130x->dclk_div = ssd130x->device_info->default_dclk_div;
+	if (device_property_read_u32(dev, "solomon,dclk-frq", &ssd130x->dclk_frq))
+		ssd130x->dclk_frq = ssd130x->device_info->default_dclk_frq;
+}
+
+static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
+{
+	struct drm_display_mode *mode = &ssd130x->mode;
+	struct device *dev = &ssd130x->client->dev;
+	struct drm_device *drm = &ssd130x->drm;
+	unsigned long max_width, max_height;
+	int ret;
+
+	ret = drmm_mode_config_init(drm);
+	if (ret) {
+		dev_err(dev, "DRM mode config init failed: %d\n", ret);
+		return ret;
+	}
+
+	mode->type = DRM_MODE_TYPE_DRIVER;
+	mode->clock = 1;
+	mode->hdisplay = mode->htotal = ssd130x->width;
+	mode->hsync_start = mode->hsync_end = ssd130x->width;
+	mode->vdisplay = mode->vtotal = ssd130x->height;
+	mode->vsync_start = mode->vsync_end = ssd130x->height;
+	mode->width_mm = 27;
+	mode->height_mm = 27;
+
+	max_width = max_t(unsigned long, mode->hdisplay, DRM_SHADOW_PLANE_MAX_WIDTH);
+	max_height = max_t(unsigned long, mode->vdisplay, DRM_SHADOW_PLANE_MAX_HEIGHT);
+
+	drm->mode_config.min_width = mode->hdisplay;
+	drm->mode_config.max_width = max_width;
+	drm->mode_config.min_height = mode->vdisplay;
+	drm->mode_config.max_height = max_height;
+	drm->mode_config.preferred_depth = 32;
+	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
+
+	ret = drm_connector_init(drm, &ssd130x->connector, &ssd130x_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret) {
+		dev_err(dev, "DRM connector init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_connector_helper_add(&ssd130x->connector, &ssd130x_connector_helper_funcs);
+
+	ret = drm_simple_display_pipe_init(drm, &ssd130x->pipe, &ssd130x_pipe_funcs,
+					   ssd130x_formats, ARRAY_SIZE(ssd130x_formats),
+					   NULL, &ssd130x->connector);
+	if (ret) {
+		dev_err(dev, "DRM simple display pipeline init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_plane_enable_fb_damage_clips(&ssd130x->pipe.plane);
+
+	drm_mode_config_reset(drm);
+
+	return 0;
+}
+
+static int ssd130x_get_resources(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+	int ret;
+
+	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ssd130x->reset)) {
+		ret = PTR_ERR(ssd130x->reset);
+		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
+		return ret;
+	}
+
+	ssd130x->vbat_reg = devm_regulator_get_optional(dev, "vbat");
+	if (IS_ERR(ssd130x->vbat_reg)) {
+		ret = PTR_ERR(ssd130x->vbat_reg);
+		if (ret == -ENODEV) {
+			ssd130x->vbat_reg = NULL;
+		} else {
+			dev_err(dev, "Failed to get VBAT regulator: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ssd130x_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ssd130x_device *ssd130x;
+	struct backlight_device *bl;
+	struct drm_device *drm;
+	int ret;
+
+	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
+				 struct ssd130x_device, drm);
+	if (IS_ERR(ssd130x)) {
+		ret = PTR_ERR(ssd130x);
+		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, ssd130x);
+
+	drm = &ssd130x->drm;
+
+	ssd130x->client = client;
+
+	ssd130x->device_info = device_get_match_data(dev);
+
+	ssd130x_parse_properties(ssd130x);
+
+	ret = ssd130x_get_resources(ssd130x);
+	if (ret)
+		return ret;
+
+	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
+					    &ssd130xfb_bl_ops, NULL);
+	if (IS_ERR(bl)) {
+		ret = PTR_ERR(bl);
+		dev_err(dev, "Unable to register backlight device: %d\n", ret);
+		return ret;
+	}
+
+	bl->props.brightness = ssd130x->contrast;
+	bl->props.max_brightness = MAX_CONTRAST;
+	ssd130x->bl_dev = bl;
+
+	ret = ssd130x_init_modeset(ssd130x);
+	if (ret)
+		return ret;
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		dev_err(dev, "DRM device register failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int ssd130x_remove(struct i2c_client *client)
+{
+	struct ssd130x_device *ssd130x = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&ssd130x->drm);
+
+	return 0;
+}
+
+static void ssd130x_shutdown(struct i2c_client *client)
+{
+	struct ssd130x_device *ssd130x = i2c_get_clientdata(client);
+
+	drm_atomic_helper_shutdown(&ssd130x->drm);
+}
+
+static struct ssd130x_deviceinfo ssd130x_ssd1305_deviceinfo = {
+	.default_vcomh = 0x34,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 7,
+};
+
+static struct ssd130x_deviceinfo ssd130x_ssd1306_deviceinfo = {
+	.default_vcomh = 0x20,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 8,
+	.need_chargepump = 1,
+};
+
+static struct ssd130x_deviceinfo ssd130x_ssd1307_deviceinfo = {
+	.default_vcomh = 0x20,
+	.default_dclk_div = 2,
+	.default_dclk_frq = 12,
+	.need_pwm = 1,
+};
+
+static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo = {
+	.default_vcomh = 0x34,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 10,
+};
+
+static const struct of_device_id ssd130x_of_match[] = {
+	{
+		.compatible = "solomon,ssd1305fb-i2c",
+		.data = (void *)&ssd130x_ssd1305_deviceinfo,
+	},
+	{
+		.compatible = "solomon,ssd1306fb-i2c",
+		.data = (void *)&ssd130x_ssd1306_deviceinfo,
+	},
+	{
+		.compatible = "solomon,ssd1307fb-i2c",
+		.data = (void *)&ssd130x_ssd1307_deviceinfo,
+	},
+	{
+		.compatible = "solomon,ssd1309fb-i2c",
+		.data = (void *)&ssd130x_ssd1309_deviceinfo,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ssd130x_of_match);
+
+
+static struct i2c_driver ssd130x_i2c_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = ssd130x_of_match,
+	},
+	.probe_new = ssd130x_probe,
+	.remove = ssd130x_remove,
+	.shutdown = ssd130x_shutdown,
+};
+
+module_i2c_driver(ssd130x_i2c_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Javier Martinez Canillas <javierm@redhat.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

[PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas]

Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
controllers that can be programmed via an I2C interface. This is a port
of the ssd1307fb driver that already supports these devices.

A Device Tree binding is not added because the DRM driver is compatible
with the existing binding for the ssd1307fb driver.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/gpu/drm/tiny/Kconfig   |  12 +
 drivers/gpu/drm/tiny/Makefile  |   1 +
 drivers/gpu/drm/tiny/ssd130x.c | 971 +++++++++++++++++++++++++++++++++
 3 files changed, 984 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ssd130x.c

diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 712e0004e96e..1b6f5aa41d69 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -67,6 +67,18 @@ config DRM_SIMPLEDRM
 	  On x86 BIOS or UEFI systems, you should also select SYSFB_SIMPLEFB
 	  to use UEFI and VESA framebuffers.
 
+config DRM_SSD130X
+	tristate "DRM support for Solomon SSD130X OLED displays"
+	depends on DRM && I2C
+	select BACKLIGHT_CLASS_DEVICE
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	help
+	  DRM driver for the SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
+	  OLED controllers that can be programmed via an I2C interface.
+
+	  If M is selected the module will be called ssd130x.
+
 config TINYDRM_HX8357D
 	tristate "DRM support for HX8357D display panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 5d5505d40e7b..18c3557dcb71 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
 obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
 obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
+obj-$(CONFIG_DRM_SSD130X)		+= ssd130x.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9163)		+= ili9163.o
 obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
diff --git a/drivers/gpu/drm/tiny/ssd130x.c b/drivers/gpu/drm/tiny/ssd130x.c
new file mode 100644
index 000000000000..b348768529dc
--- /dev/null
+++ b/drivers/gpu/drm/tiny/ssd130x.c
@@ -0,0 +1,971 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DRM driver for Solomon SSD130X OLED displays
+ *
+ * Copyright 2022 Red Hat Inc.
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons
+ *
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_rect.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#define DRIVER_NAME	"ssd130x"
+#define DRIVER_DESC	"DRM driver for Solomon SSD130X OLED displays"
+#define DRIVER_DATE	"20220131"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+#define SSD130X_DATA				0x40
+#define SSD130X_COMMAND			0x80
+
+#define SSD130X_SET_ADDRESS_MODE		0x20
+#define SSD130X_SET_ADDRESS_MODE_HORIZONTAL	(0x00)
+#define SSD130X_SET_ADDRESS_MODE_VERTICAL	(0x01)
+#define SSD130X_SET_ADDRESS_MODE_PAGE		(0x02)
+#define SSD130X_SET_COL_RANGE			0x21
+#define SSD130X_SET_PAGE_RANGE			0x22
+#define SSD130X_CONTRAST			0x81
+#define SSD130X_SET_LOOKUP_TABLE		0x91
+#define SSD130X_CHARGE_PUMP			0x8d
+#define SSD130X_SEG_REMAP_ON			0xa1
+#define SSD130X_DISPLAY_OFF			0xae
+#define SSD130X_SET_MULTIPLEX_RATIO		0xa8
+#define SSD130X_DISPLAY_ON			0xaf
+#define SSD130X_START_PAGE_ADDRESS		0xb0
+#define SSD130X_SET_DISPLAY_OFFSET		0xd3
+#define SSD130X_SET_CLOCK_FREQ			0xd5
+#define SSD130X_SET_AREA_COLOR_MODE		0xd8
+#define SSD130X_SET_PRECHARGE_PERIOD		0xd9
+#define SSD130X_SET_COM_PINS_CONFIG		0xda
+#define SSD130X_SET_VCOMH			0xdb
+
+#define MAX_CONTRAST 255
+
+struct ssd130x_deviceinfo {
+	u32 default_vcomh;
+	u32 default_dclk_div;
+	u32 default_dclk_frq;
+	int need_pwm;
+	int need_chargepump;
+};
+
+struct ssd130x_device {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	struct drm_display_mode mode;
+	struct drm_connector connector;
+	struct i2c_client *client;
+
+	const struct ssd130x_deviceinfo *device_info;
+
+	unsigned area_color_enable : 1;
+	unsigned com_invdir : 1;
+	unsigned com_lrremap : 1;
+	unsigned com_seq : 1;
+	unsigned lookup_table_set : 1;
+	unsigned low_power : 1;
+	unsigned seg_remap : 1;
+	u32 com_offset;
+	u32 contrast;
+	u32 dclk_div;
+	u32 dclk_frq;
+	u32 height;
+	u8 lookup_table[4];
+	u32 page_offset;
+	u32 col_offset;
+	u32 prechargep1;
+	u32 prechargep2;
+
+	struct backlight_device *bl_dev;
+	struct pwm_device *pwm;
+	struct gpio_desc *reset;
+	struct regulator *vbat_reg;
+	u32 vcomh;
+	u32 width;
+	/* Cached address ranges */
+	u8 col_start;
+	u8 col_end;
+	u8 page_start;
+	u8 page_end;
+};
+
+struct ssd130x_i2c_msg {
+	u8	cmd;
+	u8	data[];
+};
+
+static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
+{
+	return container_of(drm, struct ssd130x_device, drm);
+}
+
+static struct ssd130x_i2c_msg *ssd130x_alloc_msg(u32 len, u8 cmd)
+{
+	struct ssd130x_i2c_msg *msg;
+
+	msg = kzalloc(struct_size(msg, data, len), GFP_KERNEL);
+	if (!msg)
+		return NULL;
+
+	msg->cmd = cmd;
+
+	return msg;
+}
+
+static int ssd130x_write_msg(struct i2c_client *client,
+			     struct ssd130x_i2c_msg *msg, u32 len)
+{
+	int ret;
+
+	len += sizeof(*msg);
+
+	ret = i2c_master_send(client, (u8 *)msg, len);
+	if (ret != len) {
+		dev_err(&client->dev, "Couldn't send I2C command.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static inline int ssd130x_write_value(struct i2c_client *client, u8 value)
+{
+	struct ssd130x_i2c_msg *msg;
+	int ret;
+
+	msg = ssd130x_alloc_msg(1, SSD130X_COMMAND);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->data[0] = value;
+
+	ret = ssd130x_write_msg(client, msg, 1);
+	kfree(msg);
+
+	return ret;
+}
+
+static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
+				    /* u8 cmd, u8 value, ... */...)
+{
+	va_list ap;
+	u8 value;
+	int ret;
+
+	va_start(ap, count);
+
+	while (count--) {
+		value = va_arg(ap, int);
+		ret = ssd130x_write_value(client, (u8)value);
+		if (ret)
+			goto out_end;
+	}
+
+out_end:
+	va_end(ap);
+
+	return ret;
+}
+
+static int ssd130x_set_col_range(struct ssd130x_device *ssd130x,
+				 u8 col_start, u8 cols)
+{
+	u8 col_end = col_start + cols - 1;
+	int ret;
+
+	if (col_start == ssd130x->col_start && col_end == ssd130x->col_end)
+		return 0;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 3, SSD130X_SET_COL_RANGE,
+				col_start, col_end);
+	if (ret < 0)
+		return ret;
+
+	ssd130x->col_start = col_start;
+	ssd130x->col_end = col_end;
+	return 0;
+}
+
+static int ssd130x_set_page_range(struct ssd130x_device *ssd130x,
+				  u8 page_start, u8 pages)
+{
+	u8 page_end = page_start + pages - 1;
+	int ret;
+
+	if (page_start == ssd130x->page_start && page_end == ssd130x->page_end)
+		return 0;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 3, SSD130X_SET_PAGE_RANGE,
+				page_start, page_end);
+	if (ret < 0)
+		return ret;
+
+	ssd130x->page_start = page_start;
+	ssd130x->page_end = page_end;
+	return 0;
+}
+
+static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+	struct pwm_state pwmstate;
+
+	ssd130x->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(ssd130x->pwm)) {
+		dev_err(dev, "Could not get PWM from device tree!\n");
+		return PTR_ERR(ssd130x->pwm);
+	}
+
+	pwm_init_state(ssd130x->pwm, &pwmstate);
+	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
+	pwm_apply_state(ssd130x->pwm, &pwmstate);
+
+	/* Enable the PWM */
+	pwm_enable(ssd130x->pwm);
+
+	dev_dbg(dev, "Using PWM%d with a %lluns period.\n",
+		ssd130x->pwm->pwm, pwm_get_period(ssd130x->pwm));
+
+	return 0;
+}
+
+static void ssd130x_reset(struct ssd130x_device *ssd130x)
+{
+	/* Reset the screen */
+	gpiod_set_value_cansleep(ssd130x->reset, 1);
+	udelay(4);
+	gpiod_set_value_cansleep(ssd130x->reset, 0);
+	udelay(4);
+}
+
+static int ssd130x_poweron(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+	int ret;
+
+	if (ssd130x->reset)
+		ssd130x_reset(ssd130x);
+
+	if (ssd130x->vbat_reg) {
+		ret = regulator_enable(ssd130x->vbat_reg);
+		if (ret) {
+			dev_err(dev, "Failed to enable VBAT: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (ssd130x->device_info->need_pwm) {
+		ret = ssd130x_pwm_enable(ssd130x);
+		if (ret) {
+			dev_err(dev, "Failed to enable PWM: %d\n", ret);
+			if (ssd130x->vbat_reg)
+				regulator_disable(ssd130x->vbat_reg);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ssd130x_poweroff(struct ssd130x_device *ssd130x)
+{
+	if (ssd130x->device_info->need_pwm) {
+		pwm_disable(ssd130x->pwm);
+		pwm_put(ssd130x->pwm);
+	}
+
+	if (ssd130x->vbat_reg)
+		regulator_disable(ssd130x->vbat_reg);
+}
+
+static int ssd130x_init(struct ssd130x_device *ssd130x)
+{
+	u32 precharge, dclk, com_invdir, compins, chargepump;
+	int ret;
+
+	/* Set initial contrast */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
+	/* Set segment re-map */
+	if (ssd130x->seg_remap) {
+		ret = ssd130x_write_cmd(ssd130x->client, 1, SSD130X_SEG_REMAP_ON);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set COM direction */
+	com_invdir = 0xc0 | ssd130x->com_invdir << 3;
+	ret = ssd130x_write_cmd(ssd130x->client,  1, com_invdir);
+	if (ret < 0)
+		return ret;
+
+	/* Set multiplex ratio value */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_MULTIPLEX_RATIO,
+				ssd130x->height - 1);
+	if (ret < 0)
+		return ret;
+
+	/* set display offset value */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_DISPLAY_OFFSET,
+				ssd130x->com_offset);
+	if (ret < 0)
+		return ret;
+
+	/* Set clock frequency */
+	dclk = ((ssd130x->dclk_div - 1) & 0xf) | (ssd130x->dclk_frq & 0xf) << 4;
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_CLOCK_FREQ, dclk);
+	if (ret < 0)
+		return ret;
+
+	/* Set Area Color Mode ON/OFF & Low Power Display Mode */
+	if (ssd130x->area_color_enable || ssd130x->low_power) {
+		u32 mode = ((ssd130x->area_color_enable ? 0x30 : 0) |
+			    (ssd130x->low_power ? 5 : 0));
+
+		ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_AREA_COLOR_MODE, mode);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set precharge period in number of ticks from the internal clock */
+	precharge = (ssd130x->prechargep1 & 0xf) | (ssd130x->prechargep2 & 0xf) << 4;
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_PRECHARGE_PERIOD, precharge);
+	if (ret < 0)
+		return ret;
+
+	/* Set COM pins configuration */
+	compins = 0x02 | !ssd130x->com_seq << 4 | ssd130x->com_lrremap << 5;
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_COM_PINS_CONFIG, compins);
+	if (ret < 0)
+		return ret;
+
+
+	/* Set VCOMH */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_VCOMH, ssd130x->vcomh);
+	if (ret < 0)
+		return ret;
+
+	/* Turn on the DC-DC Charge Pump */
+	chargepump = BIT(4) | (ssd130x->device_info->need_chargepump ? BIT(2) : 0);
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CHARGE_PUMP, chargepump);
+	if (ret < 0)
+		return ret;
+
+	/* Set lookup table */
+	if (ssd130x->lookup_table_set) {
+		int i;
+
+		ret = ssd130x_write_cmd(ssd130x->client, 1, SSD130X_SET_LOOKUP_TABLE);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {
+			u8 val = ssd130x->lookup_table[i];
+
+			if (val < 31 || val > 63)
+				dev_warn(&ssd130x->client->dev,
+					 "lookup table index %d value out of range 31 <= %d <= 63\n",
+					 i, val);
+			ret = ssd130x_write_cmd(ssd130x->client, 1, val);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	/* Switch to horizontal addressing mode */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
+				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int ssd130x_update_display(struct ssd130x_device *ssd130x, u8 *buf,
+				  u32 width, u32 height)
+{
+	struct ssd130x_i2c_msg *msg;
+	unsigned int line_length = DIV_ROUND_UP(width, 8);
+	unsigned int pages = DIV_ROUND_UP(height, 8);
+	u32 array_idx = 0;
+	int ret, i, j, k;
+
+	msg = ssd130x_alloc_msg(width * pages, SSD130X_DATA);
+	if (!msg)
+		return -ENOMEM;
+
+	/*
+	 * The screen is divided in pages, each having a height of 8
+	 * pixels, and the width of the screen. When sending a byte of
+	 * data to the controller, it gives the 8 bits for the current
+	 * column. I.e, the first byte are the 8 bits of the first
+	 * column, then the 8 bits for the second column, etc.
+	 *
+	 *
+	 * Representation of the screen, assuming it is 5 bits
+	 * wide. Each letter-number combination is a bit that controls
+	 * one pixel.
+	 *
+	 * A0 A1 A2 A3 A4
+	 * B0 B1 B2 B3 B4
+	 * C0 C1 C2 C3 C4
+	 * D0 D1 D2 D3 D4
+	 * E0 E1 E2 E3 E4
+	 * F0 F1 F2 F3 F4
+	 * G0 G1 G2 G3 G4
+	 * H0 H1 H2 H3 H4
+	 *
+	 * If you want to update this screen, you need to send 5 bytes:
+	 *  (1) A0 B0 C0 D0 E0 F0 G0 H0
+	 *  (2) A1 B1 C1 D1 E1 F1 G1 H1
+	 *  (3) A2 B2 C2 D2 E2 F2 G2 H2
+	 *  (4) A3 B3 C3 D3 E3 F3 G3 H3
+	 *  (5) A4 B4 C4 D4 E4 F4 G4 H4
+	 */
+
+	ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset, width);
+	if (ret < 0)
+		goto out_free;
+
+	ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset / 8, pages);
+	if (ret < 0)
+		goto out_free;
+
+	for (i = 0; i < pages; i++) {
+		int m = 8;
+
+		/* Last page may be partial */
+		if (8 * (i + 1) > ssd130x->height)
+			m = ssd130x->height % 8;
+		for (j = 0; j < width; j++) {
+			u8 data = 0;
+
+			for (k = 0; k < m; k++) {
+				u8 byte = buf[(8 * i + k) * line_length +
+					       j / 8];
+				u8 bit = (byte >> (j % 8)) & 1;
+
+				data |= bit << k;
+			}
+			msg->data[array_idx++] = data;
+		}
+	}
+
+	ret = ssd130x_write_msg(ssd130x->client, msg, width * pages);
+
+out_free:
+	kfree(msg);
+	return ret;
+}
+
+static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, u32 width, u32 height)
+{
+	u8 *buf = NULL;
+
+	buf = kcalloc(width, height, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ssd130x_update_display(ssd130x, buf, width, height);
+
+	kfree(buf);
+}
+
+static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
+				struct drm_rect *rect)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+	int ret = 0;
+	u8 *buf = NULL;
+
+	buf = kcalloc(fb->width, fb->height, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
+
+	ssd130x_update_display(ssd130x, buf, fb->width, fb->height);
+
+	kfree(buf);
+
+	return ret;
+}
+
+static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					   const struct drm_display_mode *mode)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+
+	if (mode->hdisplay != ssd130x->mode.hdisplay &&
+	    mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_SIZE;
+	else if (mode->hdisplay != ssd130x->mode.hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_HEIGHT;
+
+	return MODE_OK;
+}
+
+static void ssd130x_display_pipe_enable(struct drm_simple_display_pipe *pipe,
+					struct drm_crtc_state *crtc_state,
+					struct drm_plane_state *plane_state)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx, ret;
+
+	ret = ssd130x_poweron(ssd130x);
+	if (ret)
+		return;
+
+	ret = ssd130x_init(ssd130x);
+	if (ret)
+		goto poweroff;
+
+	if (!drm_dev_enter(drm, &idx))
+		goto poweroff;
+
+	ssd130x_clear_screen(ssd130x, ssd130x->width, ssd130x->height);
+
+	ssd130x_write_cmd(ssd130x->client, 1, SSD130X_DISPLAY_ON);
+
+	backlight_enable(ssd130x->bl_dev);
+
+	drm_dev_exit(idx);
+
+	return;
+poweroff:
+	ssd130x_poweroff(ssd130x);
+}
+
+static void ssd130x_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd130x_clear_screen(ssd130x, ssd130x->width, ssd130x->height);
+
+	backlight_disable(ssd130x->bl_dev);
+
+	ssd130x_write_cmd(ssd130x->client, 1, SSD130X_DISPLAY_OFF);
+
+	ssd130x_poweroff(ssd130x);
+
+	drm_dev_exit(idx);
+}
+
+static void ssd130x_display_pipe_update(struct drm_simple_display_pipe *pipe,
+					struct drm_plane_state *old_plane_state)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_plane_state *plane_state = pipe->plane.state;
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_device *drm = &ssd130x->drm;
+	struct drm_rect src_clip, dst_clip;
+	int idx;
+
+	if (!fb)
+		return;
+
+	if (!pipe->crtc.state->active)
+		return;
+
+	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
+		return;
+
+	dst_clip = plane_state->dst;
+	if (!drm_rect_intersect(&dst_clip, &src_clip))
+		return;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
+
+	drm_dev_exit(idx);
+}
+
+static const struct drm_simple_display_pipe_funcs ssd130x_pipe_funcs = {
+	.mode_valid = ssd130x_display_pipe_mode_valid,
+	.enable = ssd130x_display_pipe_enable,
+	.disable = ssd130x_display_pipe_disable,
+	.update = ssd130x_display_pipe_update,
+	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+};
+
+static int ssd130x_connector_get_modes(struct drm_connector *connector)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(connector->dev);
+	struct drm_display_mode *mode = &ssd130x->mode;
+	struct device *dev = &ssd130x->client->dev;
+
+	mode = drm_mode_duplicate(connector->dev, &ssd130x->mode);
+	if (!mode) {
+		dev_err(dev, "Failed to duplicated mode\n");
+		return 0;
+	}
+
+	drm_mode_probed_add(connector, mode);
+	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
+
+	return 1;
+}
+
+static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {
+	.get_modes = ssd130x_connector_get_modes,
+};
+
+static const struct drm_connector_funcs ssd130x_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 ssd130x_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 uint32_t ssd130x_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+DEFINE_DRM_GEM_FOPS(ssd130x_fops);
+
+static const struct drm_driver ssd130x_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.date			= DRIVER_DATE,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &ssd130x_fops,
+};
+
+static int ssd130x_update_bl(struct backlight_device *bdev)
+{
+	struct ssd130x_device *ssd130x = bl_get_data(bdev);
+	int brightness = backlight_get_brightness(bdev);
+	int ret;
+
+	ssd130x->contrast = brightness;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 1, SSD130X_CONTRAST);
+	if (ret < 0)
+		return ret;
+
+	ret = ssd130x_write_cmd(ssd130x->client, 1, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static const struct backlight_ops ssd130xfb_bl_ops = {
+	.update_status	= ssd130x_update_bl,
+};
+
+static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+
+	if (device_property_read_u32(dev, "solomon,width", &ssd130x->width))
+		ssd130x->width = 96;
+
+	if (device_property_read_u32(dev, "solomon,height", &ssd130x->height))
+		ssd130x->height = 16;
+
+	if (device_property_read_u32(dev, "solomon,page-offset", &ssd130x->page_offset))
+		ssd130x->page_offset = 1;
+
+	if (device_property_read_u32(dev, "solomon,col-offset", &ssd130x->col_offset))
+		ssd130x->col_offset = 0;
+
+	if (device_property_read_u32(dev, "solomon,com-offset", &ssd130x->com_offset))
+		ssd130x->com_offset = 0;
+
+	if (device_property_read_u32(dev, "solomon,prechargep1", &ssd130x->prechargep1))
+		ssd130x->prechargep1 = 2;
+
+	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
+		ssd130x->prechargep2 = 2;
+
+	if (!device_property_read_u8_array(dev, "solomon,lookup-table",
+					   ssd130x->lookup_table,
+					   ARRAY_SIZE(ssd130x->lookup_table)))
+		ssd130x->lookup_table_set = 1;
+
+	ssd130x->seg_remap = !device_property_read_bool(dev, "solomon,segment-no-remap");
+	ssd130x->com_seq = device_property_read_bool(dev, "solomon,com-seq");
+	ssd130x->com_lrremap = device_property_read_bool(dev, "solomon,com-lrremap");
+	ssd130x->com_invdir = device_property_read_bool(dev, "solomon,com-invdir");
+	ssd130x->area_color_enable =
+		device_property_read_bool(dev, "solomon,area-color-enable");
+	ssd130x->low_power = device_property_read_bool(dev, "solomon,low-power");
+
+	ssd130x->contrast = 127;
+	ssd130x->vcomh = ssd130x->device_info->default_vcomh;
+
+	/* Setup display timing */
+	if (device_property_read_u32(dev, "solomon,dclk-div", &ssd130x->dclk_div))
+		ssd130x->dclk_div = ssd130x->device_info->default_dclk_div;
+	if (device_property_read_u32(dev, "solomon,dclk-frq", &ssd130x->dclk_frq))
+		ssd130x->dclk_frq = ssd130x->device_info->default_dclk_frq;
+}
+
+static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
+{
+	struct drm_display_mode *mode = &ssd130x->mode;
+	struct device *dev = &ssd130x->client->dev;
+	struct drm_device *drm = &ssd130x->drm;
+	unsigned long max_width, max_height;
+	int ret;
+
+	ret = drmm_mode_config_init(drm);
+	if (ret) {
+		dev_err(dev, "DRM mode config init failed: %d\n", ret);
+		return ret;
+	}
+
+	mode->type = DRM_MODE_TYPE_DRIVER;
+	mode->clock = 1;
+	mode->hdisplay = mode->htotal = ssd130x->width;
+	mode->hsync_start = mode->hsync_end = ssd130x->width;
+	mode->vdisplay = mode->vtotal = ssd130x->height;
+	mode->vsync_start = mode->vsync_end = ssd130x->height;
+	mode->width_mm = 27;
+	mode->height_mm = 27;
+
+	max_width = max_t(unsigned long, mode->hdisplay, DRM_SHADOW_PLANE_MAX_WIDTH);
+	max_height = max_t(unsigned long, mode->vdisplay, DRM_SHADOW_PLANE_MAX_HEIGHT);
+
+	drm->mode_config.min_width = mode->hdisplay;
+	drm->mode_config.max_width = max_width;
+	drm->mode_config.min_height = mode->vdisplay;
+	drm->mode_config.max_height = max_height;
+	drm->mode_config.preferred_depth = 32;
+	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
+
+	ret = drm_connector_init(drm, &ssd130x->connector, &ssd130x_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret) {
+		dev_err(dev, "DRM connector init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_connector_helper_add(&ssd130x->connector, &ssd130x_connector_helper_funcs);
+
+	ret = drm_simple_display_pipe_init(drm, &ssd130x->pipe, &ssd130x_pipe_funcs,
+					   ssd130x_formats, ARRAY_SIZE(ssd130x_formats),
+					   NULL, &ssd130x->connector);
+	if (ret) {
+		dev_err(dev, "DRM simple display pipeline init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_plane_enable_fb_damage_clips(&ssd130x->pipe.plane);
+
+	drm_mode_config_reset(drm);
+
+	return 0;
+}
+
+static int ssd130x_get_resources(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = &ssd130x->client->dev;
+	int ret;
+
+	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ssd130x->reset)) {
+		ret = PTR_ERR(ssd130x->reset);
+		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
+		return ret;
+	}
+
+	ssd130x->vbat_reg = devm_regulator_get_optional(dev, "vbat");
+	if (IS_ERR(ssd130x->vbat_reg)) {
+		ret = PTR_ERR(ssd130x->vbat_reg);
+		if (ret == -ENODEV) {
+			ssd130x->vbat_reg = NULL;
+		} else {
+			dev_err(dev, "Failed to get VBAT regulator: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ssd130x_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ssd130x_device *ssd130x;
+	struct backlight_device *bl;
+	struct drm_device *drm;
+	int ret;
+
+	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
+				 struct ssd130x_device, drm);
+	if (IS_ERR(ssd130x)) {
+		ret = PTR_ERR(ssd130x);
+		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, ssd130x);
+
+	drm = &ssd130x->drm;
+
+	ssd130x->client = client;
+
+	ssd130x->device_info = device_get_match_data(dev);
+
+	ssd130x_parse_properties(ssd130x);
+
+	ret = ssd130x_get_resources(ssd130x);
+	if (ret)
+		return ret;
+
+	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
+					    &ssd130xfb_bl_ops, NULL);
+	if (IS_ERR(bl)) {
+		ret = PTR_ERR(bl);
+		dev_err(dev, "Unable to register backlight device: %d\n", ret);
+		return ret;
+	}
+
+	bl->props.brightness = ssd130x->contrast;
+	bl->props.max_brightness = MAX_CONTRAST;
+	ssd130x->bl_dev = bl;
+
+	ret = ssd130x_init_modeset(ssd130x);
+	if (ret)
+		return ret;
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		dev_err(dev, "DRM device register failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int ssd130x_remove(struct i2c_client *client)
+{
+	struct ssd130x_device *ssd130x = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&ssd130x->drm);
+
+	return 0;
+}
+
+static void ssd130x_shutdown(struct i2c_client *client)
+{
+	struct ssd130x_device *ssd130x = i2c_get_clientdata(client);
+
+	drm_atomic_helper_shutdown(&ssd130x->drm);
+}
+
+static struct ssd130x_deviceinfo ssd130x_ssd1305_deviceinfo = {
+	.default_vcomh = 0x34,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 7,
+};
+
+static struct ssd130x_deviceinfo ssd130x_ssd1306_deviceinfo = {
+	.default_vcomh = 0x20,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 8,
+	.need_chargepump = 1,
+};
+
+static struct ssd130x_deviceinfo ssd130x_ssd1307_deviceinfo = {
+	.default_vcomh = 0x20,
+	.default_dclk_div = 2,
+	.default_dclk_frq = 12,
+	.need_pwm = 1,
+};
+
+static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo = {
+	.default_vcomh = 0x34,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 10,
+};
+
+static const struct of_device_id ssd130x_of_match[] = {
+	{
+		.compatible = "solomon,ssd1305fb-i2c",
+		.data = (void *)&ssd130x_ssd1305_deviceinfo,
+	},
+	{
+		.compatible = "solomon,ssd1306fb-i2c",
+		.data = (void *)&ssd130x_ssd1306_deviceinfo,
+	},
+	{
+		.compatible = "solomon,ssd1307fb-i2c",
+		.data = (void *)&ssd130x_ssd1307_deviceinfo,
+	},
+	{
+		.compatible = "solomon,ssd1309fb-i2c",
+		.data = (void *)&ssd130x_ssd1309_deviceinfo,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ssd130x_of_match);
+
+
+static struct i2c_driver ssd130x_i2c_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = ssd130x_of_match,
+	},
+	.probe_new = ssd130x_probe,
+	.remove = ssd130x_remove,
+	.shutdown = ssd130x_shutdown,
+};
+
+module_i2c_driver(ssd130x_i2c_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Javier Martinez Canillas <javierm@redhat.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
> Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
> controllers that can be programmed via an I2C interface. This is a port
> of the ssd1307fb driver that already supports these devices.
> 
> A Device Tree binding is not added because the DRM driver is compatible
> with the existing binding for the ssd1307fb driver.

...

> +/*
> + * DRM driver for Solomon SSD130X OLED displays
> + *
> + * Copyright 2022 Red Hat Inc.
> + *
> + * Based on drivers/video/fbdev/ssd1307fb.c
> + * Copyright 2012 Free Electrons

> + *

No need for this blank line.

> + */

...

> +struct ssd130x_device {
> +	struct drm_device drm;
> +	struct drm_simple_display_pipe pipe;
> +	struct drm_display_mode mode;
> +	struct drm_connector connector;


> +	struct i2c_client *client;

Can we logically separate hw protocol vs hw interface from day 1, please?
This will allow to add SPI support for this panel much easier.

Technically I would like to see here

	struct device *dev;

and probably (I haven't looked into design)

	struct ssd130x_ops *ops;

or something alike.

> +	const struct ssd130x_deviceinfo *device_info;
> +
> +	unsigned area_color_enable : 1;
> +	unsigned com_invdir : 1;
> +	unsigned com_lrremap : 1;
> +	unsigned com_seq : 1;
> +	unsigned lookup_table_set : 1;
> +	unsigned low_power : 1;
> +	unsigned seg_remap : 1;
> +	u32 com_offset;
> +	u32 contrast;
> +	u32 dclk_div;
> +	u32 dclk_frq;
> +	u32 height;
> +	u8 lookup_table[4];
> +	u32 page_offset;
> +	u32 col_offset;
> +	u32 prechargep1;
> +	u32 prechargep2;
> +
> +	struct backlight_device *bl_dev;
> +	struct pwm_device *pwm;
> +	struct gpio_desc *reset;
> +	struct regulator *vbat_reg;
> +	u32 vcomh;
> +	u32 width;
> +	/* Cached address ranges */
> +	u8 col_start;
> +	u8 col_end;
> +	u8 page_start;
> +	u8 page_end;
> +};

...

> +static inline int ssd130x_write_value(struct i2c_client *client, u8 value)

Not sure inline does anything useful here.
Ditto for the rest similar cases.

...

> +static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
> +				    /* u8 cmd, u8 value, ... */...)
> +{
> +	va_list ap;
> +	u8 value;
> +	int ret;
> +
> +	va_start(ap, count);

> +	while (count--) {
> +		value = va_arg(ap, int);
> +		ret = ssd130x_write_value(client, (u8)value);
> +		if (ret)
> +			goto out_end;
> +	}

I'm wondering if this can be written in a form

	do {
		...
	} while (--count);

In this case it will give a hint that count can't be 0.

> +out_end:
> +	va_end(ap);
> +
> +	return ret;
> +}


...

> +	ssd130x->pwm = pwm_get(dev, NULL);
> +	if (IS_ERR(ssd130x->pwm)) {
> +		dev_err(dev, "Could not get PWM from device tree!\n");

"device tree" is a bit confusing here if I run this on ACPI.
Maybe something like "firmware description"?

> +		return PTR_ERR(ssd130x->pwm);
> +	}

...

> +	/* Set initial contrast */
> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);

Creating a local variable for client allows to:
- make lines shorter and might even be less LOCs
- allow to convert struct device to client in one place
  (as per my above comment)

Ditto for other similar cases.

> +	if (ret < 0)
> +		return ret;

...

> +		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {

i++ should work same way.

> +		}

...

> +	/* Switch to horizontal addressing mode */
> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
> +				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Can it be

	return ssd130x_write_cmd(...);

?

...

> +	unsigned int line_length = DIV_ROUND_UP(width, 8);
> +	unsigned int pages = DIV_ROUND_UP(height, 8);

For power of two there are more efficient roundup()/rounddown()
(or with _ in the names, I don't remember by heart).

...

> +			for (k = 0; k < m; k++) {

> +				u8 byte = buf[(8 * i + k) * line_length +
> +					       j / 8];

One line?

> +				u8 bit = (byte >> (j % 8)) & 1;
> +
> +				data |= bit << k;
> +			}

...

> +static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +					   const struct drm_display_mode *mode)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
> +
> +	if (mode->hdisplay != ssd130x->mode.hdisplay &&
> +	    mode->vdisplay != ssd130x->mode.vdisplay)
> +		return MODE_ONE_SIZE;

> +	else if (mode->hdisplay != ssd130x->mode.hdisplay)
> +		return MODE_ONE_WIDTH;
> +	else if (mode->vdisplay != ssd130x->mode.vdisplay)
> +		return MODE_ONE_HEIGHT;

'else' in both cases is redundant.

> +	return MODE_OK;
> +}

...

> +poweroff:

out_power_off: ?

...

> +	if (!fb)
> +		return;

Can it happen?

...

> +	drm_mode_probed_add(connector, mode);
> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
> +
> +	return 1;

Positive code, what is the meaning of it?

...

> +	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
> +		ssd130x->prechargep2 = 2;

You can drop conditionals for the optional properties

	ssd130x->prechargep2 = 2;
	device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2);

and so on for the similar.

...

> +	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ssd130x->reset)) {

> +		ret = PTR_ERR(ssd130x->reset);
> +		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
> +		return ret;

Why not

	return dev_err_probe()?

Each time you call it for deferred probe, it will spam logs.

> +	}
> +
> +	ssd130x->vbat_reg = devm_regulator_get_optional(dev, "vbat");
> +	if (IS_ERR(ssd130x->vbat_reg)) {
> +		ret = PTR_ERR(ssd130x->vbat_reg);
> +		if (ret == -ENODEV) {
> +			ssd130x->vbat_reg = NULL;

> +		} else {
> +			dev_err(dev, "Failed to get VBAT regulator: %d\n", ret);
> +			return ret;
> +		}

Ditto ?

> +	}

...

> +	if (IS_ERR(ssd130x)) {
> +		ret = PTR_ERR(ssd130x);
> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
> +		return ret;
> +	}

Ditto.

...

> +	i2c_set_clientdata(client, ssd130x);

Wondering if you can split i2c part from the core part and perhaps use regmap
to access the device.

...

> +	if (IS_ERR(bl)) {
> +		ret = PTR_ERR(bl);
> +		dev_err(dev, "Unable to register backlight device: %d\n", ret);
> +		return ret;

	return dev_err_probe();

> +	}

...

> +	ret = drm_dev_register(drm, 0);
> +	if (ret) {
> +		dev_err(dev, "DRM device register failed: %d\n", ret);
> +		return ret;
> +	}

Ditto.

...

> +	{},

Comma is not needed in terminator entry.

...

> +static struct i2c_driver ssd130x_i2c_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = ssd130x_of_match,
> +	},
> +	.probe_new = ssd130x_probe,
> +	.remove = ssd130x_remove,
> +	.shutdown = ssd130x_shutdown,
> +};

> +

Redundant blank line.

> +module_i2c_driver(ssd130x_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
> Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
> controllers that can be programmed via an I2C interface. This is a port
> of the ssd1307fb driver that already supports these devices.
> 
> A Device Tree binding is not added because the DRM driver is compatible
> with the existing binding for the ssd1307fb driver.

...

> +/*
> + * DRM driver for Solomon SSD130X OLED displays
> + *
> + * Copyright 2022 Red Hat Inc.
> + *
> + * Based on drivers/video/fbdev/ssd1307fb.c
> + * Copyright 2012 Free Electrons

> + *

No need for this blank line.

> + */

...

> +struct ssd130x_device {
> +	struct drm_device drm;
> +	struct drm_simple_display_pipe pipe;
> +	struct drm_display_mode mode;
> +	struct drm_connector connector;


> +	struct i2c_client *client;

Can we logically separate hw protocol vs hw interface from day 1, please?
This will allow to add SPI support for this panel much easier.

Technically I would like to see here

	struct device *dev;

and probably (I haven't looked into design)

	struct ssd130x_ops *ops;

or something alike.

> +	const struct ssd130x_deviceinfo *device_info;
> +
> +	unsigned area_color_enable : 1;
> +	unsigned com_invdir : 1;
> +	unsigned com_lrremap : 1;
> +	unsigned com_seq : 1;
> +	unsigned lookup_table_set : 1;
> +	unsigned low_power : 1;
> +	unsigned seg_remap : 1;
> +	u32 com_offset;
> +	u32 contrast;
> +	u32 dclk_div;
> +	u32 dclk_frq;
> +	u32 height;
> +	u8 lookup_table[4];
> +	u32 page_offset;
> +	u32 col_offset;
> +	u32 prechargep1;
> +	u32 prechargep2;
> +
> +	struct backlight_device *bl_dev;
> +	struct pwm_device *pwm;
> +	struct gpio_desc *reset;
> +	struct regulator *vbat_reg;
> +	u32 vcomh;
> +	u32 width;
> +	/* Cached address ranges */
> +	u8 col_start;
> +	u8 col_end;
> +	u8 page_start;
> +	u8 page_end;
> +};

...

> +static inline int ssd130x_write_value(struct i2c_client *client, u8 value)

Not sure inline does anything useful here.
Ditto for the rest similar cases.

...

> +static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
> +				    /* u8 cmd, u8 value, ... */...)
> +{
> +	va_list ap;
> +	u8 value;
> +	int ret;
> +
> +	va_start(ap, count);

> +	while (count--) {
> +		value = va_arg(ap, int);
> +		ret = ssd130x_write_value(client, (u8)value);
> +		if (ret)
> +			goto out_end;
> +	}

I'm wondering if this can be written in a form

	do {
		...
	} while (--count);

In this case it will give a hint that count can't be 0.

> +out_end:
> +	va_end(ap);
> +
> +	return ret;
> +}


...

> +	ssd130x->pwm = pwm_get(dev, NULL);
> +	if (IS_ERR(ssd130x->pwm)) {
> +		dev_err(dev, "Could not get PWM from device tree!\n");

"device tree" is a bit confusing here if I run this on ACPI.
Maybe something like "firmware description"?

> +		return PTR_ERR(ssd130x->pwm);
> +	}

...

> +	/* Set initial contrast */
> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);

Creating a local variable for client allows to:
- make lines shorter and might even be less LOCs
- allow to convert struct device to client in one place
  (as per my above comment)

Ditto for other similar cases.

> +	if (ret < 0)
> +		return ret;

...

> +		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {

i++ should work same way.

> +		}

...

> +	/* Switch to horizontal addressing mode */
> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
> +				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Can it be

	return ssd130x_write_cmd(...);

?

...

> +	unsigned int line_length = DIV_ROUND_UP(width, 8);
> +	unsigned int pages = DIV_ROUND_UP(height, 8);

For power of two there are more efficient roundup()/rounddown()
(or with _ in the names, I don't remember by heart).

...

> +			for (k = 0; k < m; k++) {

> +				u8 byte = buf[(8 * i + k) * line_length +
> +					       j / 8];

One line?

> +				u8 bit = (byte >> (j % 8)) & 1;
> +
> +				data |= bit << k;
> +			}

...

> +static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +					   const struct drm_display_mode *mode)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
> +
> +	if (mode->hdisplay != ssd130x->mode.hdisplay &&
> +	    mode->vdisplay != ssd130x->mode.vdisplay)
> +		return MODE_ONE_SIZE;

> +	else if (mode->hdisplay != ssd130x->mode.hdisplay)
> +		return MODE_ONE_WIDTH;
> +	else if (mode->vdisplay != ssd130x->mode.vdisplay)
> +		return MODE_ONE_HEIGHT;

'else' in both cases is redundant.

> +	return MODE_OK;
> +}

...

> +poweroff:

out_power_off: ?

...

> +	if (!fb)
> +		return;

Can it happen?

...

> +	drm_mode_probed_add(connector, mode);
> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
> +
> +	return 1;

Positive code, what is the meaning of it?

...

> +	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
> +		ssd130x->prechargep2 = 2;

You can drop conditionals for the optional properties

	ssd130x->prechargep2 = 2;
	device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2);

and so on for the similar.

...

> +	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ssd130x->reset)) {

> +		ret = PTR_ERR(ssd130x->reset);
> +		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
> +		return ret;

Why not

	return dev_err_probe()?

Each time you call it for deferred probe, it will spam logs.

> +	}
> +
> +	ssd130x->vbat_reg = devm_regulator_get_optional(dev, "vbat");
> +	if (IS_ERR(ssd130x->vbat_reg)) {
> +		ret = PTR_ERR(ssd130x->vbat_reg);
> +		if (ret == -ENODEV) {
> +			ssd130x->vbat_reg = NULL;

> +		} else {
> +			dev_err(dev, "Failed to get VBAT regulator: %d\n", ret);
> +			return ret;
> +		}

Ditto ?

> +	}

...

> +	if (IS_ERR(ssd130x)) {
> +		ret = PTR_ERR(ssd130x);
> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
> +		return ret;
> +	}

Ditto.

...

> +	i2c_set_clientdata(client, ssd130x);

Wondering if you can split i2c part from the core part and perhaps use regmap
to access the device.

...

> +	if (IS_ERR(bl)) {
> +		ret = PTR_ERR(bl);
> +		dev_err(dev, "Unable to register backlight device: %d\n", ret);
> +		return ret;

	return dev_err_probe();

> +	}

...

> +	ret = drm_dev_register(drm, 0);
> +	if (ret) {
> +		dev_err(dev, "DRM device register failed: %d\n", ret);
> +		return ret;
> +	}

Ditto.

...

> +	{},

Comma is not needed in terminator entry.

...

> +static struct i2c_driver ssd130x_i2c_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = ssd130x_of_match,
> +	},
> +	.probe_new = ssd130x_probe,
> +	.remove = ssd130x_remove,
> +	.shutdown = ssd130x_shutdown,
> +};

> +

Redundant blank line.

> +module_i2c_driver(ssd130x_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas]

Hello Andy,

Thanks for your feedback.

On 2/4/22 15:26, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
>> Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
>> controllers that can be programmed via an I2C interface. This is a port
>> of the ssd1307fb driver that already supports these devices.
>>
>> A Device Tree binding is not added because the DRM driver is compatible
>> with the existing binding for the ssd1307fb driver.
> 
> ...
> 
>> +/*
>> + * DRM driver for Solomon SSD130X OLED displays
>> + *
>> + * Copyright 2022 Red Hat Inc.
>> + *
>> + * Based on drivers/video/fbdev/ssd1307fb.c
>> + * Copyright 2012 Free Electrons
> 
>> + *
> 
> No need for this blank line.
>

Ok.
 
>> + */
> 
> ...
> 
>> +struct ssd130x_device {
>> +	struct drm_device drm;
>> +	struct drm_simple_display_pipe pipe;
>> +	struct drm_display_mode mode;
>> +	struct drm_connector connector;
> 
> 
>> +	struct i2c_client *client;
> 
> Can we logically separate hw protocol vs hw interface from day 1, please?
> This will allow to add SPI support for this panel much easier.
> 
> Technically I would like to see here
> 
> 	struct device *dev;
>
> and probably (I haven't looked into design)
> 
> 	struct ssd130x_ops *ops;
> 
> or something alike.
>

Sure. I wanted to keep the driver simple, making the writes bus agnostic and
adding a level of indirection would make it more complex. But I agree that
it will also make easier to add more buses later. I will do that for v3.

[snip]

> 
>> +static inline int ssd130x_write_value(struct i2c_client *client, u8 value)
> 
> Not sure inline does anything useful here.
> Ditto for the rest similar cases.
>

Ok, I'll drop them.
 
> ...
> 
>> +static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
>> +				    /* u8 cmd, u8 value, ... */...)
>> +{
>> +	va_list ap;
>> +	u8 value;
>> +	int ret;
>> +
>> +	va_start(ap, count);
> 
>> +	while (count--) {
>> +		value = va_arg(ap, int);
>> +		ret = ssd130x_write_value(client, (u8)value);
>> +		if (ret)
>> +			goto out_end;
>> +	}
> 
> I'm wondering if this can be written in a form
> 
> 	do {
> 		...
> 	} while (--count);
> 
> In this case it will give a hint that count can't be 0.
>

Sure, I don't have a strong preference. I will change it.

[snip]
 
>> +	ssd130x->pwm = pwm_get(dev, NULL);
>> +	if (IS_ERR(ssd130x->pwm)) {
>> +		dev_err(dev, "Could not get PWM from device tree!\n");
> 
> "device tree" is a bit confusing here if I run this on ACPI.
> Maybe something like "firmware description"?
>

Indeed.
 
>> +		return PTR_ERR(ssd130x->pwm);
>> +	}
> 
> ...
> 
>> +	/* Set initial contrast */
>> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);
> 
> Creating a local variable for client allows to:
> - make lines shorter and might even be less LOCs
> - allow to convert struct device to client in one place
>   (as per my above comment)
> 
> Ditto for other similar cases.
>

Ok.
 
[snip]

>> +	/* Switch to horizontal addressing mode */
>> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
>> +				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> Can it be
> 
> 	return ssd130x_write_cmd(...);
> 
> ?
> 
> ...
>

Yes.

>> +	unsigned int line_length = DIV_ROUND_UP(width, 8);
>> +	unsigned int pages = DIV_ROUND_UP(height, 8);
> 
> For power of two there are more efficient roundup()/rounddown()
> (or with _ in the names, I don't remember by heart).
>

Oh, I didn't know about round_{up,down}(). Thanks a lot for the pointer.

> ...
> 
>> +			for (k = 0; k < m; k++) {
> 
>> +				u8 byte = buf[(8 * i + k) * line_length +
>> +					       j / 8];
> 
> One line?
>

Yes.

>> +				u8 bit = (byte >> (j % 8)) & 1;
>> +
>> +				data |= bit << k;
>> +			}
> 
> ...
> 
>> +static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +					   const struct drm_display_mode *mode)
>> +{
>> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
>> +
>> +	if (mode->hdisplay != ssd130x->mode.hdisplay &&
>> +	    mode->vdisplay != ssd130x->mode.vdisplay)
>> +		return MODE_ONE_SIZE;
> 
>> +	else if (mode->hdisplay != ssd130x->mode.hdisplay)
>> +		return MODE_ONE_WIDTH;
>> +	else if (mode->vdisplay != ssd130x->mode.vdisplay)
>> +		return MODE_ONE_HEIGHT;
> 
> 'else' in both cases is redundant.
>

Indeed.
 
>> +	return MODE_OK;
>> +}
> 
> ...
> 
>> +poweroff:
> 
> out_power_off: ?
>

Ok.
 
> ...
> 
>> +	if (!fb)
>> +		return;
> 
> Can it happen?
>

I don't know, but saw that the handler of other drivers checked for this so
preferred to play safe and do the same.
 
> ...
> 
>> +	drm_mode_probed_add(connector, mode);
>> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
>> +
>> +	return 1;
> 
> Positive code, what is the meaning of it?
>

It's the number of connector modes. The driver only supports 1.
 
> ...
> 
>> +	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
>> +		ssd130x->prechargep2 = 2;
> 
> You can drop conditionals for the optional properties
>


 
> 	ssd130x->prechargep2 = 2;
> 	device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2);
> 
> and so on for the similar.
>

Ok.
 
> ...
> 
>> +	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ssd130x->reset)) {
> 
>> +		ret = PTR_ERR(ssd130x->reset);
>> +		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
>> +		return ret;
> 
> Why not
> 
> 	return dev_err_probe()?
> 
> Each time you call it for deferred probe, it will spam logs.
>

Right. I'll change in all the places you pointed out.

[snip] 

> ...
> 
>> +	{},
> 
> Comma is not needed in terminator entry.
>

Right.
 
> ...
> 
>> +static struct i2c_driver ssd130x_i2c_driver = {
>> +	.driver = {
>> +		.name = DRIVER_NAME,
>> +		.of_match_table = ssd130x_of_match,
>> +	},
>> +	.probe_new = ssd130x_probe,
>> +	.remove = ssd130x_remove,
>> +	.shutdown = ssd130x_shutdown,
>> +};
> 
>> +
> 
> Redundant blank line.
>

Ok.
 
>> +module_i2c_driver(ssd130x_i2c_driver);
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas]

Hello Andy,

Thanks for your feedback.

On 2/4/22 15:26, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
>> Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
>> controllers that can be programmed via an I2C interface. This is a port
>> of the ssd1307fb driver that already supports these devices.
>>
>> A Device Tree binding is not added because the DRM driver is compatible
>> with the existing binding for the ssd1307fb driver.
> 
> ...
> 
>> +/*
>> + * DRM driver for Solomon SSD130X OLED displays
>> + *
>> + * Copyright 2022 Red Hat Inc.
>> + *
>> + * Based on drivers/video/fbdev/ssd1307fb.c
>> + * Copyright 2012 Free Electrons
> 
>> + *
> 
> No need for this blank line.
>

Ok.
 
>> + */
> 
> ...
> 
>> +struct ssd130x_device {
>> +	struct drm_device drm;
>> +	struct drm_simple_display_pipe pipe;
>> +	struct drm_display_mode mode;
>> +	struct drm_connector connector;
> 
> 
>> +	struct i2c_client *client;
> 
> Can we logically separate hw protocol vs hw interface from day 1, please?
> This will allow to add SPI support for this panel much easier.
> 
> Technically I would like to see here
> 
> 	struct device *dev;
>
> and probably (I haven't looked into design)
> 
> 	struct ssd130x_ops *ops;
> 
> or something alike.
>

Sure. I wanted to keep the driver simple, making the writes bus agnostic and
adding a level of indirection would make it more complex. But I agree that
it will also make easier to add more buses later. I will do that for v3.

[snip]

> 
>> +static inline int ssd130x_write_value(struct i2c_client *client, u8 value)
> 
> Not sure inline does anything useful here.
> Ditto for the rest similar cases.
>

Ok, I'll drop them.
 
> ...
> 
>> +static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
>> +				    /* u8 cmd, u8 value, ... */...)
>> +{
>> +	va_list ap;
>> +	u8 value;
>> +	int ret;
>> +
>> +	va_start(ap, count);
> 
>> +	while (count--) {
>> +		value = va_arg(ap, int);
>> +		ret = ssd130x_write_value(client, (u8)value);
>> +		if (ret)
>> +			goto out_end;
>> +	}
> 
> I'm wondering if this can be written in a form
> 
> 	do {
> 		...
> 	} while (--count);
> 
> In this case it will give a hint that count can't be 0.
>

Sure, I don't have a strong preference. I will change it.

[snip]
 
>> +	ssd130x->pwm = pwm_get(dev, NULL);
>> +	if (IS_ERR(ssd130x->pwm)) {
>> +		dev_err(dev, "Could not get PWM from device tree!\n");
> 
> "device tree" is a bit confusing here if I run this on ACPI.
> Maybe something like "firmware description"?
>

Indeed.
 
>> +		return PTR_ERR(ssd130x->pwm);
>> +	}
> 
> ...
> 
>> +	/* Set initial contrast */
>> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);
> 
> Creating a local variable for client allows to:
> - make lines shorter and might even be less LOCs
> - allow to convert struct device to client in one place
>   (as per my above comment)
> 
> Ditto for other similar cases.
>

Ok.
 
[snip]

>> +	/* Switch to horizontal addressing mode */
>> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
>> +				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> Can it be
> 
> 	return ssd130x_write_cmd(...);
> 
> ?
> 
> ...
>

Yes.

>> +	unsigned int line_length = DIV_ROUND_UP(width, 8);
>> +	unsigned int pages = DIV_ROUND_UP(height, 8);
> 
> For power of two there are more efficient roundup()/rounddown()
> (or with _ in the names, I don't remember by heart).
>

Oh, I didn't know about round_{up,down}(). Thanks a lot for the pointer.

> ...
> 
>> +			for (k = 0; k < m; k++) {
> 
>> +				u8 byte = buf[(8 * i + k) * line_length +
>> +					       j / 8];
> 
> One line?
>

Yes.

>> +				u8 bit = (byte >> (j % 8)) & 1;
>> +
>> +				data |= bit << k;
>> +			}
> 
> ...
> 
>> +static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +					   const struct drm_display_mode *mode)
>> +{
>> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
>> +
>> +	if (mode->hdisplay != ssd130x->mode.hdisplay &&
>> +	    mode->vdisplay != ssd130x->mode.vdisplay)
>> +		return MODE_ONE_SIZE;
> 
>> +	else if (mode->hdisplay != ssd130x->mode.hdisplay)
>> +		return MODE_ONE_WIDTH;
>> +	else if (mode->vdisplay != ssd130x->mode.vdisplay)
>> +		return MODE_ONE_HEIGHT;
> 
> 'else' in both cases is redundant.
>

Indeed.
 
>> +	return MODE_OK;
>> +}
> 
> ...
> 
>> +poweroff:
> 
> out_power_off: ?
>

Ok.
 
> ...
> 
>> +	if (!fb)
>> +		return;
> 
> Can it happen?
>

I don't know, but saw that the handler of other drivers checked for this so
preferred to play safe and do the same.
 
> ...
> 
>> +	drm_mode_probed_add(connector, mode);
>> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
>> +
>> +	return 1;
> 
> Positive code, what is the meaning of it?
>

It's the number of connector modes. The driver only supports 1.
 
> ...
> 
>> +	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
>> +		ssd130x->prechargep2 = 2;
> 
> You can drop conditionals for the optional properties
>


 
> 	ssd130x->prechargep2 = 2;
> 	device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2);
> 
> and so on for the similar.
>

Ok.
 
> ...
> 
>> +	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ssd130x->reset)) {
> 
>> +		ret = PTR_ERR(ssd130x->reset);
>> +		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
>> +		return ret;
> 
> Why not
> 
> 	return dev_err_probe()?
> 
> Each time you call it for deferred probe, it will spam logs.
>

Right. I'll change in all the places you pointed out.

[snip] 

> ...
> 
>> +	{},
> 
> Comma is not needed in terminator entry.
>

Right.
 
> ...
> 
>> +static struct i2c_driver ssd130x_i2c_driver = {
>> +	.driver = {
>> +		.name = DRIVER_NAME,
>> +		.of_match_table = ssd130x_of_match,
>> +	},
>> +	.probe_new = ssd130x_probe,
>> +	.remove = ssd130x_remove,
>> +	.shutdown = ssd130x_shutdown,
>> +};
> 
>> +
> 
> Redundant blank line.
>

Ok.
 
>> +module_i2c_driver(ssd130x_i2c_driver);
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 08:19:12PM +0100, Javier Martinez Canillas wrote:
> On 2/4/22 15:26, Andy Shevchenko wrote:
> > On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:

...

> >> +struct ssd130x_device {
> >> +	struct drm_device drm;
> >> +	struct drm_simple_display_pipe pipe;
> >> +	struct drm_display_mode mode;
> >> +	struct drm_connector connector;
> > 
> > 
> >> +	struct i2c_client *client;
> > 
> > Can we logically separate hw protocol vs hw interface from day 1, please?
> > This will allow to add SPI support for this panel much easier.
> > 
> > Technically I would like to see here
> > 
> > 	struct device *dev;
> >
> > and probably (I haven't looked into design)
> > 
> > 	struct ssd130x_ops *ops;
> > 
> > or something alike.
> 
> Sure. I wanted to keep the driver simple, making the writes bus agnostic and
> adding a level of indirection would make it more complex. But I agree that
> it will also make easier to add more buses later. I will do that for v3.

I have SSD1306 display with SPI interface and I'm not able to test your series.
With the above it at least gives me a point to consider helping (coding and
testing)  with SPI one.

...

> >> +	if (!fb)
> >> +		return;
> > 
> > Can it happen?
> 
> I don't know, but saw that the handler of other drivers checked for this so
> preferred to play safe and do the same.

So, either cargo-cult or indeed it may happen. Somebody may conduct a research
on this...

...

> >> +	drm_mode_probed_add(connector, mode);
> >> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
> >> +
> >> +	return 1;
> > 
> > Positive code, what is the meaning of it?
> 
> It's the number of connector modes. The driver only supports 1.

A comment then?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 08:19:12PM +0100, Javier Martinez Canillas wrote:
> On 2/4/22 15:26, Andy Shevchenko wrote:
> > On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:

...

> >> +struct ssd130x_device {
> >> +	struct drm_device drm;
> >> +	struct drm_simple_display_pipe pipe;
> >> +	struct drm_display_mode mode;
> >> +	struct drm_connector connector;
> > 
> > 
> >> +	struct i2c_client *client;
> > 
> > Can we logically separate hw protocol vs hw interface from day 1, please?
> > This will allow to add SPI support for this panel much easier.
> > 
> > Technically I would like to see here
> > 
> > 	struct device *dev;
> >
> > and probably (I haven't looked into design)
> > 
> > 	struct ssd130x_ops *ops;
> > 
> > or something alike.
> 
> Sure. I wanted to keep the driver simple, making the writes bus agnostic and
> adding a level of indirection would make it more complex. But I agree that
> it will also make easier to add more buses later. I will do that for v3.

I have SSD1306 display with SPI interface and I'm not able to test your series.
With the above it at least gives me a point to consider helping (coding and
testing)  with SPI one.

...

> >> +	if (!fb)
> >> +		return;
> > 
> > Can it happen?
> 
> I don't know, but saw that the handler of other drivers checked for this so
> preferred to play safe and do the same.

So, either cargo-cult or indeed it may happen. Somebody may conduct a research
on this...

...

> >> +	drm_mode_probed_add(connector, mode);
> >> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
> >> +
> >> +	return 1;
> > 
> > Positive code, what is the meaning of it?
> 
> It's the number of connector modes. The driver only supports 1.

A comment then?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas]

On 2/5/22 14:04, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 08:19:12PM +0100, Javier Martinez Canillas wrote:
>> On 2/4/22 15:26, Andy Shevchenko wrote:
>>> On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
> 
> ...
> 
>>>> +struct ssd130x_device {
>>>> +	struct drm_device drm;
>>>> +	struct drm_simple_display_pipe pipe;
>>>> +	struct drm_display_mode mode;
>>>> +	struct drm_connector connector;
>>>
>>>
>>>> +	struct i2c_client *client;
>>>
>>> Can we logically separate hw protocol vs hw interface from day 1, please?
>>> This will allow to add SPI support for this panel much easier.
>>>
>>> Technically I would like to see here
>>>
>>> 	struct device *dev;
>>>
>>> and probably (I haven't looked into design)
>>>
>>> 	struct ssd130x_ops *ops;
>>>
>>> or something alike.
>>
>> Sure. I wanted to keep the driver simple, making the writes bus agnostic and
>> adding a level of indirection would make it more complex. But I agree that
>> it will also make easier to add more buses later. I will do that for v3.
> 
> I have SSD1306 display with SPI interface and I'm not able to test your series.
> With the above it at least gives me a point to consider helping (coding and
> testing)  with SPI one.
>

Yes, I understand that. On the other hand, I only have a SSD1306 with an I2C
interface so I'm interested in supporting that. Then someone could extend to
support other buses :)

But I agree with you that making the driver easier to extend and using regmap
would be desirable. In fact, since I will add the level of indirection I can
got ahead and attempt to add the SPI support as well.

I won't be able to test but I can use drivers/staging/fbtft/fb_ssd1306.c as a
reference for this.

> ...
> 
>>>> +	if (!fb)
>>>> +		return;
>>>
>>> Can it happen?
>>
>> I don't know, but saw that the handler of other drivers checked for this so
>> preferred to play safe and do the same.
> 
> So, either cargo-cult or indeed it may happen. Somebody may conduct a research
> on this...
>

Someone familiar with the simple display pipe helpers should chime in, I tried
to grep around but couldn't figure out whether it was safe or not to assume the
struct drm_framebuffer won't ever be NULL in a struct drm_shadow_plane_state.

As mentioned other drivers were doing and I preferred to be defensive rather
than leading to a possible NULL pointer dereference.
 
> ...
> 
>>>> +	drm_mode_probed_add(connector, mode);
>>>> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
>>>> +
>>>> +	return 1;
>>>
>>> Positive code, what is the meaning of it?
>>
>> It's the number of connector modes. The driver only supports 1.
> 
> A comment then?
> 

Yes, that makes sense.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas]

On 2/5/22 14:04, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 08:19:12PM +0100, Javier Martinez Canillas wrote:
>> On 2/4/22 15:26, Andy Shevchenko wrote:
>>> On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
> 
> ...
> 
>>>> +struct ssd130x_device {
>>>> +	struct drm_device drm;
>>>> +	struct drm_simple_display_pipe pipe;
>>>> +	struct drm_display_mode mode;
>>>> +	struct drm_connector connector;
>>>
>>>
>>>> +	struct i2c_client *client;
>>>
>>> Can we logically separate hw protocol vs hw interface from day 1, please?
>>> This will allow to add SPI support for this panel much easier.
>>>
>>> Technically I would like to see here
>>>
>>> 	struct device *dev;
>>>
>>> and probably (I haven't looked into design)
>>>
>>> 	struct ssd130x_ops *ops;
>>>
>>> or something alike.
>>
>> Sure. I wanted to keep the driver simple, making the writes bus agnostic and
>> adding a level of indirection would make it more complex. But I agree that
>> it will also make easier to add more buses later. I will do that for v3.
> 
> I have SSD1306 display with SPI interface and I'm not able to test your series.
> With the above it at least gives me a point to consider helping (coding and
> testing)  with SPI one.
>

Yes, I understand that. On the other hand, I only have a SSD1306 with an I2C
interface so I'm interested in supporting that. Then someone could extend to
support other buses :)

But I agree with you that making the driver easier to extend and using regmap
would be desirable. In fact, since I will add the level of indirection I can
got ahead and attempt to add the SPI support as well.

I won't be able to test but I can use drivers/staging/fbtft/fb_ssd1306.c as a
reference for this.

> ...
> 
>>>> +	if (!fb)
>>>> +		return;
>>>
>>> Can it happen?
>>
>> I don't know, but saw that the handler of other drivers checked for this so
>> preferred to play safe and do the same.
> 
> So, either cargo-cult or indeed it may happen. Somebody may conduct a research
> on this...
>

Someone familiar with the simple display pipe helpers should chime in, I tried
to grep around but couldn't figure out whether it was safe or not to assume the
struct drm_framebuffer won't ever be NULL in a struct drm_shadow_plane_state.

As mentioned other drivers were doing and I preferred to be defensive rather
than leading to a possible NULL pointer dereference.
 
> ...
> 
>>>> +	drm_mode_probed_add(connector, mode);
>>>> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
>>>> +
>>>> +	return 1;
>>>
>>> Positive code, what is the meaning of it?
>>
>> It's the number of connector modes. The driver only supports 1.
> 
> A comment then?
> 

Yes, that makes sense.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Javier Martinez Canillas]

To make sure that tools like the get_maintainer.pl script will suggest
to Cc me if patches are posted for this driver.

Also include the Device Tree binding for the old ssd1307fb fbdev driver
since the new DRM driver was made compatible with the existing binding.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---

(no changes since v1)

 MAINTAINERS                         | 7 +++++++
 drivers/gpu/drm/drm_format_helper.c | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d03ad8da1f36..9061488a4113 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6102,6 +6102,13 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/repaper.txt
 F:	drivers/gpu/drm/tiny/repaper.c
 
+DRM DRIVER FOR SOLOMON SSD130X OLED DISPLAYS
+M:	Javier Martinez Canillas <javierm@redhat.com>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+F:	drivers/gpu/drm/tiny/ssd130x.c
+
 DRM DRIVER FOR QEMU'S CIRRUS DEVICE
 M:	Dave Airlie <airlied@redhat.com>
 M:	Gerd Hoffmann <kraxel@redhat.com>
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index cdce4b7c25d9..c3c1372fb771 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -661,6 +661,6 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
 		dst_pitch = drm_rect_width(clip);
 
 	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
-	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
+	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, clip);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
-- 
2.34.1

[PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Javier Martinez Canillas]

To make sure that tools like the get_maintainer.pl script will suggest
to Cc me if patches are posted for this driver.

Also include the Device Tree binding for the old ssd1307fb fbdev driver
since the new DRM driver was made compatible with the existing binding.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---

(no changes since v1)

 MAINTAINERS                         | 7 +++++++
 drivers/gpu/drm/drm_format_helper.c | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d03ad8da1f36..9061488a4113 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6102,6 +6102,13 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/repaper.txt
 F:	drivers/gpu/drm/tiny/repaper.c
 
+DRM DRIVER FOR SOLOMON SSD130X OLED DISPLAYS
+M:	Javier Martinez Canillas <javierm@redhat.com>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+F:	drivers/gpu/drm/tiny/ssd130x.c
+
 DRM DRIVER FOR QEMU'S CIRRUS DEVICE
 M:	Dave Airlie <airlied@redhat.com>
 M:	Gerd Hoffmann <kraxel@redhat.com>
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index cdce4b7c25d9..c3c1372fb771 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -661,6 +661,6 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
 		dst_pitch = drm_rect_width(clip);
 
 	drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
-	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
+	drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, clip);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
-- 
2.34.1

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:
> To make sure that tools like the get_maintainer.pl script will suggest
> to Cc me if patches are posted for this driver.
> 
> Also include the Device Tree binding for the old ssd1307fb fbdev driver
> since the new DRM driver was made compatible with the existing binding.

...

>  drivers/gpu/drm/drm_format_helper.c | 2 +-

Nothing about this in the commit message...

Stray change?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:
> To make sure that tools like the get_maintainer.pl script will suggest
> to Cc me if patches are posted for this driver.
> 
> Also include the Device Tree binding for the old ssd1307fb fbdev driver
> since the new DRM driver was made compatible with the existing binding.

...

>  drivers/gpu/drm/drm_format_helper.c | 2 +-

Nothing about this in the commit message...

Stray change?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Javier Martinez Canillas]

Hello Andy,

On 2/4/22 14:57, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:
>> To make sure that tools like the get_maintainer.pl script will suggest
>> to Cc me if patches are posted for this driver.
>>
>> Also include the Device Tree binding for the old ssd1307fb fbdev driver
>> since the new DRM driver was made compatible with the existing binding.
> 
> ...
> 
>>  drivers/gpu/drm/drm_format_helper.c | 2 +-
> 
> Nothing about this in the commit message...
> 
> Stray change?
> 

Sigh, I'm not sure how added that change. Just ignore it, I'll fix it
on v3 or when applying if there isn't another revision of this series.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Javier Martinez Canillas]

Hello Andy,

On 2/4/22 14:57, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:
>> To make sure that tools like the get_maintainer.pl script will suggest
>> to Cc me if patches are posted for this driver.
>>
>> Also include the Device Tree binding for the old ssd1307fb fbdev driver
>> since the new DRM driver was made compatible with the existing binding.
> 
> ...
> 
>>  drivers/gpu/drm/drm_format_helper.c | 2 +-
> 
> Nothing about this in the commit message...
> 
> Stray change?
> 

Sigh, I'm not sure how added that change. Just ignore it, I'll fix it
on v3 or when applying if there isn't another revision of this series.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 03:12:17PM +0100, Javier Martinez Canillas wrote:
> On 2/4/22 14:57, Andy Shevchenko wrote:
> > On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:

...

> > Stray change?
> 
> Sigh, I'm not sure how added that change. Just ignore it, I'll fix it
> on v3 or when applying if there isn't another revision of this series.

I believe v3 is warranted due to the other patch review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Andy Shevchenko]

On Fri, Feb 04, 2022 at 03:12:17PM +0100, Javier Martinez Canillas wrote:
> On 2/4/22 14:57, Andy Shevchenko wrote:
> > On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:

...

> > Stray change?
> 
> Sigh, I'm not sure how added that change. Just ignore it, I'll fix it
> on v3 or when applying if there isn't another revision of this series.

I believe v3 is warranted due to the other patch review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Javier Martinez Canillas]

On 2/4/22 15:28, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 03:12:17PM +0100, Javier Martinez Canillas wrote:
>> On 2/4/22 14:57, Andy Shevchenko wrote:
>>> On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:
> 
> ...
> 
>>> Stray change?
>>
>> Sigh, I'm not sure how added that change. Just ignore it, I'll fix it
>> on v3 or when applying if there isn't another revision of this series.
> 
> I believe v3 is warranted due to the other patch review.
> 

Agreed. Thanks a lot for your feedback and comments.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 3/4] MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver

[Javier Martinez Canillas]

On 2/4/22 15:28, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 03:12:17PM +0100, Javier Martinez Canillas wrote:
>> On 2/4/22 14:57, Andy Shevchenko wrote:
>>> On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote:
> 
> ...
> 
>>> Stray change?
>>
>> Sigh, I'm not sure how added that change. Just ignore it, I'll fix it
>> on v3 or when applying if there isn't another revision of this series.
> 
> I believe v3 is warranted due to the other patch review.
> 

Agreed. Thanks a lot for your feedback and comments.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 4/4] dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

[Javier Martinez Canillas]

The ssd130x DRM driver also makes use of this Device Tree binding to allow
existing users of the fbdev driver to migrate without the need to change
their Device Trees.

Add myself as another maintainer of the binding, to make sure that I will
be on Cc when patches are proposed for it.

Suggested-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
index 2ed2a7d0ca2f..9baafd0c42dd 100644
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
@@ -8,6 +8,7 @@ title: Solomon SSD1307 OLED Controller Framebuffer
 
 maintainers:
   - Maxime Ripard <mripard@kernel.org>
+  - Javier Martinez Canillas <javierm@redhat.com>
 
 properties:
   compatible:
-- 
2.34.1

[PATCH v2 4/4] dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

[Javier Martinez Canillas]

The ssd130x DRM driver also makes use of this Device Tree binding to allow
existing users of the fbdev driver to migrate without the need to change
their Device Trees.

Add myself as another maintainer of the binding, to make sure that I will
be on Cc when patches are proposed for it.

Suggested-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
index 2ed2a7d0ca2f..9baafd0c42dd 100644
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
@@ -8,6 +8,7 @@ title: Solomon SSD1307 OLED Controller Framebuffer
 
 maintainers:
   - Maxime Ripard <mripard@kernel.org>
+  - Javier Martinez Canillas <javierm@redhat.com>
 
 properties:
   compatible:
-- 
2.34.1

Re: [PATCH v2 4/4] dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

[Rob Herring]

On Fri, 04 Feb 2022 14:43:47 +0100, Javier Martinez Canillas wrote:
> The ssd130x DRM driver also makes use of this Device Tree binding to allow
> existing users of the fbdev driver to migrate without the need to change
> their Device Trees.
> 
> Add myself as another maintainer of the binding, to make sure that I will
> be on Cc when patches are proposed for it.
> 
> Suggested-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (no changes since v1)
> 
>  Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 4/4] dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

[Rob Herring]

On Fri, 04 Feb 2022 14:43:47 +0100, Javier Martinez Canillas wrote:
> The ssd130x DRM driver also makes use of this Device Tree binding to allow
> existing users of the fbdev driver to migrate without the need to change
> their Device Trees.
> 
> Add myself as another maintainer of the binding, to make sure that I will
> be on Cc when patches are proposed for it.
> 
> Suggested-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (no changes since v1)
> 
>  Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

[...]

> This is a v2 that addresses all the issues pointed in v1, thanks a lot
> to everyone that gave me feedback and reviews. I tried to not miss any
> comment, but there were a lot so forgive me if something is not there.

Thanks for the update!

> Changes in v2:

[...]

Note that the individual patches say "(no changes since v1)"?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

[...]

> This is a v2 that addresses all the issues pointed in v1, thanks a lot
> to everyone that gave me feedback and reviews. I tried to not miss any
> comment, but there were a lot so forgive me if something is not there.

Thanks for the update!

> Changes in v2:

[...]

Note that the individual patches say "(no changes since v1)"?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/4/22 15:31, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> [...]
> 
>> This is a v2 that addresses all the issues pointed in v1, thanks a lot
>> to everyone that gave me feedback and reviews. I tried to not miss any
>> comment, but there were a lot so forgive me if something is not there.
> 
> Thanks for the update!
>

You are welcome!
 
>> Changes in v2:
> 
> [...]
> 
> Note that the individual patches say "(no changes since v1)"?
> 

That's due patman (the tool I use to post patches) not being that smart.

I only added the v2 changelog in the cover letter and not the individual
patches to avoid adding noise, since there are a lot of changes since v1.

But patman then thought that means individual patches had no changes...

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/4/22 15:31, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> [...]
> 
>> This is a v2 that addresses all the issues pointed in v1, thanks a lot
>> to everyone that gave me feedback and reviews. I tried to not miss any
>> comment, but there were a lot so forgive me if something is not there.
> 
> Thanks for the update!
>

You are welcome!
 
>> Changes in v2:
> 
> [...]
> 
> Note that the individual patches say "(no changes since v1)"?
> 

That's due patman (the tool I use to post patches) not being that smart.

I only added the v2 changelog in the cover letter and not the individual
patches to avoid adding noise, since there are a lot of changes since v1.

But patman then thought that means individual patches had no changes...

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an
OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore.

Findings:
  - Kernel size increased by 349 KiB,
  - The "Memory:" line reports 412 KiB less memory,
  - On top of that, "free" shows ca. 92 KiB more memory in use after
    bootup.
  - The logo (I have a custom monochrome logo enabled) is no longer shown.
  - The screen is empty, with a (very very slow) flashing cursor in the
    middle of the screen, with a bogus long line next to it, which I can
    see being redrawn.
  - Writing text (e.g. hello) to /dev/tty0, I first see the text,
    followed by an enlargement of some of the characters.
  - "time ls" on the serial console (no files in the current directory,
    so nothing to print) increases from 0.86s to 1.92s, so the system is
    more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
    might be (partly) due to redrawing of the visual artefacts
    mentioned above.

So while the displays seems to be initialized correctly, it looks like
there are some serious bugs in the conversion from xrgb8888 to
monochrome.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an
OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore.

Findings:
  - Kernel size increased by 349 KiB,
  - The "Memory:" line reports 412 KiB less memory,
  - On top of that, "free" shows ca. 92 KiB more memory in use after
    bootup.
  - The logo (I have a custom monochrome logo enabled) is no longer shown.
  - The screen is empty, with a (very very slow) flashing cursor in the
    middle of the screen, with a bogus long line next to it, which I can
    see being redrawn.
  - Writing text (e.g. hello) to /dev/tty0, I first see the text,
    followed by an enlargement of some of the characters.
  - "time ls" on the serial console (no files in the current directory,
    so nothing to print) increases from 0.86s to 1.92s, so the system is
    more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
    might be (partly) due to redrawing of the visual artefacts
    mentioned above.

So while the displays seems to be initialized correctly, it looks like
there are some serious bugs in the conversion from xrgb8888 to
monochrome.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

Thanks a lot for testing!

On 2/8/22 15:19, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an
> OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore.
> 
> Findings:
>   - Kernel size increased by 349 KiB,
>   - The "Memory:" line reports 412 KiB less memory,
>   - On top of that, "free" shows ca. 92 KiB more memory in use after
>     bootup.
>   - The logo (I have a custom monochrome logo enabled) is no longer shown.

I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004

>   - The screen is empty, with a (very very slow) flashing cursor in the
>     middle of the screen, with a bogus long line next to it, which I can
>     see being redrawn.
>   - Writing text (e.g. hello) to /dev/tty0, I first see the text,
>     followed by an enlargement of some of the characters.


So far I was mostly testing using your fbtest repo tests and all of them
(modulo test009 that says "Screen size too small for this test").

But I've tried now using as a VT and I see the same visual artifacts. I
wonder what's the difference between fbcon and the way your tests use
the fbdev API.

>   - "time ls" on the serial console (no files in the current directory,
>     so nothing to print) increases from 0.86s to 1.92s, so the system is
>     more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
>     might be (partly) due to redrawing of the visual artefacts
>     mentioned above.
>

I was trying to first have the driver and then figure out how to optimize
it. For v3 I'm using regmap to access instead of the I2C layer directly.

I noticed that this is even slower but it makes the driver more clean and
allows to support both I2C and SPI (untested but will include it as a WIP).

> So while the displays seems to be initialized correctly, it looks like
> there are some serious bugs in the conversion from xrgb8888 to
> monochrome.
>

Yes, that's possible. I haven't tried to use it as a console before because
the display resolution is just too small. But will include now in my tests.

> Gr{oetje,eeting}s,
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

Thanks a lot for testing!

On 2/8/22 15:19, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an
> OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore.
> 
> Findings:
>   - Kernel size increased by 349 KiB,
>   - The "Memory:" line reports 412 KiB less memory,
>   - On top of that, "free" shows ca. 92 KiB more memory in use after
>     bootup.
>   - The logo (I have a custom monochrome logo enabled) is no longer shown.

I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004

>   - The screen is empty, with a (very very slow) flashing cursor in the
>     middle of the screen, with a bogus long line next to it, which I can
>     see being redrawn.
>   - Writing text (e.g. hello) to /dev/tty0, I first see the text,
>     followed by an enlargement of some of the characters.


So far I was mostly testing using your fbtest repo tests and all of them
(modulo test009 that says "Screen size too small for this test").

But I've tried now using as a VT and I see the same visual artifacts. I
wonder what's the difference between fbcon and the way your tests use
the fbdev API.

>   - "time ls" on the serial console (no files in the current directory,
>     so nothing to print) increases from 0.86s to 1.92s, so the system is
>     more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
>     might be (partly) due to redrawing of the visual artefacts
>     mentioned above.
>

I was trying to first have the driver and then figure out how to optimize
it. For v3 I'm using regmap to access instead of the I2C layer directly.

I noticed that this is even slower but it makes the driver more clean and
allows to support both I2C and SPI (untested but will include it as a WIP).

> So while the displays seems to be initialized correctly, it looks like
> there are some serious bugs in the conversion from xrgb8888 to
> monochrome.
>

Yes, that's possible. I haven't tried to use it as a console before because
the display resolution is just too small. But will include now in my tests.

> Gr{oetje,eeting}s,
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
> On 2/8/22 15:19, Geert Uytterhoeven wrote:

> >   - "time ls" on the serial console (no files in the current directory,
> >     so nothing to print) increases from 0.86s to 1.92s, so the system is
> >     more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
> >     might be (partly) due to redrawing of the visual artefacts
> >     mentioned above.

> I was trying to first have the driver and then figure out how to optimize
> it. For v3 I'm using regmap to access instead of the I2C layer directly.

> I noticed that this is even slower but it makes the driver more clean and
> allows to support both I2C and SPI (untested but will include it as a WIP).

I wouldn't have expected regmap to add huge overhead relative to I2C,
partly predicated on I2C being rather slow itself.  There will be some
overhead for concurrency protection and data marshalling but for I2C
clocked at normal speeds it's surprising.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
> On 2/8/22 15:19, Geert Uytterhoeven wrote:

> >   - "time ls" on the serial console (no files in the current directory,
> >     so nothing to print) increases from 0.86s to 1.92s, so the system is
> >     more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
> >     might be (partly) due to redrawing of the visual artefacts
> >     mentioned above.

> I was trying to first have the driver and then figure out how to optimize
> it. For v3 I'm using regmap to access instead of the I2C layer directly.

> I noticed that this is even slower but it makes the driver more clean and
> allows to support both I2C and SPI (untested but will include it as a WIP).

I wouldn't have expected regmap to add huge overhead relative to I2C,
partly predicated on I2C being rather slow itself.  There will be some
overhead for concurrency protection and data marshalling but for I2C
clocked at normal speeds it's surprising.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Mark,

On 2/8/22 16:18, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
>> On 2/8/22 15:19, Geert Uytterhoeven wrote:
> 
>>>   - "time ls" on the serial console (no files in the current directory,
>>>     so nothing to print) increases from 0.86s to 1.92s, so the system is
>>>     more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
>>>     might be (partly) due to redrawing of the visual artefacts
>>>     mentioned above.
> 
>> I was trying to first have the driver and then figure out how to optimize
>> it. For v3 I'm using regmap to access instead of the I2C layer directly.
> 
>> I noticed that this is even slower but it makes the driver more clean and
>> allows to support both I2C and SPI (untested but will include it as a WIP).
> 
> I wouldn't have expected regmap to add huge overhead relative to I2C,
> partly predicated on I2C being rather slow itself.  There will be some
> overhead for concurrency protection and data marshalling but for I2C
> clocked at normal speeds it's surprising.

Thanks for chiming in. That's good to know, I'll investigate more then.

Probably I was wrongly blaming regmap while it was another change that
is causing the display to be refreshed at a slower rate than before.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Mark,

On 2/8/22 16:18, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
>> On 2/8/22 15:19, Geert Uytterhoeven wrote:
> 
>>>   - "time ls" on the serial console (no files in the current directory,
>>>     so nothing to print) increases from 0.86s to 1.92s, so the system is
>>>     more loaded.  As ssd1307fb relied on deferred I/O too, the slowdown
>>>     might be (partly) due to redrawing of the visual artefacts
>>>     mentioned above.
> 
>> I was trying to first have the driver and then figure out how to optimize
>> it. For v3 I'm using regmap to access instead of the I2C layer directly.
> 
>> I noticed that this is even slower but it makes the driver more clean and
>> allows to support both I2C and SPI (untested but will include it as a WIP).
> 
> I wouldn't have expected regmap to add huge overhead relative to I2C,
> partly predicated on I2C being rather slow itself.  There will be some
> overhead for concurrency protection and data marshalling but for I2C
> clocked at normal speeds it's surprising.

Thanks for chiming in. That's good to know, I'll investigate more then.

Probably I was wrongly blaming regmap while it was another change that
is causing the display to be refreshed at a slower rate than before.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Tue, Feb 8, 2022 at 4:10 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 2/8/22 15:19, Geert Uytterhoeven wrote:
> > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> >> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> >
> > I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an
> > OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore.
> >
> > Findings:
> >   - Kernel size increased by 349 KiB,
> >   - The "Memory:" line reports 412 KiB less memory,
> >   - On top of that, "free" shows ca. 92 KiB more memory in use after
> >     bootup.
> >   - The logo (I have a custom monochrome logo enabled) is no longer shown.
>
> I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004

I meant the kernel's logo (FB_LOGO_*),. Obviously you need to enable
a smaller one, as the default 80x80 logo is too large, and thus can't
be drawn on your 128x64 or my 128x32 display.

> >   - The screen is empty, with a (very very slow) flashing cursor in the
> >     middle of the screen, with a bogus long line next to it, which I can
> >     see being redrawn.
> >   - Writing text (e.g. hello) to /dev/tty0, I first see the text,
> >     followed by an enlargement of some of the characters.
>
> So far I was mostly testing using your fbtest repo tests and all of them
> (modulo test009 that says "Screen size too small for this test").
>
> But I've tried now using as a VT and I see the same visual artifacts. I
> wonder what's the difference between fbcon and the way your tests use
> the fbdev API.

Fbcon does small writes to the shadow frame buffer, while fbtest
writes to the mmap()ed /dev/fbX, causing a full page to be updated.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Tue, Feb 8, 2022 at 4:10 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 2/8/22 15:19, Geert Uytterhoeven wrote:
> > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> >> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> >
> > I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an
> > OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore.
> >
> > Findings:
> >   - Kernel size increased by 349 KiB,
> >   - The "Memory:" line reports 412 KiB less memory,
> >   - On top of that, "free" shows ca. 92 KiB more memory in use after
> >     bootup.
> >   - The logo (I have a custom monochrome logo enabled) is no longer shown.
>
> I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004

I meant the kernel's logo (FB_LOGO_*),. Obviously you need to enable
a smaller one, as the default 80x80 logo is too large, and thus can't
be drawn on your 128x64 or my 128x32 display.

> >   - The screen is empty, with a (very very slow) flashing cursor in the
> >     middle of the screen, with a bogus long line next to it, which I can
> >     see being redrawn.
> >   - Writing text (e.g. hello) to /dev/tty0, I first see the text,
> >     followed by an enlargement of some of the characters.
>
> So far I was mostly testing using your fbtest repo tests and all of them
> (modulo test009 that says "Screen size too small for this test").
>
> But I've tried now using as a VT and I see the same visual artifacts. I
> wonder what's the difference between fbcon and the way your tests use
> the fbdev API.

Fbcon does small writes to the shadow frame buffer, while fbtest
writes to the mmap()ed /dev/fbX, causing a full page to be updated.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/8/22 16:23, Geert Uytterhoeven wrote:

[snip]

>>>   - The logo (I have a custom monochrome logo enabled) is no longer shown.
>>
>> I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004
> 
> I meant the kernel's logo (FB_LOGO_*),. Obviously you need to enable
> a smaller one, as the default 80x80 logo is too large, and thus can't
> be drawn on your 128x64 or my 128x32 display.
>

That makes sense.
 
>>>   - The screen is empty, with a (very very slow) flashing cursor in the
>>>     middle of the screen, with a bogus long line next to it, which I can
>>>     see being redrawn.
>>>   - Writing text (e.g. hello) to /dev/tty0, I first see the text,
>>>     followed by an enlargement of some of the characters.
>>
>> So far I was mostly testing using your fbtest repo tests and all of them
>> (modulo test009 that says "Screen size too small for this test").
>>
>> But I've tried now using as a VT and I see the same visual artifacts. I
>> wonder what's the difference between fbcon and the way your tests use
>> the fbdev API.
> 
> Fbcon does small writes to the shadow frame buffer, while fbtest
> writes to the mmap()ed /dev/fbX, causing a full page to be updated.
>

I see. Thanks for the information.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/8/22 16:23, Geert Uytterhoeven wrote:

[snip]

>>>   - The logo (I have a custom monochrome logo enabled) is no longer shown.
>>
>> I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004
> 
> I meant the kernel's logo (FB_LOGO_*),. Obviously you need to enable
> a smaller one, as the default 80x80 logo is too large, and thus can't
> be drawn on your 128x64 or my 128x32 display.
>

That makes sense.
 
>>>   - The screen is empty, with a (very very slow) flashing cursor in the
>>>     middle of the screen, with a bogus long line next to it, which I can
>>>     see being redrawn.
>>>   - Writing text (e.g. hello) to /dev/tty0, I first see the text,
>>>     followed by an enlargement of some of the characters.
>>
>> So far I was mostly testing using your fbtest repo tests and all of them
>> (modulo test009 that says "Screen size too small for this test").
>>
>> But I've tried now using as a VT and I see the same visual artifacts. I
>> wonder what's the difference between fbcon and the way your tests use
>> the fbdev API.
> 
> Fbcon does small writes to the shadow frame buffer, while fbtest
> writes to the mmap()ed /dev/fbX, causing a full page to be updated.
>

I see. Thanks for the information.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/8/22 16:40, Javier Martinez Canillas wrote:
> On 2/8/22 16:23, Geert Uytterhoeven wrote:

[snip]

>>
>> Fbcon does small writes to the shadow frame buffer, while fbtest
>> writes to the mmap()ed /dev/fbX, causing a full page to be updated.
>>
> 
> I see. Thanks for the information.
> 

I found the bug. Partial updates where indeed broken and only a full
screen update was working. I didn't notice because where using your
fbtests that mmap and do a full update.

Thanks a lot for reporting this, the issue should be fixed in v3.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/8/22 16:40, Javier Martinez Canillas wrote:
> On 2/8/22 16:23, Geert Uytterhoeven wrote:

[snip]

>>
>> Fbcon does small writes to the shadow frame buffer, while fbtest
>> writes to the mmap()ed /dev/fbX, causing a full page to be updated.
>>
> 
> I see. Thanks for the information.
> 

I found the bug. Partial updates where indeed broken and only a full
screen update was working. I didn't notice because where using your
fbtests that mmap and do a full update.

Thanks a lot for reporting this, the issue should be fixed in v3.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
> On 2/8/22 15:19, Geert Uytterhoeven wrote:
> > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:

> >   - Kernel size increased by 349 KiB,
> >   - The "Memory:" line reports 412 KiB less memory,
> >   - On top of that, "free" shows ca. 92 KiB more memory in use after
> >     bootup.

The memory consumption should really be taken seriously, because these kind of
displays are for embedded platforms with limited amount of resources.

Thanks, Geert, for testing and reporting this issue in particular.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
> On 2/8/22 15:19, Geert Uytterhoeven wrote:
> > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:

> >   - Kernel size increased by 349 KiB,
> >   - The "Memory:" line reports 412 KiB less memory,
> >   - On top of that, "free" shows ca. 92 KiB more memory in use after
> >     bootup.

The memory consumption should really be taken seriously, because these kind of
displays are for embedded platforms with limited amount of resources.

Thanks, Geert, for testing and reporting this issue in particular.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Andy,

On Wed, Feb 9, 2022 at 2:48 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
> > On 2/8/22 15:19, Geert Uytterhoeven wrote:
> > > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> > > <javierm@redhat.com> wrote:
> > >   - Kernel size increased by 349 KiB,
> > >   - The "Memory:" line reports 412 KiB less memory,
> > >   - On top of that, "free" shows ca. 92 KiB more memory in use after
> > >     bootup.
>
> The memory consumption should really be taken seriously, because these kind of
> displays are for embedded platforms with limited amount of resources.

Thanks for your concern!

Looking at the options that are auto-enabled, a few stand out that
look like they're not needed on systems witch such small displays,
or on legacy systems predating DDC:

    menuconfig DRM
            tristate "Direct Rendering Manager (XFree86 4.1.0 and
higher DRI support)"
            depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
            select DRM_NOMODESET
            select DRM_PANEL_ORIENTATION_QUIRKS
            select HDMI

Not everyone pays HDMI royalties ;-)

            select FB_CMDLINE
            select I2C
            select I2C_ALGOBIT

I do need I2C, as it's the transport for my SSD1306 display, but not
everyone needs it.

            select DMA_SHARED_BUFFER
            select SYNC_FILE
    # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
    # device and dmabuf fd. Let's make sure that is available for our userspace.
            select KCMP

And:

    config DRM_BRIDGE
            def_bool y
            depends on DRM
            help
              Bridge registration and lookup framework.

    config DRM_PANEL_BRIDGE
            def_bool y


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Andy,

On Wed, Feb 9, 2022 at 2:48 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
> > On 2/8/22 15:19, Geert Uytterhoeven wrote:
> > > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
> > > <javierm@redhat.com> wrote:
> > >   - Kernel size increased by 349 KiB,
> > >   - The "Memory:" line reports 412 KiB less memory,
> > >   - On top of that, "free" shows ca. 92 KiB more memory in use after
> > >     bootup.
>
> The memory consumption should really be taken seriously, because these kind of
> displays are for embedded platforms with limited amount of resources.

Thanks for your concern!

Looking at the options that are auto-enabled, a few stand out that
look like they're not needed on systems witch such small displays,
or on legacy systems predating DDC:

    menuconfig DRM
            tristate "Direct Rendering Manager (XFree86 4.1.0 and
higher DRI support)"
            depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
            select DRM_NOMODESET
            select DRM_PANEL_ORIENTATION_QUIRKS
            select HDMI

Not everyone pays HDMI royalties ;-)

            select FB_CMDLINE
            select I2C
            select I2C_ALGOBIT

I do need I2C, as it's the transport for my SSD1306 display, but not
everyone needs it.

            select DMA_SHARED_BUFFER
            select SYNC_FILE
    # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
    # device and dmabuf fd. Let's make sure that is available for our userspace.
            select KCMP

And:

    config DRM_BRIDGE
            def_bool y
            depends on DRM
            help
              Bridge registration and lookup framework.

    config DRM_PANEL_BRIDGE
            def_bool y


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/9/22 15:27, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Wed, Feb 9, 2022 at 2:48 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
>>> On 2/8/22 15:19, Geert Uytterhoeven wrote:
>>>> On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
>>>> <javierm@redhat.com> wrote:
>>>>   - Kernel size increased by 349 KiB,
>>>>   - The "Memory:" line reports 412 KiB less memory,
>>>>   - On top of that, "free" shows ca. 92 KiB more memory in use after
>>>>     bootup.
>>
>> The memory consumption should really be taken seriously, because these kind of
>> displays are for embedded platforms with limited amount of resources.
> 
> Thanks for your concern!
> 
> Looking at the options that are auto-enabled, a few stand out that
> look like they're not needed on systems witch such small displays,
> or on legacy systems predating DDC:

Thanks for your analysis.

Since drivers are replacing the {simple,efi}fb drivers and others with the
simpledrm driver, the DRM subsystem is now built into the kernel and no
longer a loadable module.

So there has been some effort to make it more modular and smaller, as an
example the following patch-set from Thomas:

https://www.spinics.net/lists/dri-devel/msg329120.html

But there are still a lot of room to reduce this and certainly enabling
CONFIG_DRM will be noticeable for such memory constrainted systems.

This is outside the scope of this patch series though, that is only about
adding a new DRM driver :)

Now, this is a reason why I mentioned that the old fbdev driver shouldn't
be removed yet.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/9/22 15:27, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Wed, Feb 9, 2022 at 2:48 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote:
>>> On 2/8/22 15:19, Geert Uytterhoeven wrote:
>>>> On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas
>>>> <javierm@redhat.com> wrote:
>>>>   - Kernel size increased by 349 KiB,
>>>>   - The "Memory:" line reports 412 KiB less memory,
>>>>   - On top of that, "free" shows ca. 92 KiB more memory in use after
>>>>     bootup.
>>
>> The memory consumption should really be taken seriously, because these kind of
>> displays are for embedded platforms with limited amount of resources.
> 
> Thanks for your concern!
> 
> Looking at the options that are auto-enabled, a few stand out that
> look like they're not needed on systems witch such small displays,
> or on legacy systems predating DDC:

Thanks for your analysis.

Since drivers are replacing the {simple,efi}fb drivers and others with the
simpledrm driver, the DRM subsystem is now built into the kernel and no
longer a loadable module.

So there has been some effort to make it more modular and smaller, as an
example the following patch-set from Thomas:

https://www.spinics.net/lists/dri-devel/msg329120.html

But there are still a lot of room to reduce this and certainly enabling
CONFIG_DRM will be noticeable for such memory constrainted systems.

This is outside the scope of this patch series though, that is only about
adding a new DRM driver :)

Now, this is a reason why I mentioned that the old fbdev driver shouldn't
be removed yet.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 09, 2022 at 03:42:16PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 15:27, Geert Uytterhoeven wrote:

...

> Now, this is a reason why I mentioned that the old fbdev driver shouldn't
> be removed yet.

I agree on this conclusion.

I think based on the fbtft resurrection discussion I can send a new version
to unorphan it, route via fbdev, and leave under staging, so it will be a
compromise between all stakeholders.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 09, 2022 at 03:42:16PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 15:27, Geert Uytterhoeven wrote:

...

> Now, this is a reason why I mentioned that the old fbdev driver shouldn't
> be removed yet.

I agree on this conclusion.

I think based on the fbtft resurrection discussion I can send a new version
to unorphan it, route via fbdev, and leave under staging, so it will be a
compromise between all stakeholders.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Wed, Feb 09, 2022 at 05:32:15PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 09, 2022 at 03:42:16PM +0100, Javier Martinez Canillas wrote:
> > On 2/9/22 15:27, Geert Uytterhoeven wrote:
> 
> ...
> 
> > Now, this is a reason why I mentioned that the old fbdev driver shouldn't
> > be removed yet.
> 
> I agree on this conclusion.
> 
> I think based on the fbtft resurrection discussion I can send a new version
> to unorphan it, route via fbdev, and leave under staging, so it will be a
> compromise between all stakeholders.

The DT bindings still don't belong anywhere in the main tree.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Wed, Feb 09, 2022 at 05:32:15PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 09, 2022 at 03:42:16PM +0100, Javier Martinez Canillas wrote:
> > On 2/9/22 15:27, Geert Uytterhoeven wrote:
> 
> ...
> 
> > Now, this is a reason why I mentioned that the old fbdev driver shouldn't
> > be removed yet.
> 
> I agree on this conclusion.
> 
> I think based on the fbtft resurrection discussion I can send a new version
> to unorphan it, route via fbdev, and leave under staging, so it will be a
> compromise between all stakeholders.

The DT bindings still don't belong anywhere in the main tree.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]