2013-05-16 12:58:39

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 0/7] media: davinci: vpif trivial cleanup

From: Lad, Prabhakar <[email protected]>

This patch series cleans the VPIF driver, uses devm_* api wherever
required and uses module_platform_driver() to simplify the code.

This patch series applies on 3.10.rc1 and is tested on OMAP-L138.

Lad, Prabhakar (7):
media: davinci: vpif: remove unwanted header includes
media: davinci: vpif: Convert to devm_* api
media: davinci: vpif: remove unnecessary braces around defines
media: davinci: vpif_capture: remove unwanted header inclusion and
sort them alphabetically
media: davinci: vpif_capture: Convert to devm_* api
media: davinci: vpif_display: remove unwanted header inclusion and
sort them alphabetically
media: davinci: vpif_display: Convert to devm_* api

drivers/media/platform/davinci/vpif.c | 42 ++---------
drivers/media/platform/davinci/vpif_capture.c | 96 +++++--------------------
drivers/media/platform/davinci/vpif_capture.h | 6 +-
drivers/media/platform/davinci/vpif_display.c | 85 ++++------------------
drivers/media/platform/davinci/vpif_display.h | 6 +-
5 files changed, 46 insertions(+), 189 deletions(-)

--
1.7.4.1


2013-05-16 12:59:04

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 1/7] media: davinci: vpif: remove unwanted header includes

From: Lad, Prabhakar <[email protected]>

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index ea82a8b..d354d50 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -17,18 +17,11 @@
* GNU General Public License for more details.
*/

-#include <linux/init.h>
#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/spinlock.h>
-#include <linux/kernel.h>
-#include <linux/io.h>
-#include <linux/err.h>
#include <linux/pm_runtime.h>
#include <linux/v4l2-dv-timings.h>

-#include <mach/hardware.h>
-
#include "vpif.h"

MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
--
1.7.4.1

2013-05-16 12:59:23

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 2/7] media: davinci: vpif: Convert to devm_* api

From: Lad, Prabhakar <[email protected]>

Use devm_ioremap_resource instead of reques_mem_region()/ioremap().
This ensures more consistent error values and simplifies error paths.

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif.c | 27 ++++-----------------------
1 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index d354d50..7d028ca 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -32,8 +32,6 @@ MODULE_LICENSE("GPL");
#define VPIF_CH2_MAX_MODES (15)
#define VPIF_CH3_MAX_MODES (02)

-static resource_size_t res_len;
-static struct resource *res;
spinlock_t vpif_lock;

void __iomem *vpif_base;
@@ -416,23 +414,12 @@ EXPORT_SYMBOL(vpif_channel_getfid);

static int vpif_probe(struct platform_device *pdev)
{
- int status = 0;
+ static struct resource *res;

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res)
- return -ENOENT;
-
- res_len = resource_size(res);
-
- res = request_mem_region(res->start, res_len, res->name);
- if (!res)
- return -EBUSY;
-
- vpif_base = ioremap(res->start, res_len);
- if (!vpif_base) {
- status = -EBUSY;
- goto fail;
- }
+ vpif_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(vpif_base))
+ return PTR_ERR(vpif_base);

pm_runtime_enable(&pdev->dev);
pm_runtime_get(&pdev->dev);
@@ -440,17 +427,11 @@ static int vpif_probe(struct platform_device *pdev)
spin_lock_init(&vpif_lock);
dev_info(&pdev->dev, "vpif probe success\n");
return 0;
-
-fail:
- release_mem_region(res->start, res_len);
- return status;
}

static int vpif_remove(struct platform_device *pdev)
{
pm_runtime_disable(&pdev->dev);
- iounmap(vpif_base);
- release_mem_region(res->start, res_len);
return 0;
}

--
1.7.4.1

2013-05-16 12:59:49

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 3/7] media: davinci: vpif: remove unnecessary braces around defines

