Message-ID: <f40abf01-46f2-41e6-a21f-c58c21d653c4@bootlin.com>
Date: Thu, 20 Nov 2025 10:09:03 +0100
From: Thomas Richard <thomas.richard@...tlin.com>
To: petri.karhula@...atron.fi, Lee Jones <lee@...nel.org>,
 Daniel Thompson <danielt@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
 Helge Deller <deller@....de>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v2 1/2] backlight: Add Congatec Board Controller (CGBC)
 backlight support

Hello Petri,

Thanks for your patch.

On 11/19/25 9:25 AM, Petri Karhula via B4 Relay wrote:
> From: Petri Karhula <petri.karhula@...atron.fi>
> 
> This driver provides backlight brightness control through the Linux
> backlight subsystem. It communicates with the board controller to
> adjust LCD backlight using PWM signals. Communication is done
> through Congatec Board Controller core driver.
> 
> Signed-off-by: Petri Karhula <petri.karhula@...atron.fi>
> ---
>  drivers/video/backlight/Kconfig   |  11 ++
>  drivers/video/backlight/Makefile  |   1 +
>  drivers/video/backlight/cgbc_bl.c | 281 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 293 insertions(+)
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index d9374d208cee..702f3b8ed036 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -249,6 +249,17 @@ config BACKLIGHT_PWM
>  	  If you have a LCD backlight adjustable by PWM, say Y to enable
>  	  this driver.
>  
> +config BACKLIGHT_CGBC
> +	tristate "Congatec Board Controller (CGBC) backlight support"
> +	depends on MFD_CGBC && X86
> +	help
> +	  Say Y here to enable support for LCD backlight control on Congatec
> +	  x86-based boards via the CGBC (Congatec Board Controller).
> +
> +	  This driver provides backlight brightness control through the Linux
> +	  backlight subsystem. It communicates with the board controller to
> +	  adjust LCD backlight using PWM signals.
> +
>  config BACKLIGHT_DA903X
>  	tristate "Backlight Driver for DA9030/DA9034 using WLED"
>  	depends on PMIC_DA903X
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index dfbb169bf6ea..0169fd8873ed 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_BACKLIGHT_APPLE_DWI)	+= apple_dwi_bl.o
>  obj-$(CONFIG_BACKLIGHT_AS3711)		+= as3711_bl.o
>  obj-$(CONFIG_BACKLIGHT_BD6107)		+= bd6107.o
>  obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE)	+= backlight.o
> +obj-$(CONFIG_BACKLIGHT_CGBC)		+= cgbc_bl.o
>  obj-$(CONFIG_BACKLIGHT_DA903X)		+= da903x_bl.o
>  obj-$(CONFIG_BACKLIGHT_DA9052)		+= da9052_bl.o
>  obj-$(CONFIG_BACKLIGHT_EP93XX)		+= ep93xx_bl.o
> diff --git a/drivers/video/backlight/cgbc_bl.c b/drivers/video/backlight/cgbc_bl.c
> new file mode 100644
> index 000000000000..4382321f4cac
> --- /dev/null
> +++ b/drivers/video/backlight/cgbc_bl.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Congatec Board Controller (CGBC) Backlight Driver
> + *
> + * This driver provides backlight control for LCD displays connected to
> + * Congatec boards via the CGBC (Congatec Board Controller). It integrates
> + * with the Linux backlight subsystem and communicates with hardware through
> + * the cgbc-core module.
> + *
> + * Copyright (C) 2025 Novatron Oy
> + *
> + * Author: Petri Karhula <petri.karhula@...atron.fi>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/backlight.h>
> +
> +#include <linux/mfd/cgbc.h>

headers shall be sorted in alphabetical order

> +
> +#define CGBC_BL_DRIVER_VERSION     "0.0.1"

not needed

> +
> +#define BLT_PWM_DUTY_MASK          0x7F
> +#define BLT_PWM_INVERTED_MASK      0x80

Use GENMASK

