[PATCH 1/4] video: fbdev: pxafb: clear allocated memory for video modes

[PATCH 1/4] video: fbdev: pxafb: clear allocated memory for video modes

[Daniel Mack]

When parsing the video modes from DT properties, make sure to zero out
memory before using it. This is important because not all fields in the mode
struct are explicitly initialized, even though they are used later on.

Fixes: 420a488278e86 (video: fbdev: pxafb: initial devicetree conversion)
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/video/fbdev/pxafb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 76722a59f55e..dfe382e68287 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2128,8 +2128,8 @@ static int of_get_pxafb_display(struct device *dev, struct device_node *disp,
 		return -EINVAL;
 
 	ret = -ENOMEM;
-	info->modes = kmalloc_array(timings->num_timings,
-				    sizeof(info->modes[0]), GFP_KERNEL);
+	info->modes = kcalloc(timings->num_timings, sizeof(info->modes[0]),
+			      GFP_KERNEL);
 	if (!info->modes)
 		goto out;
 	info->num_modes = timings->num_timings;
-- 
2.17.1

[PATCH 2/4] video: fbdev: pxafb: switch to devm_* API

[Daniel Mack]

This helps us clean up the probe() and remove() implementations.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/video/fbdev/pxafb.c | 64 ++++++++++---------------------------
 1 file changed, 17 insertions(+), 47 deletions(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index dfe382e68287..6f3a93b3097c 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -1799,19 +1799,17 @@ static struct pxafb_info *pxafb_init_fbinfo(struct device *dev,
 	void *addr;
 
 	/* Alloc the pxafb_info and pseudo_palette in one step */
-	fbi = kmalloc(sizeof(struct pxafb_info) + sizeof(u32) * 16, GFP_KERNEL);
+	fbi = devm_kzalloc(dev, sizeof(struct pxafb_info) + sizeof(u32) * 16,
+			   GFP_KERNEL);
 	if (!fbi)
 		return NULL;
 
-	memset(fbi, 0, sizeof(struct pxafb_info));
 	fbi->dev = dev;
 	fbi->inf = inf;
 
-	fbi->clk = clk_get(dev, NULL);
-	if (IS_ERR(fbi->clk)) {
-		kfree(fbi);
+	fbi->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(fbi->clk))
 		return NULL;
-	}
 
 	strcpy(fbi->fb.fix.id, PXA_NAME);
 
@@ -2128,8 +2126,9 @@ static int of_get_pxafb_display(struct device *dev, struct device_node *disp,
 		return -EINVAL;
 
 	ret = -ENOMEM;
-	info->modes = kcalloc(timings->num_timings, sizeof(info->modes[0]),
-			      GFP_KERNEL);
+	info->modes = devm_kcalloc(dev, timings->num_timings,
+				   sizeof(info->modes[0]),
+				   GFP_KERNEL);
 	if (!info->modes)
 		goto out;
 	info->num_modes = timings->num_timings;
@@ -2305,21 +2304,14 @@ static int pxafb_probe(struct platform_device *dev)
 	if (r = NULL) {
 		dev_err(&dev->dev, "no I/O memory resource defined\n");
 		ret = -ENODEV;
-		goto failed_fbi;
-	}
-
-	r = request_mem_region(r->start, resource_size(r), dev->name);
-	if (r = NULL) {
-		dev_err(&dev->dev, "failed to request I/O memory\n");
-		ret = -EBUSY;
-		goto failed_fbi;
+		goto failed;
 	}
 
-	fbi->mmio_base = ioremap(r->start, resource_size(r));
-	if (fbi->mmio_base = NULL) {
-		dev_err(&dev->dev, "failed to map I/O memory\n");
+	fbi->mmio_base = devm_ioremap_resource(&dev->dev, r);
+	if (IS_ERR(fbi->mmio_base)) {
+		dev_err(&dev->dev, "failed to get I/O memory\n");
 		ret = -EBUSY;
-		goto failed_free_res;
+		goto failed;
 	}
 
 	fbi->dma_buff_size = PAGE_ALIGN(sizeof(struct pxafb_dma_buff));
@@ -2328,7 +2320,7 @@ static int pxafb_probe(struct platform_device *dev)
 	if (fbi->dma_buff = NULL) {
 		dev_err(&dev->dev, "failed to allocate memory for DMA\n");
 		ret = -ENOMEM;
-		goto failed_free_io;
+		goto failed;
 	}
 
 	ret = pxafb_init_video_memory(fbi);
@@ -2345,7 +2337,7 @@ static int pxafb_probe(struct platform_device *dev)
 		goto failed_free_mem;
 	}
 
-	ret = request_irq(irq, pxafb_handle_irq, 0, "LCD", fbi);
+	ret = devm_request_irq(&dev->dev, irq, pxafb_handle_irq, 0, "LCD", fbi);
 	if (ret) {
 		dev_err(&dev->dev, "request_irq failed: %d\n", ret);
 		ret = -EBUSY;
@@ -2355,7 +2347,7 @@ static int pxafb_probe(struct platform_device *dev)
 	ret = pxafb_smart_init(fbi);
 	if (ret) {
 		dev_err(&dev->dev, "failed to initialize smartpanel\n");
-		goto failed_free_irq;
+		goto failed_free_mem;
 	}
 
 	/*
@@ -2365,13 +2357,13 @@ static int pxafb_probe(struct platform_device *dev)
 	ret = pxafb_check_var(&fbi->fb.var, &fbi->fb);
 	if (ret) {
 		dev_err(&dev->dev, "failed to get suitable mode\n");
-		goto failed_free_irq;
+		goto failed_free_mem;
 	}
 
 	ret = pxafb_set_par(&fbi->fb);
 	if (ret) {
 		dev_err(&dev->dev, "Failed to set parameters\n");
-		goto failed_free_irq;
+		goto failed_free_mem;
 	}
 
 	platform_set_drvdata(dev, fbi);
@@ -2404,20 +2396,11 @@ static int pxafb_probe(struct platform_device *dev)
 failed_free_cmap:
 	if (fbi->fb.cmap.len)
 		fb_dealloc_cmap(&fbi->fb.cmap);
-failed_free_irq:
-	free_irq(irq, fbi);
 failed_free_mem:
 	free_pages_exact(fbi->video_mem, fbi->video_mem_size);
 failed_free_dma:
 	dma_free_coherent(&dev->dev, fbi->dma_buff_size,
 			fbi->dma_buff, fbi->dma_buff_phys);
-failed_free_io:
-	iounmap(fbi->mmio_base);
-failed_free_res:
-	release_mem_region(r->start, resource_size(r));
-failed_fbi:
-	clk_put(fbi->clk);
-	kfree(fbi);
 failed:
 	return ret;
 }
@@ -2425,8 +2408,6 @@ static int pxafb_probe(struct platform_device *dev)
 static int pxafb_remove(struct platform_device *dev)
 {
 	struct pxafb_info *fbi = platform_get_drvdata(dev);
-	struct resource *r;
-	int irq;
 	struct fb_info *info;
 
 	if (!fbi)
@@ -2442,22 +2423,11 @@ static int pxafb_remove(struct platform_device *dev)
 	if (fbi->fb.cmap.len)
 		fb_dealloc_cmap(&fbi->fb.cmap);
 
-	irq = platform_get_irq(dev, 0);
-	free_irq(irq, fbi);
-
 	free_pages_exact(fbi->video_mem, fbi->video_mem_size);
 
 	dma_free_wc(&dev->dev, fbi->dma_buff_size, fbi->dma_buff,
 		    fbi->dma_buff_phys);
 
-	iounmap(fbi->mmio_base);
-
-	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	release_mem_region(r->start, resource_size(r));
-
-	clk_put(fbi->clk);
-	kfree(fbi);
-
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH 2/4] video: fbdev: pxafb: switch to devm_* API

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> This helps us clean up the probe() and remove() implementations.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [PATCH 2/4] video: fbdev: pxafb: switch to devm_* API

[Bartlomiej Zolnierkiewicz]

On Sunday, June 24, 2018 05:38:15 PM Daniel Mack wrote:
> This helps us clean up the probe() and remove() implementations.
> 
> 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

[PATCH 3/4] video: fbdev: pxafb: handle errors from pxafb_init_fbinfo() correctly

[Daniel Mack]

pxafb_init_fbinfo() can not only report errors caused by failed
allocations but also when the clock can't be found.

To fix this, return an error pointer instead of NULL in case of errors,
and up-chain the result.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/video/fbdev/pxafb.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 6f3a93b3097c..68459b07d442 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -1802,14 +1802,14 @@ static struct pxafb_info *pxafb_init_fbinfo(struct device *dev,
 	fbi = devm_kzalloc(dev, sizeof(struct pxafb_info) + sizeof(u32) * 16,
 			   GFP_KERNEL);
 	if (!fbi)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	fbi->dev = dev;
 	fbi->inf = inf;
 
 	fbi->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(fbi->clk))
-		return NULL;
+		return ERR_CAST(fbi->clk);
 
 	strcpy(fbi->fb.fix.id, PXA_NAME);
 
@@ -2287,10 +2287,9 @@ static int pxafb_probe(struct platform_device *dev)
 	}
 
 	fbi = pxafb_init_fbinfo(&dev->dev, inf);
-	if (!fbi) {
-		/* only reason for pxafb_init_fbinfo to fail is kmalloc */
+	if (IS_ERR(fbi)) {
 		dev_err(&dev->dev, "Failed to initialize framebuffer device\n");
-		ret = -ENOMEM;
+		ret = PTR_ERR(fbi);
 		goto failed;
 	}
 
-- 
2.17.1

Re: [PATCH 3/4] video: fbdev: pxafb: handle errors from pxafb_init_fbinfo() correctly

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> pxafb_init_fbinfo() can not only report errors caused by failed
> allocations but also when the clock can't be found.
>
> To fix this, return an error pointer instead of NULL in case of errors,
> and up-chain the result.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [PATCH 3/4] video: fbdev: pxafb: handle errors from pxafb_init_fbinfo() correctly

[Bartlomiej Zolnierkiewicz]

On Sunday, June 24, 2018 05:38:16 PM Daniel Mack wrote:
> pxafb_init_fbinfo() can not only report errors caused by failed
> allocations but also when the clock can't be found.
> 
> To fix this, return an error pointer instead of NULL in case of errors,
> and up-chain the result.
> 
> 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

[PATCH 4/4] video: fbdev: pxafb: Add support for lcd-supply regulator

[Daniel Mack]

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

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);
 
-- 
2.17.1

Re: [PATCH 4/4] video: fbdev: pxafb: Add support for lcd-supply regulator

[Robert Jarzmik]

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

Re: [PATCH 4/4] video: fbdev: pxafb: Add support for lcd-supply regulator

[Daniel Mack]

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

Re: [PATCH 4/4] video: fbdev: pxafb: Add support for lcd-supply regulator

[Robert Jarzmik]

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.

-- 
Robert

Re: [PATCH 4/4] video: fbdev: pxafb: Add support for lcd-supply regulator

[Bartlomiej Zolnierkiewicz]

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

Re: [PATCH 1/4] video: fbdev: pxafb: clear allocated memory for video modes

[Daniel Mack]

Hi Bartlomiej,

Should I resend with Robert's Reviewed-bys again? I'd like to get this 
merged for 4.19 if possible.


Thanks,
Daniel


On Sunday, June 24, 2018 05:38 PM, Daniel Mack wrote:
> When parsing the video modes from DT properties, make sure to zero out
> memory before using it. This is important because not all fields in the mode
> struct are explicitly initialized, even though they are used later on.
> 
> Fixes: 420a488278e86 (video: fbdev: pxafb: initial devicetree conversion)
> Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>   drivers/video/fbdev/pxafb.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
> index 76722a59f55e..dfe382e68287 100644
> --- a/drivers/video/fbdev/pxafb.c
> +++ b/drivers/video/fbdev/pxafb.c
> @@ -2128,8 +2128,8 @@ static int of_get_pxafb_display(struct device *dev, struct device_node *disp,
>   		return -EINVAL;
>   
>   	ret = -ENOMEM;
> -	info->modes = kmalloc_array(timings->num_timings,
> -				    sizeof(info->modes[0]), GFP_KERNEL);
> +	info->modes = kcalloc(timings->num_timings, sizeof(info->modes[0]),
> +			      GFP_KERNEL);
>   	if (!info->modes)
>   		goto out;
>   	info->num_modes = timings->num_timings;
> 

Re: [PATCH 1/4] video: fbdev: pxafb: clear allocated memory for video modes

[Bartlomiej Zolnierkiewicz]

On Monday, July 09, 2018 07:12:50 AM Daniel Mack wrote:
> Hi Bartlomiej,

Hi,

> Should I resend with Robert's Reviewed-bys again? I'd like to get this 
> merged for 4.19 if possible.

No need for resend, I added Robert's tags while applying your patches.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 1/4] video: fbdev: pxafb: clear allocated memory for video modes

[Daniel Mack]

On Tuesday, July 24, 2018 05:03 PM, Bartlomiej Zolnierkiewicz wrote:
> On Monday, July 09, 2018 07:12:50 AM Daniel Mack wrote:

>> Should I resend with Robert's Reviewed-bys again? I'd like to get this
>> merged for 4.19 if possible.
> 
> No need for resend, I added Robert's tags while applying your patches.

Great, thank you!

Daniel

Re: [PATCH 1/4] video: fbdev: pxafb: clear allocated memory for video modes

[Bartlomiej Zolnierkiewicz]

On Sunday, June 24, 2018 05:38:14 PM Daniel Mack wrote:
> When parsing the video modes from DT properties, make sure to zero out
> memory before using it. This is important because not all fields in the mode
> struct are explicitly initialized, even though they are used later on.
> 
> Fixes: 420a488278e86 (video: fbdev: pxafb: initial devicetree conversion)
> Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 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