[PATCH v2 0/2] Backlight driver to control backlight behind Congatec Board Controller.

[PATCH v2 0/2] Backlight driver to control backlight behind Congatec Board Controller.

[Petri Karhula via B4 Relay]

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@novatron.fi>
---
Changes in v2:
- Separated Board Controller core driver change into its own patch
- Link to v1: https://lore.kernel.org/r/20251118-cgbc-backlight-v1-1-cc6ac5301034@novatron.fi

---
Petri Karhula (2):
      backlight: Add Congatec Board Controller (CGBC) backlight support
      mfd: cgbc: Add support for backlight

 drivers/mfd/cgbc-core.c           |   1 +
 drivers/video/backlight/Kconfig   |  11 ++
 drivers/video/backlight/Makefile  |   1 +
 drivers/video/backlight/cgbc_bl.c | 281 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 294 insertions(+)
---
base-commit: e7c375b181600caf135cfd03eadbc45eb530f2cb
change-id: 20251118-cgbc-backlight-35c1109db0b8

Best regards,
-- 
Petri Karhula <petri.karhula@novatron.fi>

[PATCH v2 1/2] backlight: Add Congatec Board Controller (CGBC) backlight support

[Petri Karhula via B4 Relay]

From: Petri Karhula <petri.karhula@novatron.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@novatron.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@novatron.fi>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/backlight.h>
+
+#include <linux/mfd/cgbc.h>
+
+#define CGBC_BL_DRIVER_VERSION     "0.0.1"
+
+#define BLT_PWM_DUTY_MASK          0x7F
+#define BLT_PWM_INVERTED_MASK      0x80
+
+/* 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;
+	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;
+	}
+
+	/*
+	 * Only return PWM duty factor percentage,
+	 * ignore polarity inversion bit (bit 7)
+	 */
+	bl_data->current_brightness = reply_buf[0] & BLT_PWM_DUTY_MASK;
+
+	dev_dbg(bl_data->dev, "Current PWM duty=%u\n", bl_data->current_brightness);
+
+	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 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;
+	}
+
+	/*
+	 * Prepare command buffer for writing new settings. Only 2nd byte is changed
+	 * to set new brightness (PWM duty cycle %). Other balues (polarity, frequency)
+	 * are preserved from the read values.
+	 */
+	cmd_buf[1] = (reply_buf[0] & BLT_PWM_INVERTED_MASK) |
+		     (BLT_PWM_DUTY_MASK & brightness);
+	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);
+		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;
+	}
+
+	dev_dbg(bl_data->dev, "Set brightness to %u\n", brightness);
+
+	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;
+
+	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;
+		}
+	}
+
+	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;
+	}
+
+	return bl_data->current_brightness;
+}
+
+/* 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;
+
+	bl_data = devm_kzalloc(&pdev->dev, sizeof(*bl_data), GFP_KERNEL);
+
+	if (!bl_data)
+		return -ENOMEM;
+
+	bl_data->dev = &pdev->dev;
+	bl_data->cgbc = cgbc;
+
+	ret = cgbc_bl_read_pwm_settings(bl_data);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read initial PWM settings: %d\n",
+			ret);
+		return ret;
+	}
+
+	memset(&props, 0, sizeof(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);
+	}
+
+	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);
+
+	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");
+}
+
+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@novatron.fi>");
+MODULE_DESCRIPTION("Congatec Board Controller (CGBC) Backlight Driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(CGBC_BL_DRIVER_VERSION);
+MODULE_ALIAS("platform:cgbc-backlight");

-- 
2.34.1

Re: [PATCH v2 1/2] backlight: Add Congatec Board Controller (CGBC) backlight support

[Thomas Richard]

Hello Petri,

Thanks for your patch.

On 11/19/25 9:25 AM, Petri Karhula via B4 Relay wrote:
> From: Petri Karhula <petri.karhula@novatron.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@novatron.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@novatron.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@novatron.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

Re: [PATCH v2 1/2] backlight: Add Congatec Board Controller (CGBC) backlight support

[Petri Karhula]

Hello Thomas and Krzysztof, 

Thanks for the comments. I have fixed most of them, but I disagree
about removing the error messages. I think that those would be needed.
Without those the kernel does not seem to print any errors to the logs
if the operation fails. It would be then the responsible of the calling
user space program to print the error to the log. But that also could
be missing due to the bug or bad design. This is targeted to be running
in the embedded device that is possibly used in places without internet
coverage. If the user is in such case calling to the support and
complains that backlight control is not working, the only way to get
further information are the logs that the device will send to the cloud
when it happens to be in the coverage of the network. When the log is
the only source of information, it would be really nice to be able to
read clear error messages if the oration has been failed in the driver.
> 

> +	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.
> 

You are absolutely right. It is extremely unlikely that the brightness
level wouldn't match to what is expected. A bug in the Congatec's
firmware could be one possibility. But then that extremely unlikely
situation will happen, I definitely would like to see it in the log.


Best Regards, 
Petri

Re: [PATCH v2 1/2] backlight: Add Congatec Board Controller (CGBC) backlight support

[Krzysztof Kozlowski]

On 19/11/2025 09:25, Petri Karhula via B4 Relay wrote:
> +}
> +
> +/**
> + * 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");
> +}


I reviewed v1, but all comments are applicable.

Best regards,
Krzysztof

[PATCH v2 2/2] mfd: cgbc: Add support for backlight

[Petri Karhula via B4 Relay]

From: Petri Karhula <petri.karhula@novatron.fi>

The Board Controller has control for display backlight.
Add backlight cell for the cgbc-backlight driver which
adds support for backlight brightness control.

Signed-off-by: Petri Karhula <petri.karhula@novatron.fi>
---
 drivers/mfd/cgbc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
index 4782ff1114a9..10bb4b414c34 100644
--- a/drivers/mfd/cgbc-core.c
+++ b/drivers/mfd/cgbc-core.c
@@ -237,6 +237,7 @@ static struct mfd_cell cgbc_devs[] = {
 	{ .name = "cgbc-i2c", .id = 1 },
 	{ .name = "cgbc-i2c", .id = 2 },
 	{ .name = "cgbc-hwmon"	},
+	{ .name = "cgbc-backlight" },
 };
 
 static int cgbc_map(struct cgbc_device_data *cgbc)

-- 
2.34.1