Re: [PATCH 15/16] backlight: remove pandora_bl
"H. Nikolaus Schaller" <[email protected]>
| Newsgroups | org.kernel.vger.linux-fbdev,dev.linux.lists.mfd,org.freedesktop.lists.dri-devel,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel,org.kernel.vger.linux-omap,org.kernel.vger.linux-sound |
|---|---|
| Message-ID | <[email protected]> |
> Am 10.08.2026 um 15:23 schrieb H. Nikolaus Schaller <[email protected]>: > > > >> Am 10.08.2026 um 14:39 schrieb Andreas Kemnade <[email protected]>: >> >> On Mon, 10 Aug 2026 12:45:16 +0200 >> "H. Nikolaus Schaller" <[email protected]> wrote: >> >>> Hi Daniel, >>> >>>> Am 10.08.2026 um 12:35 schrieb Daniel Thompson <[email protected]>: >>>> >>>> On Sat, Jul 11, 2026 at 08:02:02AM +0200, H. Nikolaus Schaller wrote: >>>>> Retire the platform specific pandora backlight driver since >>>>> it can now be replaced by twl_pm and device tree. >>>>> >>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]> >>>> >>>> Yay! >>>> >>>> Reviewed-by: Daniel Thompson (RISCstar) <[email protected]> >>> >>> Well, there were some good comments by Sahiko to the prerequisites of this patch, >>> because it has one speciality the PWM BL driver does not handle (avoid >>> interpretation of PWM signals as W1 protocol). Here is the report: https://sashiko.dev/#/patchset/[email protected]?part=15 >>> >>> I haven't found time to work on this, so we can't remove this yet. >>> >>> In the end it may either remain a pandora_bl driver (converted to DT) >>> or become a patch for something else. >>> >> But then it is a TPS61161 on top of PWM, not a pandora driver. >> And then there might be just a quirk for that chip in the pwm_bl using >> triggered by a compatible. I guess the quirk is this: >> >> >> 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); >> - } >> Exactly. The Sashiko comment to this section was: 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? Well, I didn't see this but it would be on the safe side to have it. > > Yes that is what I mean. It needs to be ported somewhere. If it can be ported to > some compatible = "pwm_bl_for_tps61161" or what fits best it will be fine. Or it > could be in some compatible = "pandora_bl" driver. > > Has to be worked out when the development machines are not blocked by 3 or 4 other > projects already running in parallel :) Nevertheless I did a quick look into the pandora_bl driver structure. Here are my findings: - the code fragment is part of pandora_backlight_update_status() - priv->old_state remembers if brighness was set to 0 or not - so this code triggers on any change from bright = 0 to bright > 0 - the pandora_bl driver is the only one to use TWL_MODULE_PWM besides twl-pwm So the most natural way of transplanting this appears to me: - add this logic to the twl-pwm driver - add a propery to escribe this special need (e.g. "disable-easyscale") - use this to triger the extra code in case this flag is set - and if the pwm duty cycle changes from permanently off to real PWM - this should obviously go into twl_pwm_config() to detect through duty_ns - alternativey it might be possible to handle everything in twl4030_pwm_enable Suggestions welcome. BR, Nikolaus