On 18/11/2025 17:43, Petri Karhula via B4 Relay wrote:
> +
> +/**
> + * 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
> + */
Why are you documenting standard API?
> +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 */
Huh? Can it be a GPIO 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
Very redundant and useless comment.
> + */
> +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);
> +
Drop blank line. There is never such line between allocation and check.
> + 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 dev_err_probe
> + 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 dev_err_probe
> + 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);
Drop.
This does not look like useful printk message. Drivers should be silent
on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
> +
> + 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.
Drop such comments, they are not useful. Please write only useful
comments, not ones stating obvious.
> + */
> +static void cgbc_bl_remove(struct platform_device *pdev)
> +{
> + dev_info(&pdev->dev, "CGBC backlight driver removed\n");
Drop, there is no such code in Linux kernel. Drop it.
> +}
> +
Best regards,
Krzysztof