From: Lad, Prabhakar <[email protected]>

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index 7d028ca..1f2b2c6 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -27,10 +27,10 @@
MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
MODULE_LICENSE("GPL");

-#define VPIF_CH0_MAX_MODES (22)
-#define VPIF_CH1_MAX_MODES (02)
-#define VPIF_CH2_MAX_MODES (15)
-#define VPIF_CH3_MAX_MODES (02)
+#define VPIF_CH0_MAX_MODES 22
+#define VPIF_CH1_MAX_MODES 02
+#define VPIF_CH2_MAX_MODES 15
+#define VPIF_CH3_MAX_MODES 02

spinlock_t vpif_lock;

--
1.7.4.1

2013-05-16 12:59:57

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 4/7] media: davinci: vpif_capture: remove unwanted header inclusion and sort them alphabetically

From: Lad, Prabhakar <[email protected]>

This patch removes unwanted header inclusion and sorts them alphabetically

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif_capture.c | 21 +++++----------------
drivers/media/platform/davinci/vpif_capture.h | 6 ++----
2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 5f98df1..c26b6e4 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -18,28 +18,17 @@
* TODO : add support for VBI & HBI data service
* add static buffer allocation
*/
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/errno.h>
-#include <linux/fs.h>
-#include <linux/mm.h>
+
#include <linux/interrupt.h>
-#include <linux/workqueue.h>
-#include <linux/string.h>
-#include <linux/videodev2.h>
-#include <linux/wait.h>
-#include <linux/time.h>
-#include <linux/i2c.h>
+#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/io.h>
#include <linux/slab.h>
-#include <media/v4l2-device.h>
-#include <media/v4l2-ioctl.h>
+
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ioctl.h>

-#include "vpif_capture.h"
#include "vpif.h"
+#include "vpif_capture.h"

MODULE_DESCRIPTION("TI DaVinci VPIF Capture driver");
MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/davinci/vpif_capture.h b/drivers/media/platform/davinci/vpif_capture.h
index 3d3c1e5..517e7d1 100644
--- a/drivers/media/platform/davinci/vpif_capture.h
+++ b/drivers/media/platform/davinci/vpif_capture.h
@@ -22,11 +22,9 @@
#ifdef __KERNEL__

/* Header files */
-#include <linux/videodev2.h>
-#include <media/v4l2-common.h>
-#include <media/v4l2-device.h>
-#include <media/videobuf2-dma-contig.h>
#include <media/davinci/vpif_types.h>
+#include <media/videobuf2-dma-contig.h>
+#include <media/v4l2-device.h>

#include "vpif.h"

--
1.7.4.1

2013-05-16 13:00:08

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 5/7] media: davinci: vpif_capture: Convert to devm_* api

From: Lad, Prabhakar <[email protected]>

use devm_request_irq() instead of request_irq().
This ensures more consistent error values and simplifies error paths.

use module_platform_driver to simplify the code.

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif_capture.c | 75 +++++--------------------
1 files changed, 14 insertions(+), 61 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index c26b6e4..3e48c9c 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -2082,14 +2082,13 @@ static __init int vpif_probe(struct platform_device *pdev)

while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
for (i = res->start; i <= res->end; i++) {
- if (request_irq(i, vpif_channel_isr, IRQF_SHARED,
- "VPIF_Capture", (void *)
- (&vpif_obj.dev[res_idx]->channel_id))) {
- err = -EBUSY;
- for (j = 0; j < i; j++)
- free_irq(j, (void *)
- (&vpif_obj.dev[res_idx]->channel_id));
- goto vpif_int_err;
+ err = devm_request_irq(&pdev->dev, i, vpif_channel_isr,
+ IRQF_SHARED, "VPIF_Capture",
+ (void *)(&vpif_obj.dev[res_idx]->
+ channel_id));
+ if (err) {
+ err = -EINVAL;
+ goto vpif_unregister;
}
}
res_idx++;
@@ -2106,7 +2105,7 @@ static __init int vpif_probe(struct platform_device *pdev)
video_device_release(ch->video_dev);
}
err = -ENOMEM;
- goto vpif_int_err;
+ goto vpif_unregister;
}

