Re: [PATCH v6 2/3] drm/ch1115: add support for Chipwealth CH1115 OLED controller

["Nicolás Antinori" <nico.antinori.7@gmail.com>]

Hello,

On Tue Nov 3, 2026 at 6:05 PM -03, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] drm/ch1115: out-of-range display offset 64 sent in ch1115_oled_init()
> - [Medium] drm/ch1115: missing DC-DC disable in ch1115_encoder_atomic_disable()
> --
>
> commit e5268b4f2a7c534421925bdee0145caf63c50632
> Author: Nicolás Antinori <nico.antinori.7@gmail.com>
>
> drm/ch1115: add support for Chipwealth CH1115 OLED controller
>
> This commit adds a DRM driver for Chipwealth CH1115 monochrome dot matrix
> OLED panels connected over I2C.
>
>> diff --git a/drivers/gpu/drm/tiny/ch1115-i2c.c b/drivers/gpu/drm/tiny/ch1115-i2c.c
>> new file mode 100644
>> index 0000000000000..08fee5a09f4f9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/ch1115-i2c.c
>> @@ -0,0 +1,859 @@
>
> [ ... ]
>
>> +static int ch1115_oled_init(struct ch1115_device *ch1115)
>> +{
>> +	u8 commands[] = {
>> +		CH1115_CMD_OFF,
>> +
>> +		CH1115_CMD_CLOCK_DIVIDE_RATIO,
>> +		CH1115_VAL_CLOCK_DIVIDE_RATIO(CH1115_VAL_CLOCK_OSC_FREQ,
>> +					      CH1115_VAL_CLOCK_DIV_RATIO),
>> +
>> +		CH1115_CMD_MUX_RATIO,
>> +		ch1115->height - 1,
>> +
>> +		CH1115_CMD_OFFSET,
>> +		ch1115->flip_vertical
>> +			? ch1115->y_start_offset
>> +			: CH1115_MAX_HEIGHT - ch1115->y_start_offset,
>
> [Severity: Medium]
> Does ch1115_oled_init() send an out-of-range value for CH1115_CMD_OFFSET when
> flip_vertical is false and y_start_offset is 0?
>
> In ch1115_parse_dt(), y_start_offset defaults to 0 when the chipwealth,offset
> property is omitted, and is otherwise restricted to [0, CH1115_MAX_HEIGHT - 1]
> (0 to 63):
>
> 	ret = of_property_read_u32(np, "chipwealth,offset", &val);
> 	if (ret) {
> 		ch1115->y_start_offset = 0;
> 	} else {
> 		if (val > CH1115_MAX_HEIGHT - 1) {
> 			dev_err(dev, "chipwealth,offset=%d must be [0-%d]\n",
> 				val, CH1115_MAX_HEIGHT - 1);
> 			return -EINVAL;
> 		}
> 		ch1115->y_start_offset = (u8)val;
> 	}
>
> When ch1115_encoder_atomic_enable() calls ch1115_oled_init() with
> flip_vertical set to false and y_start_offset set to 0, the expression
> CH1115_MAX_HEIGHT - ch1115->y_start_offset evaluates to 64 (0x40), which
> overflows the 6-bit [0, 63] vertical offset field instead of wrapping modulo
> CH1115_MAX_HEIGHT to 0.

This is a false positive, the datasheet [1] states that the formula for
this command (D3) is 64 - n, where n is a 6 bit number in the range [0,
63]. I did not use a mask for this because the unused bits get ignored.

>
> [ ... ]
>
>> +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);
>> +	int ret;
>> +
>> +	ret = ch1115_oled_init(ch1115);
>> +	if (ret < 0)
>> +		return;
>> +
>> +	u8 command = CH1115_CMD_ON;
>> +
>> +	ret = ch1115_send_command_list(ch1115, &command, 1);
>> +	if (ret < 0)
>> +		return;
>> +
>> +	backlight_enable(ch1115->bl_dev);
>> +}
>> +
>> +static void ch1115_encoder_atomic_disable(struct drm_encoder *encoder,
>> +					  struct drm_atomic_commit *state)
>> +{
>> +	struct drm_device *drm = encoder->dev;
>> +	struct ch1115_device *ch1115 = drm_to_ch1115(drm);
>> +
>> +	backlight_disable(ch1115->bl_dev);
>> +
>> +	u8 command = CH1115_CMD_OFF;
>> +
>> +	ch1115_send_command_list(ch1115, &command, 1);
>> +}
>
> [Severity: Medium]
> Should ch1115_encoder_atomic_disable() also disable the internal DC-DC
> converter?
>
> When the encoder is enabled, ch1115_encoder_atomic_enable() calls
> ch1115_oled_init(), which turns on the DC-DC converter with CH1115_CMD_DC_DC
> and CH1115_VAL_DC_DC_ON. However, when the display is disabled via KMS or
> during system shutdown via ch1115_shutdown() -> drm_atomic_helper_shutdown(),
> ch1115_encoder_atomic_disable() only sends CH1115_CMD_OFF, leaving the DC-DC
> converter enabled.
>
> By contrast, ch1115_remove() explicitly sends CH1115_CMD_OFF,
> CH1115_CMD_DC_DC, and CH1115_VAL_DC_DC_OFF after calling
> drm_atomic_helper_shutdown():
>
> 	u8 cmd_list[] = {
> 		CH1115_CMD_OFF,
> 		CH1115_CMD_DC_DC,
> 		CH1115_VAL_DC_DC_OFF,
> 	};
>
> 	drm_dev_unplug(&ch1115->dev);
> 	drm_atomic_helper_shutdown(&ch1115->dev);
>
> 	ch1115_send_command_list(ch1115, cmd_list, ARRAY_SIZE(cmd_list));
>
> If ch1115_encoder_atomic_disable() sent CH1115_CMD_OFF, CH1115_CMD_DC_DC, and
> CH1115_VAL_DC_DC_OFF, wouldn't that properly power down the DC-DC converter on
> both KMS disable and ch1115_shutdown(), while avoiding the duplicate command
> sequence in ch1115_remove()?

I believe this is accurate, apologies for this oversight. I have a local
version of this fixed, tested with modetest, unloading the module,
rebooting and power off the system entirely. I'll send a v7 in a few
days in case there is a review.

Best regards,
Nicolás

[1] https://datasheet4u.com/download/1576606/CH1115.html