> +
> +/* CGBC command for PWM brightness control*/
> +#define CGBC_CMD_BLT0_PWM          0x75
> +
> +#define CGBC_BL_MAX_BRIGHTNESS     100
> +
> +/**
> + * CGBC backlight driver data
> + * @dev: Pointer to the platform device
> + * @bl_dev: Pointer to the backlight device
> + * @cgbc: Pointer to the parent CGBC device data
> + * @current_brightness: Current brightness level (0-100)
> + */
> +struct cgbc_bl_data {
> +	struct device *dev;
> +	struct backlight_device *bl_dev;

not used

> +	struct cgbc_device_data *cgbc;
> +	unsigned int current_brightness;
> +};
> +
> +/**
> + * Read current PWM settings from hardware
> + * @bl_data: Backlight driver data
> + *
> + * Reads the current PWM duty cycle percentage (= brightness level)
> + * from the board controller.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cgbc_bl_read_pwm_settings(struct cgbc_bl_data *bl_data)
> +{
> +	u8 cmd_buf[4] = { CGBC_CMD_BLT0_PWM, 0, 0, 0 };
> +	u8 reply_buf[3];
> +	int ret;
> +
> +	ret = cgbc_command(bl_data->cgbc, cmd_buf, sizeof(cmd_buf), reply_buf,
> +			   sizeof(reply_buf), NULL);
> +
> +	if (ret < 0) {
> +		dev_err(bl_data->dev, "Failed to read PWM settings: %d\n", ret);
> +		return ret;
> +	}

error message not needed from my point of view.

> +
> +	/*
> +	 * Only return PWM duty factor percentage,
> +	 * ignore polarity inversion bit (bit 7)
> +	 */
> +	bl_data->current_brightness = reply_buf[0] & BLT_PWM_DUTY_MASK;

I would prefer to use FIELD_GET

> +
> +	dev_dbg(bl_data->dev, "Current PWM duty=%u\n", bl_data->current_brightness);

Not needed from my point of view.

> +
> +	return 0;
> +}
> +
> +/**
> + * Set backlight brightness
> + * @bl_data: Backlight driver data
> + * @brightness: Brightness level (0-100)
> + *
> + * Sets the backlight brightness by configuring the PWM duty cycle.
> + * Preserves the current polarity and frequency settings.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cgbc_bl_set_brightness(struct cgbc_bl_data *bl_data, u8 brightness)
> +{
> +	u8 cmd_buf[4] = { CGBC_CMD_BLT0_PWM, 0, 0, 0 };

u8 cmd_buf[4] = { CGBC_CMD_BLT0_PWM };

> +	u8 reply_buf[3];
> +	int ret;
> +
> +	/* Read the current values */
> +	ret = cgbc_command(bl_data->cgbc, cmd_buf, sizeof(cmd_buf), reply_buf,
> +			   sizeof(reply_buf), NULL);
> +
> +	if (ret < 0) {
> +		dev_err(bl_data->dev, "Failed to read PWM settings: %d\n", ret);
> +		return ret;
> +	}

error message not needed from my point of view.

> +
> +	/*
> +	 * Prepare command buffer for writing new settings. Only 2nd byte is changed
> +	 * to set new brightness (PWM duty cycle %). Other balues (polarity, frequency)

values

> +	 * are preserved from the read values.
> +	 */
> +	cmd_buf[1] = (reply_buf[0] & BLT_PWM_INVERTED_MASK) |
> +		     (BLT_PWM_DUTY_MASK & brightness);

use FIELD_PREP

> +	cmd_buf[2] = reply_buf[1];
> +	cmd_buf[3] = reply_buf[2];
> +
> +	ret = cgbc_command(bl_data->cgbc, cmd_buf, sizeof(cmd_buf), reply_buf,
> +			   sizeof(reply_buf), NULL);
> +
> +	if (ret < 0) {
> +		dev_err(bl_data->dev, "Failed to set brightness: %d\n", ret);

error messages not needed from my point of view.

> +		return ret;
> +	}
> +
> +	bl_data->current_brightness = reply_buf[0] & BLT_PWM_DUTY_MASK;
> +
> +	/* Verify the setting was applied correctly */
> +	if (bl_data->current_brightness != brightness) {
> +		dev_err(bl_data->dev,
> +			"Brightness setting verification failed\n");
> +		return -EIO;
> +	}

Do we really need to check the brightness returned by the board
controller? Have you ever run into a situation where cbgc_command
completed without errors, but the brightness level didn’t match what you
expected? Maybe we could assume that if the cbgc_command returned
successfully the brightness value is correct?

I'm not against checking the backlight value. I looked at Congatec's
implementation and they also check it.

> +
> +	dev_dbg(bl_data->dev, "Set brightness to %u\n", brightness);

Not needed, the core already has this message