/* Initialize field of video device */
@@ -2206,13 +2205,8 @@ vpif_sd_error:
/* Note: does nothing if ch->video_dev == NULL */
video_device_release(ch->video_dev);
}
-vpif_int_err:
+vpif_unregister:
v4l2_device_unregister(&vpif_obj.v4l2_dev);
- for (i = 0; i < res_idx; i++) {
- res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
- for (j = res->start; j <= res->end; j++)
- free_irq(j, (void *)(&vpif_obj.dev[i]->channel_id));
- }
return err;
}

@@ -2224,18 +2218,20 @@ vpif_int_err:
*/
static int vpif_remove(struct platform_device *device)
{
- int i;
struct channel_obj *ch;
+ int i;

v4l2_device_unregister(&vpif_obj.v4l2_dev);
-
+ kfree(vpif_obj.sd);
/* un-register device */
for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
/* Get the pointer to the channel object */
ch = vpif_obj.dev[i];
/* Unregister video device */
video_unregister_device(ch->video_dev);
+ kfree(vpif_obj.dev[i]);
}
+
return 0;
}

@@ -2325,47 +2321,4 @@ static __refdata struct platform_driver vpif_driver = {
.remove = vpif_remove,
};

-/**
- * vpif_init: initialize the vpif driver
- *
- * This function registers device and driver to the kernel, requests irq
- * handler and allocates memory
- * for channel objects
- */
-static __init int vpif_init(void)
-{
- return platform_driver_register(&vpif_driver);
-}
-
-/**
- * vpif_cleanup : This function clean up the vpif capture resources
- *
- * This will un-registers device and driver to the kernel, frees
- * requested irq handler and de-allocates memory allocated for channel
- * objects.
- */
-static void vpif_cleanup(void)
-{
- struct platform_device *pdev;
- struct resource *res;
- int irq_num;
- int i = 0;
-
- pdev = container_of(vpif_dev, struct platform_device, dev);
- while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
- for (irq_num = res->start; irq_num <= res->end; irq_num++)
- free_irq(irq_num,
- (void *)(&vpif_obj.dev[i]->channel_id));
- i++;
- }
-
- platform_driver_unregister(&vpif_driver);
-
- kfree(vpif_obj.sd);
- for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++)
- kfree(vpif_obj.dev[i]);
-}
-
-/* Function for module initialization and cleanup */
-module_init(vpif_init);
-module_exit(vpif_cleanup);
+module_platform_driver(vpif_driver);
--
1.7.4.1

2013-05-16 13:00:17

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 6/7] media: davinci: vpif_display: remove unwanted header inclusion and sort them alphabetically

From: Lad, Prabhakar <[email protected]>

This patch removes unwanted header inclusion and sorts them alphabetically

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif_display.c | 23 +++--------------------
drivers/media/platform/davinci/vpif_display.h | 6 ++----
2 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index 1b3fb5c..d833056 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -14,33 +14,16 @@
* GNU General Public License for more details.
*/

-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/errno.h>
-#include <linux/fs.h>
-#include <linux/mm.h>
#include <linux/interrupt.h>
-#include <linux/workqueue.h>
-#include <linux/string.h>
-#include <linux/videodev2.h>
-#include <linux/wait.h>
-#include <linux/time.h>
-#include <linux/i2c.h>
+#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/io.h>
#include <linux/slab.h>

-#include <asm/irq.h>
-#include <asm/page.h>
-
-#include <media/adv7343.h>
-#include <media/v4l2-device.h>
-#include <media/v4l2-ioctl.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ioctl.h>

-#include "vpif_display.h"
#include "vpif.h"
+#include "vpif_display.h"

