Re: [PATCH 15/16] backlight: remove pandora_bl

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removal of the TPS61161 backlight controller PWM mode initialization sequence, potentially breaking backlight control.
- [Low] The 'pandora-backlight' platform device is still registered in legacy pdata quirks despite the driver being removed, resulting in an unbound dead device.
- [Low] Stale Kconfig reference to `CONFIG_BACKLIGHT_PANDORA` remains in `omap2plus_defconfig`.
--

--- Patch [15]: [PATCH 15/16] backlight: remove pandora_bl ---
commit 6eef80f63fc65c8cd1ed05d5c2e07cc3c726434e
Author: H. Nikolaus Schaller <[email protected]>

backlight: remove pandora_bl

Retire the platform specific pandora backlight driver since
it can now be replaced by twl_pm and device tree.

[Severity: Low]
Does this commit leave an unbound dead device registered in the system?

The pandora-backlight platform driver is removed here, but it appears
arch/arm/mach-omap2/pdata-quirks.c still statically defines and registers
the device in omap3_pandora_legacy_init():

    platform_device_register(&pandora_backlight);

Should this platform device registration be removed as well?

> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 7aa1c4b21111f..a250b3edc152c 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -451,13 +451,6 @@ config BACKLIGHT_MP3309C
[ ... ]
> -config BACKLIGHT_PANDORA
> -	tristate "Backlight driver for Pandora console"
> -	depends on TWL4030_CORE
> -	help
> -	  If you have a Pandora console, say Y to enable the
> -	  backlight driver.
> -

[Severity: Low]
Will this removal trigger a Kconfig warning during omap2plus_defconfig
generation?

The defconfig file still explicitly enables CONFIG_BACKLIGHT_PANDORA=m.
Should arch/arm/configs/omap2plus_defconfig be updated to remove this stale
entry?

> diff --git a/drivers/video/backlight/pandora_bl.c b/drivers/video/backlight/pandora_bl.c
> deleted file mode 100644
> index 8a63ded0fa90f..0000000000000
> --- a/drivers/video/backlight/pandora_bl.c
> +++ /dev/null
> @@ -1,159 +0,0 @@
[ ... ]
> -	if (priv->old_state == PANDORABL_WAS_OFF) {
> -		/*
> -		 * set PWM duty cycle to max. TPS61161 seems to use this
> -		 * to calibrate it's PWM sensitivity when it starts.
> -		 */
> -		twl_i2c_write_u8(TWL_MODULE_PWM, MAX_VALUE, TWL_PWM0_OFF);
> -
> -		/* first enable clock, then PWM0 out */
> -		twl_i2c_read_u8(TWL4030_MODULE_INTBR, &r, TWL_INTBR_GPBR1);
> -		r &= ~PWM0_ENABLE;
> -		r |= PWM0_CLK_ENABLE;
> -		twl_i2c_write_u8(TWL4030_MODULE_INTBR, r, TWL_INTBR_GPBR1);
> -		r |= PWM0_ENABLE;
> -		twl_i2c_write_u8(TWL4030_MODULE_INTBR, r, TWL_INTBR_GPBR1);
> -
> -		/*
> -		 * TI made it very easy to enable digital control, so easy that
> -		 * it often triggers unintentionally and disabes PWM control,
> -		 * so wait until 1 wire mode detection window ends.
> -		 */
> -		usleep_range(2000, 10000);
> -	}

[Severity: High]
When switching to the generic pwm-backlight driver, how is the TPS61161
initialized into PWM mode?

Because the TPS61161 shares its EN pin for both 1-wire EasyScale digital
control and PWM control, the removed pandora_bl driver included this
hardware workaround to force a 100% PWM duty cycle and sleep for 2-10ms at
startup.

Without this initialization pulse, could the TPS61161 misinterpret the initial
PWM signal as a 1-wire EasyScale command and disable PWM control, preventing
the backlight from turning on properly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15