| Message ID | 1369569612-30915-5-git-send-email-prabhakar.csengg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 26-05-2013 16:00, Prabhakar Lad wrote: > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Ideally the freeing of irq's and the global variables needs to be > done in the remove() rather than module_exit(), this patch moves > the freeing up of irq's and freeing the memory allocated to channel > objects to remove() callback of struct platform_driver. > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> > --- > drivers/media/platform/davinci/vpif_capture.c | 31 ++++++++++-------------- > 1 files changed, 13 insertions(+), 18 deletions(-) > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c > index caaf4fe..f8b7304 100644 > --- a/drivers/media/platform/davinci/vpif_capture.c > +++ b/drivers/media/platform/davinci/vpif_capture.c > @@ -2225,17 +2225,29 @@ vpif_int_err: > */ > static int vpif_remove(struct platform_device *device) > { > - int i; > + struct platform_device *pdev; > struct channel_obj *ch; > + struct resource *res; > + int irq_num, i = 0; > + > + pdev = container_of(vpif_dev, struct platform_device, dev); Why you need this if you should be already called with the correct platform device? WBR, Sergei
Hi Prabhakar, Thanks for the patch. On Sunday 26 May 2013 17:30:07 Prabhakar Lad wrote: > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Ideally the freeing of irq's and the global variables needs to be > done in the remove() rather than module_exit(), this patch moves > the freeing up of irq's and freeing the memory allocated to channel > objects to remove() callback of struct platform_driver. > > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> > --- > drivers/media/platform/davinci/vpif_capture.c | 31 ++++++++++------------ > 1 files changed, 13 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/platform/davinci/vpif_capture.c > b/drivers/media/platform/davinci/vpif_capture.c index caaf4fe..f8b7304 > 100644 > --- a/drivers/media/platform/davinci/vpif_capture.c > +++ b/drivers/media/platform/davinci/vpif_capture.c > @@ -2225,17 +2225,29 @@ vpif_int_err: > */ > static int vpif_remove(struct platform_device *device) > { > - int i; > + struct platform_device *pdev; > struct channel_obj *ch; > + struct resource *res; > + int irq_num, i = 0; > + > + pdev = container_of(vpif_dev, struct platform_device, dev); As Sergei mentioned, the platform device is already passed to the function as an argument. > + 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)); A quick look at board code shows that each IRQ resource contains a single IRQ. The second loop could thus be removed. You could also add another patch to perform similar cleanup for the probe code. > + 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; > } > @@ -2347,24 +2359,7 @@ static __init int vpif_init(void) > */ > 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 */
Hi Laurent, Thanks for the review. On Wed, May 29, 2013 at 8:02 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Prabhakar, > > Thanks for the patch. > > On Sunday 26 May 2013 17:30:07 Prabhakar Lad wrote: >> From: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> >> Ideally the freeing of irq's and the global variables needs to be >> done in the remove() rather than module_exit(), this patch moves >> the freeing up of irq's and freeing the memory allocated to channel >> objects to remove() callback of struct platform_driver. >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> --- >> drivers/media/platform/davinci/vpif_capture.c | 31 ++++++++++------------ >> 1 files changed, 13 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c >> b/drivers/media/platform/davinci/vpif_capture.c index caaf4fe..f8b7304 >> 100644 >> --- a/drivers/media/platform/davinci/vpif_capture.c >> +++ b/drivers/media/platform/davinci/vpif_capture.c >> @@ -2225,17 +2225,29 @@ vpif_int_err: >> */ >> static int vpif_remove(struct platform_device *device) >> { >> - int i; >> + struct platform_device *pdev; >> struct channel_obj *ch; >> + struct resource *res; >> + int irq_num, i = 0; >> + >> + pdev = container_of(vpif_dev, struct platform_device, dev); > > As Sergei mentioned, the platform device is already passed to the function as > an argument. > OK >> + 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)); > > A quick look at board code shows that each IRQ resource contains a single IRQ. > The second loop could thus be removed. You could also add another patch to > perform similar cleanup for the probe code. > Any way this will code will be removed in the next patch of the same series due to usage of devm_* api. I'll do this change only in the probe code. Regards, --Prabhakar Lad
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c index caaf4fe..f8b7304 100644 --- a/drivers/media/platform/davinci/vpif_capture.c +++ b/drivers/media/platform/davinci/vpif_capture.c @@ -2225,17 +2225,29 @@ vpif_int_err: */ static int vpif_remove(struct platform_device *device) { - int i; + struct platform_device *pdev; struct channel_obj *ch; + struct resource *res; + int irq_num, 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++; + } 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; } @@ -2347,24 +2359,7 @@ static __init int vpif_init(void) */ 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 */