MODULE_DESCRIPTION("TI DaVinci VPIF Display driver");
MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/davinci/vpif_display.h b/drivers/media/platform/davinci/vpif_display.h
index a5a18f7..b4bb7d2 100644
--- a/drivers/media/platform/davinci/vpif_display.h
+++ b/drivers/media/platform/davinci/vpif_display.h
@@ -17,11 +17,9 @@
#define DAVINCIHD_DISPLAY_H

/* Header files */
-#include <linux/videodev2.h>
-#include <media/v4l2-common.h>
-#include <media/v4l2-device.h>
-#include <media/videobuf2-dma-contig.h>
#include <media/davinci/vpif_types.h>
+#include <media/videobuf2-dma-contig.h>
+#include <media/v4l2-device.h>

#include "vpif.h"

--
1.7.4.1

2013-05-16 13:00:23

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 7/7] media: davinci: vpif_display: Convert to devm_* api

From: Lad, Prabhakar <[email protected]>

use devm_request_irq() instead of request_irq().
This ensures more consistent error values and simplifies error paths.

use module_platform_driver to simplify the code.

Signed-off-by: Lad, Prabhakar <[email protected]>
---
drivers/media/platform/davinci/vpif_display.c | 62 +++++--------------------
1 files changed, 12 insertions(+), 50 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index d833056..87e9381 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -1718,14 +1718,13 @@ static __init int vpif_probe(struct platform_device *pdev)

while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
for (i = res->start; i <= res->end; i++) {
- if (request_irq(i, vpif_channel_isr, IRQF_SHARED,
- "VPIF_Display", (void *)
- (&vpif_obj.dev[res_idx]->channel_id))) {
- err = -EBUSY;
- for (j = 0; j < i; j++)
- free_irq(j, (void *)
- (&vpif_obj.dev[res_idx]->channel_id));
- goto vpif_int_err;
+ err = devm_request_irq(&pdev->dev, i, vpif_channel_isr,
+ IRQF_SHARED, "VPIF_Display",
+ (void *)(&vpif_obj.dev[res_idx]->
+ channel_id));
+ if (err) {
+ err = -EINVAL;
+ goto vpif_unregister;
}
}
res_idx++;
@@ -1743,7 +1742,7 @@ static __init int vpif_probe(struct platform_device *pdev)
video_device_release(ch->video_dev);
}
err = -ENOMEM;
- goto vpif_int_err;
+ goto vpif_unregister;
}

/* Initialize field of video device */
@@ -1876,14 +1875,8 @@ vpif_sd_error:
/* Note: does nothing if ch->video_dev == NULL */
video_device_release(ch->video_dev);
}
-vpif_int_err:
+vpif_unregister:
v4l2_device_unregister(&vpif_obj.v4l2_dev);
- vpif_err("VPIF IRQ request failed\n");
- for (i = 0; i < res_idx; i++) {
- res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
- for (j = res->start; j <= res->end; j++)
- free_irq(j, (void *)(&vpif_obj.dev[i]->channel_id));
- }

return err;
}
@@ -1898,6 +1891,7 @@ static int vpif_remove(struct platform_device *device)

v4l2_device_unregister(&vpif_obj.v4l2_dev);

+ kfree(vpif_obj.sd);
/* un-register device */
for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
/* Get the pointer to the channel object */
@@ -1906,6 +1900,7 @@ static int vpif_remove(struct platform_device *device)
video_unregister_device(ch->video_dev);

ch->video_dev = NULL;
+ kfree(vpif_obj.dev[i]);
}

return 0;
@@ -1991,37 +1986,4 @@ static __refdata struct platform_driver vpif_driver = {
.remove = vpif_remove,
};

