[PATCH v2 1/3] video: xilinxfb: Use standard variable name convention

[PATCH v2 1/3] video: xilinxfb: Use standard variable name convention

[Michal Simek]

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

s/op/pdev/ in xilinxfb_of_probe().
No functional chagnes.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2: None

 drivers/video/xilinxfb.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 0e1dd33..d12345f 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -411,7 +411,7 @@ static int xilinxfb_release(struct device *dev)
  * OF bus binding
  */

-static int xilinxfb_of_probe(struct platform_device *op)
+static int xilinxfb_of_probe(struct platform_device *pdev)
 {
 	const u32 *prop;
 	u32 tft_access = 0;
@@ -425,7 +425,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
 	/* Allocate the driver data region */
 	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
 	if (!drvdata) {
-		dev_err(&op->dev, "Couldn't allocate device private record\n");
+		dev_err(&pdev->dev, "Couldn't allocate device private record\n");
 		return -ENOMEM;
 	}

@@ -433,7 +433,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
 	 * To check whether the core is connected directly to DCR or BUS
 	 * interface and initialize the tft_access accordingly.
 	 */
-	of_property_read_u32(op->dev.of_node, "xlnx,dcr-splb-slave-if",
+	of_property_read_u32(pdev->dev.of_node, "xlnx,dcr-splb-slave-if",
 			     &tft_access);

 	/*
@@ -457,29 +457,29 @@ static int xilinxfb_of_probe(struct platform_device *op)
 	}
 #endif

-	prop = of_get_property(op->dev.of_node, "phys-size", &size);
+	prop = of_get_property(pdev->dev.of_node, "phys-size", &size);
 	if ((prop) && (size >= sizeof(u32)*2)) {
 		pdata.screen_width_mm = prop[0];
 		pdata.screen_height_mm = prop[1];
 	}

-	prop = of_get_property(op->dev.of_node, "resolution", &size);
+	prop = of_get_property(pdev->dev.of_node, "resolution", &size);
 	if ((prop) && (size >= sizeof(u32)*2)) {
 		pdata.xres = prop[0];
 		pdata.yres = prop[1];
 	}

-	prop = of_get_property(op->dev.of_node, "virtual-resolution", &size);
+	prop = of_get_property(pdev->dev.of_node, "virtual-resolution", &size);
 	if ((prop) && (size >= sizeof(u32)*2)) {
 		pdata.xvirt = prop[0];
 		pdata.yvirt = prop[1];
 	}

-	if (of_find_property(op->dev.of_node, "rotate-display", NULL))
+	if (of_find_property(pdev->dev.of_node, "rotate-display", NULL))
 		pdata.rotate_screen = 1;

-	dev_set_drvdata(&op->dev, drvdata);
-	return xilinxfb_assign(op, drvdata, &pdata);
+	dev_set_drvdata(&pdev->dev, drvdata);
+	return xilinxfb_assign(pdev, drvdata, &pdata);
 }

 static int xilinxfb_of_remove(struct platform_device *op)
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 2/3] video: xilinxfb: Use devm_kzalloc instead of kzalloc

[Michal Simek]

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

Simplify driver probe and release function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Jingoo Han <jg1.han@samsung.com>
---
Changes in v2:
Rebased on git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git for-next

 drivers/video/xilinxfb.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index d12345f..98c7a6f 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -368,8 +368,6 @@ err_fbmem:
 		devm_iounmap(dev, drvdata->regs);

 err_region:
-	kfree(drvdata);
-
 	return rc;
 }

@@ -402,8 +400,6 @@ static int xilinxfb_release(struct device *dev)
 		dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
 #endif

-	kfree(drvdata);
-
 	return 0;
 }

@@ -423,7 +419,7 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
 	pdata = xilinx_fb_default_pdata;

 	/* Allocate the driver data region */
-	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
 	if (!drvdata) {
 		dev_err(&pdev->dev, "Couldn't allocate device private record\n");
 		return -ENOMEM;
@@ -451,7 +447,6 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
 		drvdata->dcr_host = dcr_map(op->dev.of_node, start, drvdata->dcr_len);
 		if (!DCR_MAP_OK(drvdata->dcr_host)) {
 			dev_err(&op->dev, "invalid DCR address\n");
-			kfree(drvdata);
 			return -ENODEV;
 		}
 	}
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/3] video: xilinxfb: Use devm_kzalloc instead of kzalloc

[Joe Perches]

On Wed, 2013-10-09 at 12:52 +0200, Michal Simek wrote:
> Simplify driver probe and release function.
[]
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
[]
> @@ -423,7 +419,7 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
>  	pdata = xilinx_fb_default_pdata;
> 
>  	/* Allocate the driver data region */
> -	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata) {
>  		dev_err(&pdev->dev, "Couldn't allocate device private record\n");

Be nice to remove the unnecessary OOM message.
There's already a generic dump_stack on OOM.

Re: [PATCH v2 2/3] video: xilinxfb: Use devm_kzalloc instead of kzalloc

[Michal Simek]

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

On 10/09/2013 05:13 PM, Joe Perches wrote:
> On Wed, 2013-10-09 at 12:52 +0200, Michal Simek wrote:
>> Simplify driver probe and release function.
> []
>> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> []
>> @@ -423,7 +419,7 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
>>  	pdata = xilinx_fb_default_pdata;
>>
>>  	/* Allocate the driver data region */
>> -	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
>> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
>>  	if (!drvdata) {
>>  		dev_err(&pdev->dev, "Couldn't allocate device private record\n");
> 
> Be nice to remove the unnecessary OOM message.
> There's already a generic dump_stack on OOM.

Ah yeah - this series was made before I knew this.
Will send v3.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2 3/3] video: xilinxfb: Simplify error path

[Michal Simek]

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

devm_iounmap is called automatically that's why remove it from the code
dev_set_drvdata(dev, NULL) is called by generic code
after device_release or on probe failure.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Reviewed-by: Jingoo Han <jg1.han@samsung.com>
---
Changes in v2:
Rebased on git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git for-next

 drivers/video/xilinxfb.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 98c7a6f..7e3036c 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -260,10 +260,9 @@ static int xilinxfb_assign(struct platform_device *pdev,

 		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 		drvdata->regs = devm_ioremap_resource(&pdev->dev, res);
-		if (IS_ERR(drvdata->regs)) {
-			rc = PTR_ERR(drvdata->regs);
-			goto err_region;
-		}
+		if (IS_ERR(drvdata->regs))
+			return PTR_ERR(drvdata->regs);
+
 		drvdata->regs_phys = res->start;
 	}

@@ -279,11 +278,7 @@ static int xilinxfb_assign(struct platform_device *pdev,

 	if (!drvdata->fb_virt) {
 		dev_err(dev, "Could not allocate frame buffer memory\n");
-		rc = -ENOMEM;
-		if (drvdata->flags & BUS_ACCESS_FLAG)
-			goto err_fbmem;
-		else
-			goto err_region;
+		return -ENOMEM;
 	}

 	/* Clear (turn to black) the framebuffer */
@@ -363,11 +358,6 @@ err_cmap:
 	/* Turn off the display */
 	xilinx_fb_out32(drvdata, REG_CTRL, 0);

-err_fbmem:
-	if (drvdata->flags & BUS_ACCESS_FLAG)
-		devm_iounmap(dev, drvdata->regs);
-
-err_region:
 	return rc;
 }

@@ -392,11 +382,9 @@ static int xilinxfb_release(struct device *dev)
 	/* Turn off the display */
 	xilinx_fb_out32(drvdata, REG_CTRL, 0);

-	/* Release the resources, as allocated based on interface */
-	if (drvdata->flags & BUS_ACCESS_FLAG)
-		devm_iounmap(dev, drvdata->regs);
 #ifdef CONFIG_PPC_DCR
-	else
+	/* Release the resources, as allocated based on interface */
+	if (!(drvdata->flags & BUS_ACCESS_FLAG))
 		dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
 #endif

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 1/3] video: xilinxfb: Use standard variable name convention