> +
> +	return 0;
> +}
> +
> +/**
> + * Backlight update callback
> + * @bl: Backlight device
> + *
> + * Called by the backlight subsystem when brightness needs to be updated.
> + * Changes the brightness level on the hardware
> + * if requested value differs from the current setting.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cgbc_bl_update_status(struct backlight_device *bl)
> +{
> +	struct cgbc_bl_data *bl_data = bl_get_data(bl);
> +	u8 brightness;
> +	int ret;
> +
> +	brightness = bl->props.brightness;

use backlight_get_brightness()

> +
> +	if (brightness != bl_data->current_brightness) {
> +		ret = cgbc_bl_set_brightness(bl_data, brightness);
> +
> +		if (ret < 0) {
> +			dev_err(bl_data->dev, "Failed to set brightness: %d\n",
> +			       ret);
> +			return ret;
> +		}

error message not needed from my point of view.

> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * Get current backlight brightness
> + * @bl: Backlight device
> + *
> + * Returns the current brightness level by reading from hardware.
> + *
> + * Return: Current brightness level (0-100), or negative error code
> + */
> +static int cgbc_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct cgbc_bl_data *bl_data = bl_get_data(bl);
> +	int ret;
> +
> +	/* Read current PWM brightness settings */
> +	ret = cgbc_bl_read_pwm_settings(bl_data);
> +
> +	if (ret < 0) {
> +		dev_err(bl_data->dev, "Failed to read PWM settings: %d\n", ret);
> +		return ret;
> +	}

error message not needed from my point of view.
If you remove all these error messages, you can also remove the struct
device in the struct cgbc_bl_data.

> +
> +	return bl_data->current_brightness;
> +}

Maybe you can remove cgbc_bl_read_pwm_settings() and move all the code
in cgbc_bl_get_brightness(). It makes the code easier to read.

> +
> +/* Backlight device operations */
> +static const struct backlight_ops cgbc_bl_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = cgbc_bl_update_status,
> +	.get_brightness = cgbc_bl_get_brightness,
> +};
> +
> +/**
> + * Probe function for CGBC backlight driver
> + * @pdev: Platform device
> + *
> + * Initializes the CGBC backlight driver and registers it with the
> + * Linux backlight subsystem.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cgbc_bl_probe(struct platform_device *pdev)
> +{
> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
> +	struct cgbc_bl_data *bl_data;
> +	struct backlight_properties props;
> +	struct backlight_device *bl_dev;
> +	int ret;

nitpick: reverse xmas tree

> +
> +	bl_data = devm_kzalloc(&pdev->dev, sizeof(*bl_data), GFP_KERNEL);
> +

nitpick: drop empty line.

> +	if (!bl_data)
> +		return -ENOMEM;
> +
> +	bl_data->dev = &pdev->dev;
> +	bl_data->cgbc = cgbc;
> +
> +	ret = cgbc_bl_read_pwm_settings(bl_data);
> +

nitpick: drop empty line.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to read initial PWM settings: %d\n",
> +			ret);
> +		return ret;
> +	}

Use dev_err_probe().

> +
> +	memset(&props, 0, sizeof(props));

Use struct backlight_properties props = { };

> +	props.type = BACKLIGHT_PLATFORM;
> +	props.max_brightness = CGBC_BL_MAX_BRIGHTNESS;
> +	props.brightness = bl_data->current_brightness;
> +
> +	bl_dev = devm_backlight_device_register(&pdev->dev, "cgbc-backlight",
> +						&pdev->dev, bl_data,
> +						&cgbc_bl_ops, &props);
> +
> +	if (IS_ERR(bl_dev)) {
> +		dev_err(&pdev->dev, "Failed to register backlight device\n");
> +		return PTR_ERR(bl_dev);
> +	}

Use dev_err_probe()

> +
> +	bl_data->bl_dev = bl_dev;
> +	platform_set_drvdata(pdev, bl_data);
> +
> +	dev_info(&pdev->dev,
> +		 "CGBC backlight driver registered (brightness=%u)\n",
> +		 bl_data->current_brightness);

No logs if device probes successfully.

> +
> +	return 0;
> +}
> +
> +/**
> + * Remove function for CGBC backlight driver
> + * @pdev: Platform device
> + *
> + * The Linux device-managed resource framework (devres) does the cleanup.
> + * No explicit cleanup is needed here.
> + */
> +static void cgbc_bl_remove(struct platform_device *pdev)
> +{
> +	dev_info(&pdev->dev, "CGBC backlight driver removed\n");
> +}

Remove operation does nothing so drop it.

> +
> +static struct platform_driver cgbc_bl_driver = {
> +	.driver = {
> +		.name = "cgbc-backlight",
> +	},
> +	.probe = cgbc_bl_probe,
> +	.remove = cgbc_bl_remove,
> +};
> +
> +module_platform_driver(cgbc_bl_driver);
> +
> +MODULE_AUTHOR("Petri Karhula <petri.karhula@...atron.fi>");
> +MODULE_DESCRIPTION("Congatec Board Controller (CGBC) Backlight Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(CGBC_BL_DRIVER_VERSION);

Not needed

> +MODULE_ALIAS("platform:cgbc-backlight");
> 

Best Regards,
Thomas

Powered by blists - more mailing lists