-static __init int vpif_init(void)
-{
- return platform_driver_register(&vpif_driver);
-}
-
-/*
- * vpif_cleanup: This function un-registers device and driver to the kernel,
- * frees requested irq handler and de-allocates memory allocated for channel
- * objects.
- */
-static void vpif_cleanup(void)
-{
- struct platform_device *pdev;
- struct resource *res;
- int irq_num;
- int i = 0;
-
- pdev = container_of(vpif_dev, struct platform_device, dev);
-
- while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
- for (irq_num = res->start; irq_num <= res->end; irq_num++)
- free_irq(irq_num,
- (void *)(&vpif_obj.dev[i]->channel_id));
- i++;
- }
-
- platform_driver_unregister(&vpif_driver);
- kfree(vpif_obj.sd);
- for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)
- kfree(vpif_obj.dev[i]);
-}
-
-module_init(vpif_init);
-module_exit(vpif_cleanup);
+module_platform_driver(vpif_driver);
--
1.7.4.1

2013-05-16 13:02:38

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH 1/7] media: davinci: vpif: remove unwanted header includes

Hi Prabhakar,

Thank you for the patch.

On Thursday 16 May 2013 18:28:16 Lad Prabhakar wrote:
> From: Lad, Prabhakar <[email protected]>
>
> Signed-off-by: Lad, Prabhakar <[email protected]>
> ---
> drivers/media/platform/davinci/vpif.c | 7 -------
> 1 files changed, 0 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpif.c
> b/drivers/media/platform/davinci/vpif.c index ea82a8b..d354d50 100644
> --- a/drivers/media/platform/davinci/vpif.c
> +++ b/drivers/media/platform/davinci/vpif.c
> @@ -17,18 +17,11 @@
> * GNU General Public License for more details.
> */
>
> -#include <linux/init.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> -#include <linux/spinlock.h>
> -#include <linux/kernel.h>
> -#include <linux/io.h>
> -#include <linux/err.h>
> #include <linux/pm_runtime.h>
> #include <linux/v4l2-dv-timings.h>

I think you should keep most of those includes. For instance this file uses
spinlock functions, so linux/spinlock.h should be included. It might work fine
now due to nested includes, but if someone reorganizes the kernel headers
internal includes then the driver might break. As a general rule of good
practice you should include headers for all the APIs you use.

>
> -#include <mach/hardware.h>
> -
> #include "vpif.h"
>
> MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
--
Regards,

Laurent Pinchart

2013-05-17 05:10:46

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 1/7] media: davinci: vpif: remove unwanted header includes

Hi Laurent,

Thanks for the review.

On Thu, May 16, 2013 at 6:32 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Thursday 16 May 2013 18:28:16 Lad Prabhakar wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> ---
>> drivers/media/platform/davinci/vpif.c | 7 -------
>> 1 files changed, 0 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/davinci/vpif.c
>> b/drivers/media/platform/davinci/vpif.c index ea82a8b..d354d50 100644
>> --- a/drivers/media/platform/davinci/vpif.c
>> +++ b/drivers/media/platform/davinci/vpif.c
>> @@ -17,18 +17,11 @@
>> * GNU General Public License for more details.
>> */
>>
>> -#include <linux/init.h>
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> -#include <linux/spinlock.h>
>> -#include <linux/kernel.h>
>> -#include <linux/io.h>
>> -#include <linux/err.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/v4l2-dv-timings.h>
>
> I think you should keep most of those includes. For instance this file uses
> spinlock functions, so linux/spinlock.h should be included. It might work fine
> now due to nested includes, but if someone reorganizes the kernel headers
> internal includes then the driver might break. As a general rule of good
> practice you should include headers for all the APIs you use.
>
OK, do you want me too drop the similar patches from this series ?

Regards,
--Prabhakar Lad

2013-05-17 09:57:53

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH 1/7] media: davinci: vpif: remove unwanted header includes

Hi Prabhakar,

