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

[Christophe JAILLET]

Le 29/08/2026 à 08:32, Nicolás Antinori a écrit :
> Chipwealth CH1115 is a controller for monochrome dot matrix OLED panels.
>
> Add DRM support for this chip using i2c bus.
>
> Signed-off-by: Nicolás Antinori <nico.antinori.7@xxxxxxxxx>

Hi,

a few nitpick below, should it help.

...

> +static int ch1115_connector_init(struct ch1115_device *ch1115)
> +{
> +	struct drm_connector *connector = &ch1115->connector;
> +	struct drm_encoder *encoder = &ch1115->encoder;
> +	struct drm_device *dev = &ch1115->dev;
> +	int ret;
> +
> +	ret = drm_connector_init(dev, connector, &ch1115_connector_funcs,
> +				 DRM_MODE_CONNECTOR_Unknown);
> +

Unneeded empty ilne.

> +	if (ret)
> +		return ret;
> +
> +	drm_connector_helper_add(connector, &ch1115_connector_helper_funcs);
> +
> +	return drm_connector_attach_encoder(connector, encoder);
> +}

...

> +static int ch1115_probe(struct i2c_client *client)
> +{
> +	struct ch1115_device *ch1115;
> +	struct device *dev;
> +	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);
> +
> +	i2c_set_clientdata(client, ch1115);
> +
> +	ch1115->client = client;
> +	ch1115->regmap = devm_regmap_init(dev, &ch1115_regmap_bus,
> +					  client, &ch1115_regmap_config);
> +	if (IS_ERR(ch1115->regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(ch1115->regmap),
> +				     "Failed to initialize regmap\n");
> +	}

Un-needed { } to be consistent with other paths below.

> +
> +	ret = ch1115_parse_dt(ch1115);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to parse DT\n");
> +
> +	ch1115->pages = DIV_ROUND_UP(ch1115->height, CH1115_PAGE_HEIGHT);
> +	ch1115->bytes_per_row = DIV_ROUND_UP(ch1115->width, 8);
> +	ch1115->x_start_offset = ch1115->flip_horizontal
> +		? CH1115_MAX_WIDTH - ch1115->width
> +		: 0;
> +	ch1115->mode = ch1115_mode(ch1115);
> +
> +	ch1115->hwbuf = devm_kzalloc(dev, (ch1115->width * ch1115->height) / 8,
> +				     GFP_KERNEL);
> +	if (!ch1115->hwbuf)
> +		return -ENOMEM;
> +
> +	ch1115->row = devm_kzalloc(dev, ch1115->width, GFP_KERNEL);
> +	if (!ch1115->row)
> +		return -ENOMEM;
> +
> +	// Backlight
> +	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ch1115,
> +					    &ch1115_bl_ops, NULL);
> +	if (IS_ERR(bl))
> +		return dev_err_probe(dev, PTR_ERR(bl),
> +				     "Unable to register backlight device\n");
> +
> +	bl->props.brightness = ch1115->contrast;
> +	bl->props.max_brightness = CH1115_MAX_CONTRAST;
> +	ch1115->bl_dev = bl;
> +
> +	// DRM
> +	ret = ch1115_mode_config_init(ch1115);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to initialize mode config\n");
> +
> +	ret = ch1115_plane_init(ch1115);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to initialize primary plane\n");
> +
> +	ret = ch1115_crtc_init(ch1115);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to initialize CRTC\n");
> +
> +	ret = ch1115_encoder_init(ch1115);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to initialize encoder\n");
> +
> +	ret = ch1115_connector_init(ch1115);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to initialize connector\n");
> +
> +	drm_mode_config_reset(&ch1115->dev);
> +
> +	ret = drm_dev_register(&ch1115->dev, 0);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to register DRM device\n");
> +
> +	drm_client_setup(&ch1115->dev, NULL);
> +
> +	return 0;
> +}
> +
> +static void ch1115_remove(struct i2c_client *client)
> +{
> +	struct ch1115_device *ch1115 = i2c_get_clientdata(client);
> +	int ret;
> +
> +	u8 cmd_list[] = {
> +		CH1115_CMD_OFF,
> +
> +		CH1115_CMD_ALL_ON,
> +
> +		CH1115_CMD_DC_DC,
> +		CH1115_VAL_DC_DC_OFF,
> +	};
> +
> +	drm_dev_unplug(&ch1115->dev);
> +	drm_atomic_helper_shutdown(&ch1115->dev);
> +
> +	ret = ch1115_send_command_list(ch1115, cmd_list, ARRAY_SIZE(cmd_list));
> +	if (ret < 0)
> +		dev_err(&client->dev, "There was an error executing the shutdown commands");

Missing trailing \n to be consistent with other messages.

> +}
...

CJ