| Message ID | 20180624153817.24387-4-daniel@zonque.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel Mack <daniel@zonque.org> writes: > + if (fbi->lcd_supply && fbi->lcd_supply_enabled != on) { Mmh this looks weird ... If lcd_supply_enabled == on, then the next block is never evaluated, and the value of "on" is not considered in order to call regulator_disable() ... > + int ret; > + > + if (on) > + ret = regulator_enable(fbi->lcd_supply); > + else > + ret = regulator_disable(fbi->lcd_supply); This apart, this was a change I was expecting for pxafb, one of the 2 in my backlog, which is great. The second one was linking a backlight ... Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, June 26, 2018 10:27 AM, Robert Jarzmik wrote: > Daniel Mack <daniel@zonque.org> writes: > >> + if (fbi->lcd_supply && fbi->lcd_supply_enabled != on) { > Mmh this looks weird ... > If lcd_supply_enabled == on, then the next block is never evaluated, and the > value of "on" is not considered in order to call regulator_disable() ... Hmm? This early bail just avoids unbalanced calls to the regulator core, which doesn't like that at all. IOW, the rest of this function is only executed if the desired supply state differs from our locally cached version. This also worked well in my tests. Am I missing something? >> + int ret; >> + >> + if (on) >> + ret = regulator_enable(fbi->lcd_supply); >> + else >> + ret = regulator_disable(fbi->lcd_supply); > > This apart, this was a change I was expecting for pxafb, one of the 2 in my > backlog, which is great. The second one was linking a backlight ... That should be done with devm_of_find_backlight() I figure, and not via a regulator. But it's trivial to do as a separate patch, yes. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Daniel Mack <daniel@zonque.org> writes: > On Tuesday, June 26, 2018 10:27 AM, Robert Jarzmik wrote: >> Daniel Mack <daniel@zonque.org> writes: >> >>> + if (fbi->lcd_supply && fbi->lcd_supply_enabled != on) { >> Mmh this looks weird ... >> If lcd_supply_enabled == on, then the next block is never evaluated, and the >> value of "on" is not considered in order to call regulator_disable() ... > > Hmm? This early bail just avoids unbalanced calls to the regulator core, which > doesn't like that at all. IOW, the rest of this function is only executed if the > desired supply state differs from our locally cached version. > > This also worked well in my tests. Am I missing something? Ah yes, you're right. My brain read : "if the cached value is not _on_ (ie. lcd_supply_enabled != 1), then ..." instead of "if the cached value is not the new desired value" ... > That should be done with devm_of_find_backlight() I figure, and not via a > regulator. But it's trivial to do as a separate patch, yes. Yep. Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr> Cheers.
On Sunday, June 24, 2018 05:38:17 PM Daniel Mack wrote: > Optionally obtain a lcd-supply regulator during probe and use it in > __pxafb_lcd_power() to switch the power supply of LCD panels. > > This helps boards booted from DT to control such voltages without > callbacks. > > Signed-off-by: Daniel Mack <daniel@zonque.org> Patch queued for 4.19, thanks. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt b/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt index f79641bd5f18..45ffd6c41748 100644 --- a/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt +++ b/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt @@ -10,6 +10,9 @@ Required properties: - interrupts : framebuffer controller interrupt. - clocks: phandle to input clocks +Optional properties: + - lcd-supply: A phandle to a power regulator that controls the LCD voltage. + Required nodes: - port: connection to the LCD panel (see video-interfaces.txt) This node must have its properties bus-width and remote-endpoint set. diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c index 68459b07d442..bbed039617a4 100644 --- a/drivers/video/fbdev/pxafb.c +++ b/drivers/video/fbdev/pxafb.c @@ -56,6 +56,7 @@ #include <linux/freezer.h> #include <linux/console.h> #include <linux/of_graph.h> +#include <linux/regulator/consumer.h> #include <video/of_display_timing.h> #include <video/videomode.h> @@ -1423,6 +1424,21 @@ static inline void __pxafb_lcd_power(struct pxafb_info *fbi, int on) if (fbi->lcd_power) fbi->lcd_power(on, &fbi->fb.var); + + if (fbi->lcd_supply && fbi->lcd_supply_enabled != on) { + int ret; + + if (on) + ret = regulator_enable(fbi->lcd_supply); + else + ret = regulator_disable(fbi->lcd_supply); + + if (ret < 0) + pr_warn("Unable to %s LCD supply regulator: %d\n", + on ? "enable" : "disable", ret); + else + fbi->lcd_supply_enabled = on; + } } static void pxafb_enable_controller(struct pxafb_info *fbi) @@ -2299,6 +2315,14 @@ static int pxafb_probe(struct platform_device *dev) fbi->backlight_power = inf->pxafb_backlight_power; fbi->lcd_power = inf->pxafb_lcd_power; + fbi->lcd_supply = devm_regulator_get_optional(&dev->dev, "lcd"); + if (IS_ERR(fbi->lcd_supply)) { + if (PTR_ERR(fbi->lcd_supply) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + fbi->lcd_supply = NULL; + } + r = platform_get_resource(dev, IORESOURCE_MEM, 0); if (r == NULL) { dev_err(&dev->dev, "no I/O memory resource defined\n"); diff --git a/drivers/video/fbdev/pxafb.h b/drivers/video/fbdev/pxafb.h index 5dc414e26fc8..b641289c8a99 100644 --- a/drivers/video/fbdev/pxafb.h +++ b/drivers/video/fbdev/pxafb.h @@ -165,6 +165,9 @@ struct pxafb_info { struct notifier_block freq_policy; #endif + struct regulator *lcd_supply; + bool lcd_supply_enabled; + void (*lcd_power)(int, struct fb_var_screeninfo *); void (*backlight_power)(int);
Optionally obtain a lcd-supply regulator during probe and use it in __pxafb_lcd_power() to switch the power supply of LCD panels. This helps boards booted from DT to control such voltages without callbacks. Signed-off-by: Daniel Mack <daniel@zonque.org> --- .../bindings/display/marvell,pxa2xx-lcdc.txt | 3 +++ drivers/video/fbdev/pxafb.c | 24 +++++++++++++++++++ drivers/video/fbdev/pxafb.h | 3 +++ 3 files changed, 30 insertions(+)