On Friday 17 May 2013 10:40:24 Prabhakar Lad wrote:
> On Thu, May 16, 2013 at 6:32 PM, Laurent Pinchart wrote:
> > On Thursday 16 May 2013 18:28:16 Lad Prabhakar wrote:
> >> From: Lad, Prabhakar <[email protected]>
> >>
> >> Signed-off-by: Lad, Prabhakar <[email protected]>
> >> ---
> >>
> >> drivers/media/platform/davinci/vpif.c | 7 -------
> >> 1 files changed, 0 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/davinci/vpif.c
> >> b/drivers/media/platform/davinci/vpif.c index ea82a8b..d354d50 100644
> >> --- a/drivers/media/platform/davinci/vpif.c
> >> +++ b/drivers/media/platform/davinci/vpif.c
> >> @@ -17,18 +17,11 @@
> >> * GNU General Public License for more details.
> >> */
> >>
> >> -#include <linux/init.h>
> >> #include <linux/module.h>
> >> #include <linux/platform_device.h>
> >> -#include <linux/spinlock.h>
> >> -#include <linux/kernel.h>
> >> -#include <linux/io.h>
> >> -#include <linux/err.h>
> >> #include <linux/pm_runtime.h>
> >> #include <linux/v4l2-dv-timings.h>
> >
> > I think you should keep most of those includes. For instance this file
> > uses spinlock functions, so linux/spinlock.h should be included. It might
> > work fine now due to nested includes, but if someone reorganizes the
> > kernel headers internal includes then the driver might break. As a general
> > rule of good practice you should include headers for all the APIs you use.
>
> OK, do you want me too drop the similar patches from this series ?

Please at least go through them and make sure to keep the includes for APIs
used in the file. If there's unneeded includes you can of course remove them,
and if it turns out that all includes are useful please drop the patch.

--
Regards,

Laurent Pinchart

2013-05-23 09:38:31

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH 0/7] media: davinci: vpif trivial cleanup

On Thu 16 May 2013 14:58:15 Lad Prabhakar wrote:
> From: Lad, Prabhakar <[email protected]>
>
> This patch series cleans the VPIF driver, uses devm_* api wherever
> required and uses module_platform_driver() to simplify the code.
>
> This patch series applies on 3.10.rc1 and is tested on OMAP-L138.

Can you repost this taken into account Laurent's comments regarding the
unwanted header includes?

Thanks,

Hans

>
> Lad, Prabhakar (7):
> media: davinci: vpif: remove unwanted header includes
> media: davinci: vpif: Convert to devm_* api
> media: davinci: vpif: remove unnecessary braces around defines
> media: davinci: vpif_capture: remove unwanted header inclusion and
> sort them alphabetically
> media: davinci: vpif_capture: Convert to devm_* api
> media: davinci: vpif_display: remove unwanted header inclusion and
> sort them alphabetically
> media: davinci: vpif_display: Convert to devm_* api
>
> drivers/media/platform/davinci/vpif.c | 42 ++---------
> drivers/media/platform/davinci/vpif_capture.c | 96 +++++--------------------
> drivers/media/platform/davinci/vpif_capture.h | 6 +-
> drivers/media/platform/davinci/vpif_display.c | 85 ++++------------------
> drivers/media/platform/davinci/vpif_display.h | 6 +-
> 5 files changed, 46 insertions(+), 189 deletions(-)
>
>

2013-05-23 09:40:24

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 0/7] media: davinci: vpif trivial cleanup

Hi Hans,

On Thu, May 23, 2013 at 3:08 PM, Hans Verkuil <[email protected]> wrote:
> On Thu 16 May 2013 14:58:15 Lad Prabhakar wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> This patch series cleans the VPIF driver, uses devm_* api wherever
>> required and uses module_platform_driver() to simplify the code.
>>
>> This patch series applies on 3.10.rc1 and is tested on OMAP-L138.
>
> Can you repost this taken into account Laurent's comments regarding the
> unwanted header includes?
>
Yes I plan to do this weekend.

Regards,
--Prabhakar Lad