[Mark Rutland]

On Wed, Oct 09, 2013 at 11:52:12AM +0100, Michal Simek wrote:
> s/op/pdev/ in xilinxfb_of_probe().
> No functional chagnes.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> Changes in v2: None
> 
>  drivers/video/xilinxfb.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index 0e1dd33..d12345f 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -411,7 +411,7 @@ static int xilinxfb_release(struct device *dev)
>   * OF bus binding
>   */
> 
> -static int xilinxfb_of_probe(struct platform_device *op)
> +static int xilinxfb_of_probe(struct platform_device *pdev)
>  {
>  	const u32 *prop;
>  	u32 tft_access = 0;
> @@ -425,7 +425,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
>  	/* Allocate the driver data region */
>  	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata) {
> -		dev_err(&op->dev, "Couldn't allocate device private record\n");
> +		dev_err(&pdev->dev, "Couldn't allocate device private record\n");
>  		return -ENOMEM;
>  	}
> 
> @@ -433,7 +433,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
>  	 * To check whether the core is connected directly to DCR or BUS
>  	 * interface and initialize the tft_access accordingly.
>  	 */
> -	of_property_read_u32(op->dev.of_node, "xlnx,dcr-splb-slave-if",
> +	of_property_read_u32(pdev->dev.of_node, "xlnx,dcr-splb-slave-if",
>  			     &tft_access);
> 
>  	/*
> @@ -457,29 +457,29 @@ static int xilinxfb_of_probe(struct platform_device *op)
>  	}
>  #endif
> 
> -	prop = of_get_property(op->dev.of_node, "phys-size", &size);
> +	prop = of_get_property(pdev->dev.of_node, "phys-size", &size);
>  	if ((prop) && (size >= sizeof(u32)*2)) {
>  		pdata.screen_width_mm = prop[0];
>  		pdata.screen_height_mm = prop[1];
>  	}

While you're changing these lines, it would be nice to change this
pattern (here and elsewhere) to use of_property_read_u32_array, so that
it's endian-safe and consistent with other devicetree parsing code:

  of_property_read_u32_array(pdev->dev.of_node, "phys-size", prop, 2);

It won't read the values if the property data's too short, so that
should be consistent with the existing code.

It would also make the diffstat negative :)

> 
> -	prop = of_get_property(op->dev.of_node, "resolution", &size);
> +	prop = of_get_property(pdev->dev.of_node, "resolution", &size);
>  	if ((prop) && (size >= sizeof(u32)*2)) {
>  		pdata.xres = prop[0];
>  		pdata.yres = prop[1];
>  	}
> 
> -	prop = of_get_property(op->dev.of_node, "virtual-resolution", &size);
> +	prop = of_get_property(pdev->dev.of_node, "virtual-resolution", &size);
>  	if ((prop) && (size >= sizeof(u32)*2)) {
>  		pdata.xvirt = prop[0];
>  		pdata.yvirt = prop[1];
>  	}
> 
> -	if (of_find_property(op->dev.of_node, "rotate-display", NULL))
> +	if (of_find_property(pdev->dev.of_node, "rotate-display", NULL))
>  		pdata.rotate_screen = 1;

Similarly, this could use of_property_read_bool:

  pdata.rotate_screen = of_property_read_bool(pdev->dev.of_node,
                                              "rotate-display");

It won't help the diffstat, but it makes the intent clearer.

Cheers,
Mark.

Re: [PATCH v2 1/3] video: xilinxfb: Use standard variable name convention

[Michal Simek]

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

On 10/09/2013 01:06 PM, Mark Rutland wrote:
> On Wed, Oct 09, 2013 at 11:52:12AM +0100, Michal Simek wrote:
>> s/op/pdev/ in xilinxfb_of_probe().
>> No functional chagnes.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>> Changes in v2: None
>>
>>  drivers/video/xilinxfb.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
>> index 0e1dd33..d12345f 100644
>> --- a/drivers/video/xilinxfb.c
>> +++ b/drivers/video/xilinxfb.c
>> @@ -411,7 +411,7 @@ static int xilinxfb_release(struct device *dev)
>>   * OF bus binding
>>   */
>>
>> -static int xilinxfb_of_probe(struct platform_device *op)
>> +static int xilinxfb_of_probe(struct platform_device *pdev)
>>  {
>>  	const u32 *prop;
>>  	u32 tft_access = 0;
>> @@ -425,7 +425,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
>>  	/* Allocate the driver data region */
>>  	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
>>  	if (!drvdata) {
>> -		dev_err(&op->dev, "Couldn't allocate device private record\n");
>> +		dev_err(&pdev->dev, "Couldn't allocate device private record\n");
>>  		return -ENOMEM;
>>  	}
>>
>> @@ -433,7 +433,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
>>  	 * To check whether the core is connected directly to DCR or BUS
>>  	 * interface and initialize the tft_access accordingly.
>>  	 */
>> -	of_property_read_u32(op->dev.of_node, "xlnx,dcr-splb-slave-if",
>> +	of_property_read_u32(pdev->dev.of_node, "xlnx,dcr-splb-slave-if",
>>  			     &tft_access);
>>
>>  	/*
>> @@ -457,29 +457,29 @@ static int xilinxfb_of_probe(struct platform_device *op)
>>  	}
>>  #endif
>>
>> -	prop = of_get_property(op->dev.of_node, "phys-size", &size);
>> +	prop = of_get_property(pdev->dev.of_node, "phys-size", &size);
>>  	if ((prop) && (size >= sizeof(u32)*2)) {
>>  		pdata.screen_width_mm = prop[0];
>>  		pdata.screen_height_mm = prop[1];
>>  	}
> 
> While you're changing these lines, it would be nice to change this
> pattern (here and elsewhere) to use of_property_read_u32_array, so that
> it's endian-safe and consistent with other devicetree parsing code:
> 
>   of_property_read_u32_array(pdev->dev.of_node, "phys-size", prop, 2);
> 
> It won't read the values if the property data's too short, so that
> should be consistent with the existing code.
> 
> It would also make the diffstat negative :)

The intention of this patch is simple rename which is exactly how
patch should look like. It means one change per patch.

It means these changes you have describe should be in separate patch
and I definitely agree with them.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]