On Tue, Jan 28, 2025 at 05:49:56PM +0100, Ahmad Fatoum wrote:
Hi, On 28.01.25 17:06, Michael Grzeschik wrote: It's good style to write some message here. ;)
Right, but I skipped it for v5. sorry. :)
Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> --- v3 -> v4: - better bail out if the regulator is set but not available v2 -> v3: - reset regulator->power to NULL in case nothing was found v1 -> v2: - added the missing signed-off-by --- drivers/video/simple-panel.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/video/simple-panel.c b/drivers/video/simple-panel.c index 7048e2f51b..905fd94fb7 100644 --- a/drivers/video/simple-panel.c +++ b/drivers/video/simple-panel.c @@ -12,6 +12,7 @@ #include <fb.h> #include <gpio.h> #include <of_gpio.h> +#include <regulator.h> #include <video/backlight.h> #include <video/vpl.h> #include <i2c/i2c.h> @@ -25,6 +26,7 @@ struct simple_panel { struct device_node *backlight_node; struct backlight_device *backlight; struct device_node *ddc_node; + struct regulator *power; int enable_delay; }; @@ -49,6 +51,8 @@ static int simple_panel_enable(struct simple_panel *panel) if (panel->enable_delay) mdelay(panel->enable_delay); + regulator_enable(panel->power);Conceptually, you power on a device before you toggle its pins, so this needs to be moved before the enable_gpio toggling.+ if (panel->backlight) { ret = backlight_set_brightness_default(panel->backlight); if (ret) @@ -65,6 +69,8 @@ static int simple_panel_disable(struct simple_panel *panel) if (panel->backlight) backlight_set_brightness(panel->backlight, 0); + regulator_disable(panel->power);Similarly, this should be after the GPIO was dis-asserted.
This is true for both cases. I fixed that.
+ if (gpio_is_valid(panel->enable_gpio)) gpio_direction_output(panel->enable_gpio, !panel->enable_active_high); @@ -154,6 +160,12 @@ static int simple_panel_probe(struct device *dev) panel->backlight_node = of_parse_phandle(node, "backlight", 0); + panel->power = regulator_get(dev, "power"); + if (IS_ERR(panel->power)) { + dev_warn(dev, "Cannot find regulator\n"); + return PTR_ERR(panel->power);return dev_errp_probe(dev, panel->power, "Cannot find regulator\n"); So: - error code is logged - EPROBE_DEFER doesn't result in a diagnostic yet - the correct log level is used (warning is for issues that the driver can recover from)
Great hint. Did that in v5.
+ } + ret = vpl_register(&panel->vpl); if (ret) return ret;-- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
-- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
