Add support for CH1115 Controller
Subsystems (3)
v5 cover Add support for CH1115 Controller
This series add support for the CH1115 Controller [1]. It is a monochrome dot matrix OLED controller that supports several interfaces such as SPI and I2C.
This driver only supports the I2C interface, but all common parts could easily be put into a common file to be used with other interfaces. I only have I2C to test with.
This is the first driver I write. The work is based on the driver located at:
- drivers/gpu/drm/sitronix/st7571-i2c.c
- drivers/gpu/drm/sitronix/st7571.c
Tested with Raspberry Pi 3b+.
Changelog:
v5:
General:
- Patch series based on 7.3-rc1
Driver:
- Fixed KConfig dependencies (OF and BACKLIGHT_CLASS_DEVICE)
- Initializing baclight with backlight_properties struct
- Moved driver from drivers/gpu/drm/chipwealth to drivers/gpu/drm/tiny
- Fixed allocation for hwbuf if width or height are not multiple of 8. I was allocating less memory than needed in those cases.
- Fixed damage rect horizontal alignment.
Note: The last two fixes listed were pointed out by Sashiko in some occasions but when I checked them I dismissed them for false positives because I misunderstood them.
Note 2: Sashiko keeps complaining that i2c transfers are called in atomic_update callback stating that it is forbidden to call sleeping functions there because we are in an atomic context. AFAIK this atomic context is not that the kernel is in a non-sleepable state, but refers to the DRM way of working by doing check -> commit. In fact, the driver I based this driver (st7571) and others (ssd103x) do i2c transfer in an atomic plane update. Am I missing something?
v4: [6]
General:
- Patch series based in kernel v7.3-rc1
Driver:
- Fixed nits found by Christophe JAILLET.
- Fixed max_width and max_height in modeset to avoid having bigger framebuffers than resolution.
v3: [5]
- General:
- Patch series based in kernel v7.2
- In dt-bindings:
- Removed "chipwealth,contrast" property (added backlight_device support in driver's code)
- Renamed "chipwealth,invert" -> "chipwealth,inverted"
- Added default value to "chipwealth,offset"
- Removed 'status = "okay"' from example
- Indented example with 4 spaces as stated in the documentation [4]
- In driver code:
- Added support for backlight_device
- Replaced drm_dev_unregister with drm_dev_unplug to cover the dt overlay unload case
- Executing drm shutdown operations before turning off the display
- Checking that the width (panel-timing.hactive) and height (panel-timing.vactive) parameters are greater than 0
v2: [3]
- General:
- Patch series based in kernel v7.2
- In dt-bindings:
- Fixed linter problems
- Replaced first_page property with offset
- In driver code:
- Fixed dst_pitch calculation
- Renamed file from ch1115.c to ch1115-i2c.c
- Support flip-vertical and flip-horizontal dt properties
- Fixed several things found by sashiko-ai
v1: [2]
[1] https://datasheet4u.com/download/1576606/CH1115.html
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://docs.kernel.org/devicetree/bindings/writing-schema.html#annotated-example-schema
[5] https://lore.kernel.org/all/[email protected]/
[6] https://lore.kernel.org/all/[email protected]/
Nicolás Antinori (3):
dt-bindings: display: add Chipwealth CH1115 OLED Controller drm/ch1115: add support for Chipwealth CH1115 OLED controller
MAINTAINERS: add entry for Chipwealth CH1115 OLED Controller
.../bindings/display/chipwealth,ch1115.yaml | 81 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 6 + drivers/gpu/drm/tiny/Kconfig | 17 + drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/ch1115-i2c.c | 895 ++++++++++++++++++ 6 files changed, 1002 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/chipwealth,ch1115.yaml create mode 100644 drivers/gpu/drm/tiny/ch1115-i2c.c
--
2.47.3
PATCH v5 1/3 dt-bindings: display: add Chipwealth CH1115 OLED Controller
Chipwealth CH1115 is a controller for monochrome dot matrix OLED panels.
---
Notes:
- Patch series based on 7.3-rc1
- Carried "Reviewed-by: Krzysztof Kozlowski <...>" from v3 since this patch did not change.
.../bindings/display/chipwealth,ch1115.yaml | 81 +++++++++++++++++++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + 2 files changed, 83 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/chipwealth,ch1115.yaml
Documentation/devicetree/bindings/display/chipwealth,ch1115.yaml new file +81 -0
@@ -0,0 +1,81 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/chipwealth,ch1115.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Chipwealth CH1115 Display Controller + +maintainers: + - Nicolás Antinori <[email protected]> + +description: + Chipwealth CH1115 is a controller for monochrome dot matrix OLED + panels. + + https://datasheet4u.com/download/1576606/CH1115.html + +allOf: + - $ref: panel/panel-common.yaml# + +properties: + compatible: + const: chipwealth,ch1115 + + reg: + maxItems: 1 + + chipwealth,inverted: + type: boolean + description: + Display pixels are inverted, i.e. 0 is white and 1 is black. + + chipwealth,offset: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 63 + default: 0 + description: + Display start line offset. Specifies the vertical alignment and row shift + along the Y-coordinate. + + width-mm: true + height-mm: true + flip-horizontal: true + flip-vertical: true + panel-timing: true + +required: + - compatible + - reg + - width-mm + - height-mm + - panel-timing + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + display@3c { + compatible = "chipwealth,ch1115"; + reg = <0x3c>; + width-mm = <12>; + height-mm = <8>; + chipwealth,offset = <8>; + panel-timing { + clock-frequency = <0>; + hactive = <88>; + vactive = <48>; + hfront-porch = <0>; + hback-porch = <0>; + hsync-len = <0>; + vsync-len = <0>; + vfront-porch = <0>; + vback-porch = <0>; + }; + }; + };
Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -0
@@ -331,6 +331,8 @@ patternProperties: description: ChipOne "^chipspark,.*": description: ChipSPARK + "^chipwealth,.*": + description: Chip Wealth Technology, Ltd. "^chongzhou,.*": description: Shenzhen Chongzhou Electronic Technology Co., Ltd "^chrontel,.*":
--
2.47.3
PATCH v5 2/3 drm/ch1115: add support for Chipwealth CH1115 OLED controller
In reply to Nicolás AntinoriChipwealth CH1115 is a controller for monochrome dot matrix OLED panels.
Add DRM support for this chip using i2c bus.
--- drivers/gpu/drm/tiny/Kconfig | 17 + drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/ch1115-i2c.c | 895 ++++++++++++++++++++++++++++++ 3 files changed, 913 insertions(+) create mode 100644 drivers/gpu/drm/tiny/ch1115-i2c.c
drivers/gpu/drm/tiny/Kconfig +17 -0
@@ -36,6 +36,23 @@ config DRM_BOCHS If M is selected the module will be called bochs. +config DRM_CH1115_I2C + tristate "DRM support for Ch1115 OLED panels (I2C BUS)" + depends on DRM && I2C && OF + select BACKLIGHT_CLASS_DEVICE + select DRM_CLIENT_SELECTION + select DRM_GEM_SHMEM_HELPER + select DRM_KMS_HELPER + select REGMAP_I2C + select VIDEOMODE_HELPERS + help + Chipwealth CH1115 is a controller for monochrome dot matrix OLED + panels. + + DRM Driver for Ch1115 OLED panels connected via I2C. + + If M is selected the module will be called ch1115-i2c. + config DRM_CIRRUS_QEMU tristate "Cirrus driver for QEMU emulated device" depends on DRM && PCI
drivers/gpu/drm/tiny/Makefile +1 -0
@@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_APPLETBDRM) += appletbdrm.o obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o obj-$(CONFIG_DRM_BOCHS) += bochs.o +obj-$(CONFIG_DRM_CH1115_I2C) += ch1115-i2c.o obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus-qemu.o obj-$(CONFIG_DRM_GM12U320) += gm12u320.o obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
drivers/gpu/drm/tiny/ch1115-i2c.c new file +895 -0
895 lines not on this page · raw message
--
2.47.3
[email protected]
AI reviewRe: [PATCH v5 2/3] drm/ch1115: add support for Chipwealth CH1115 OLED controller
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] core: uninitialized variable `bl` used in IS_ERR check - [High] drm: out-of-bounds heap read in…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] core: uninitialized variable bl used in IS_ERR check
- [High] drm: out-of-bounds heap read in ch1115_transform_xy()
- [High] drm: sleeping in atomic context during plane and encoder callbacks
- [Medium] sysfs: unprotected state modification on registered I2C device --
commit 63d6db1725e38b56da7d2687e33c28c0f46a0455
Author: Nicolás Antinori <[email protected]>
drm/ch1115: add support for Chipwealth CH1115 OLED controller
This commit adds DRM support for the Chipwealth CH1115 monochrome dot matrix OLED controller connected via an I2C bus.
Quoted 4 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +0
-0
[ ... ]
Quoted 10 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +10
-0+static inline u8 ch1115_transform_xy (const char *p,int x,int y,u8 bytes_per_row) +{ +int xrest = x %8 ; +u8 result =0 ; + +/* +* Transforms an (x, y) pixel coordinate into a vertical 8-bit +* column from the framebuffer. It calculates the corresponding byte in the +* framebuffer, extracts the bit at the given x position across 8 consecutive +* rows, and packs those bits into a single byte.
drivers/gpu/drm/tiny/ch1115-i2c.c line 272
Quoted 10 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +10
-0+* +* Return an 8-bit value representing a vertical column of pixels. +*/ + + x = x /8 ; + y = (y /8 ) *8 ; + +for (int i =0 ; i <8 ; i++) { +int row_idx = y + i; +u8 byte = p[row_idx * bytes_per_row + x];
[Severity: High]
Can this result in an out-of-bounds memory read?
In ch1115_probe(), the hardware buffer ch1115->hwbuf is allocated with a size of bytes_per_row * ch1115->height.
Here, the loop unconditionally iterates 8 times to pack 8 vertical pixels. If ch1115->height is not a multiple of 8, the final loop iterations will calculate a row_idx that exceeds the allocated height, causing a heap out-of-bounds read.
Quoted 7 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +7
-0+u8 bit = (byte >> xrest) &1 ; + + result |= (bit << i); + } + +return result; +}
[ ... ]
Quoted 8 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +8
-0+static void ch1115_primary_plane_helper_atomic_update (struct drm_plane *plane, +struct drm_atomic_commit *state) +{ +struct drm_plane_state *old_plane_state =drm_atomic_get_old_plane_state (state, plane); +struct drm_plane_state *plane_state =drm_atomic_get_new_plane_state (state, plane); +struct drm_shadow_plane_state *shadow_plane_state =to_drm_shadow_plane_state (plane_state); +struct drm_framebuffer *fb = plane_state->fb; +struct drm_atomic_helper_damage_iter iter;
drivers/gpu/drm/tiny/ch1115-i2c.c line 404
Quoted 10 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +10
-0+struct drm_device *dev = plane->dev; +struct drm_rect damage; +struct ch1115_device *ch1115 =drm_to_ch1115 (plane->dev); +int idx; + +if (!fb) +return ; + +if (drm_gem_fb_begin_cpu_access (fb,DMA_FROM_DEVICE )) +return ;
[Severity: High]
Does this execute sleepable functions in atomic context?
Plane atomic_update callbacks run in atomic context during non-blocking commits, so sleeping is forbidden. Here, drm_gem_fb_begin_cpu_access() is called, which waits for fences and can sleep.
Additionally, this path calls ch1115_fb_update_rect() -> ch1115_draw_screen() -> regmap_raw_write(), which executes a sleepable i2c_transfer(). This will cause kernel warnings and potential deadlocks.
Quoted 3 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +3
-0+ +if (!drm_dev_enter (dev, &idx)) + goto out_drm_gem_fb_end_cpu_access;
[ ... ]
drivers/gpu/drm/tiny/ch1115-i2c.c line 533
Quoted 13 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +13
-0+static void ch1115_encoder_atomic_enable (struct drm_encoder *encoder, +struct drm_atomic_commit *state) +{ +struct drm_device *drm = encoder->dev; +struct ch1115_device *ch1115 =drm_to_ch1115 (drm); + +ch1115_oled_init (ch1115); + +u8 command =CH1115_CMD_ON ; + +ch1115_send_command_list (ch1115, &command,1 ); + +backlight_enable (ch1115->bl_dev);
[Severity: High]
Does this callback also sleep in atomic context?
Encoder atomic_enable callbacks also execute in atomic context. Calling backlight_enable() attempts to acquire a mutex, and ch1115_oled_init() performs sleepable I2C transfers. These operations are unsafe here.
Quoted 1 line
+}
[ ... ]
Quoted 4 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +4
-0+static int ch1115_probe (struct i2c_client *client) +{ +struct ch1115_device *ch1115; +struct device *dev;
drivers/gpu/drm/tiny/ch1115-i2c.c line 763
Quoted 10 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +10
-0+struct backlight_device *bl; +int ret; + + dev = &client->dev; + ch1115 =devm_drm_dev_alloc (&client->dev, &ch1115_driver, + struct ch1115_device, dev); +if (IS_ERR (ch1115)) +return PTR_ERR (ch1115); + +dev_set_name (dev,"ch1115-i2c" );
[Severity: Medium]
Is it safe to alter the kobject name of an actively registered device here?
The driver modifies the kobject name of the already-registered I2C client device (dev = &client->dev) without using device_rename().
Modifying the name while the device is exposed in sysfs frees the existing name string, creating a race condition that can lead to sysfs corruption and use-after-free bugs if concurrently read.
Quoted 2 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +2
-0+ +i2c_set_clientdata (client, ch1115);
[ ... ]
Quoted 11 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +11
-0+ ch1115->hwbuf =devm_kzalloc (dev, +DIV_ROUND_UP (ch1115->width,8 ) * ch1115->height, +GFP_KERNEL ); + +if (!ch1115->hwbuf) +return -ENOMEM ; + + ch1115->row =devm_kzalloc (dev, ch1115->width,GFP_KERNEL ); +if (!ch1115->row) +return -ENOMEM ; +
drivers/gpu/drm/tiny/ch1115-i2c.c line 805
Quoted 10 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +10
-0+// Backlight +const struct backlight_properties bl_props = { + .type =BACKLIGHT_RAW , + .brightness = ch1115->contrast, + .max_brightness =CH1115_MAX_CONTRAST , + }; + + ch1115->bl_dev =devm_backlight_device_register (dev,dev_name (dev), dev, ch1115, + &ch1115_bl_ops, &bl_props); +if (IS_ERR (bl))
[Severity: High]
Is the uninitialized variable bl being evaluated here instead of ch1115->bl_dev?
The result of devm_backlight_device_register() is stored in ch1115->bl_dev, but the error check uses the uninitialized stack variable bl. This will evaluate stack garbage, either causing spurious probe failures by returning garbage, or bypassing the error check entirely if registration fails, guaranteeing a panic when the ERR_PTR is dereferenced later.
Quoted 4 lines
drivers/gpu/drm/tiny/ch1115-i2c.c new file +4
-0+return dev_err_probe (dev,PTR_ERR (bl), +"Unable to register backlight device\n" ); + +// DRM
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH 1/1 fixup: remove uninitialized variable bl
In reply to Nicolás AntinoriRemove the uninitialized variable bl and fix the IS_ERR check.
---
Small follow-up patch that fixes usage of unintialized variable.
Apologies for this oversight!
drivers/gpu/drm/tiny/ch1115-i2c.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/gpu/drm/tiny/ch1115-i2c.c +2 -3
@@ -751,7 +751,6 @@ static int ch1115_probe(struct i2c_client *client) {struct ch1115_device *ch1115;struct device *dev; -struct backlight_device *bl;int ret; dev = &client->dev;
@@ -802,8 +801,8 @@ static int ch1115_probe(struct i2c_client *client) ch1115->bl_dev =devm_backlight_device_register (dev,dev_name (dev), dev, ch1115, &ch1115_bl_ops, &bl_props); -if (IS_ERR (bl)) -return dev_err_probe (dev,PTR_ERR (bl), +if (IS_ERR (ch1115->bl_dev)) +return dev_err_probe (dev,PTR_ERR (ch1115->bl_dev),"Unable to register backlight device\n" );// DRM
--
2.47.3
PATCH v5 3/3 MAINTAINERS: add entry for Chipwealth CH1115 OLED Controller
In reply to Nicolás AntinoriAdd MAINTAINERS entry for the Chipwealth CH1115 dot matrix OLED controller.
--- MAINTAINERS | 6 ++++++ 1 file changed, 6 insertions(+)
MAINTAINERS +6 -0
@@ -8665,6 +8665,12 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git F: Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml F: drivers/gpu/drm/panel/panel-widechips-ws2401.c +DRM DRIVER FOR CHIPWEALTH CH1115 PANELS +M: Nicolás Antinori <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/display/chipwealth,ch1115.yaml +F: drivers/gpu/drm/tiny/ch1115-i2c.c + DRM DRIVERS M: David Airlie <[email protected]> M: Simona Vetter <[email protected]>
--
2.47.3