[PATCH v4] video: simple-panel: add regulator support

[PATCH v4] video: simple-panel: add regulator support

[Michael Grzeschik]

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
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);
+
 	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);
+
 	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);
+	}
+
 	ret = vpl_register(&panel->vpl);
 	if (ret)
 		return ret;
-- 
2.39.5

Re: [PATCH v4] video: simple-panel: add regulator support

[Ahmad Fatoum]

Hi,

On 28.01.25 17:06, Michael Grzeschik wrote:

It's good style to write some message here. ;)

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> 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.

> +
>  	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)



> +	}
> +
>  	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 |

Re: [PATCH v4] video: simple-panel: add regulator support

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3472 bytes --]

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@pengutronix.de>
>>
>> ---
>> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]