| Message ID | 20170317170059.17821-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Philipp Zabel <p.zabel@pengutronix.de> writes: > Use platform_register_drivers instead of open coding the iteration over > component platform drivers in the vc4_drv module. Reviewed and applied. Thanks!
Hi Philipp, I think you patch is OK, just a small question about the existing code. It might be better suited for Eric... not sure. On 17 March 2017 at 17:00, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Use platform_register_drivers instead of open coding the iteration over > component platform drivers in the vc4_drv module. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c > index 205c1961ffb4c..61e674baf3a6f 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.c > +++ b/drivers/gpu/drm/vc4/vc4_drv.c > @@ -349,26 +349,20 @@ static struct platform_driver vc4_platform_driver = { > > static int __init vc4_drm_register(void) > { > - int i, ret; > + int ret; > + > + ret = platform_register_drivers(component_drivers, > + ARRAY_SIZE(component_drivers)); > + if (ret) > + return ret; > > - for (i = 0; i < ARRAY_SIZE(component_drivers); i++) { > - ret = platform_driver_register(component_drivers[i]); > - if (ret) { > - while (--i >= 0) > - platform_driver_unregister(component_drivers[i]); > - return ret; > - } > - } > return platform_driver_register(&vc4_platform_driver); Is there any reason why vc4_platform_driver isn't part of the component_drivers array ? > } > > static void __exit vc4_drm_unregister(void) > { > - int i; > - > - for (i = ARRAY_SIZE(component_drivers) - 1; i >= 0; i--) > - platform_driver_unregister(component_drivers[i]); > - > + platform_unregister_drivers(component_drivers, > + ARRAY_SIZE(component_drivers)); > platform_driver_unregister(&vc4_platform_driver); Order seems wrong here - shouldn't one unregister vc4_platform_driver first here ? Perhaps that 's the reason why it is handled separately in vc4_drm_register. There's [seemingly] no comment that covers this so it seems like a copy/paste mistake. Thanks Emil
On Sun, 2017-03-19 at 11:28 +0000, Emil Velikov wrote: > Hi Philipp, > > I think you patch is OK, just a small question about the existing code. > It might be better suited for Eric... not sure. > > On 17 March 2017 at 17:00, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Use platform_register_drivers instead of open coding the iteration over > > component platform drivers in the vc4_drv module. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++-------------- > > 1 file changed, 8 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c > > index 205c1961ffb4c..61e674baf3a6f 100644 > > --- a/drivers/gpu/drm/vc4/vc4_drv.c > > +++ b/drivers/gpu/drm/vc4/vc4_drv.c > > @@ -349,26 +349,20 @@ static struct platform_driver vc4_platform_driver = { > > > > static int __init vc4_drm_register(void) > > { > > - int i, ret; > > + int ret; > > + > > + ret = platform_register_drivers(component_drivers, > > + ARRAY_SIZE(component_drivers)); > > + if (ret) > > + return ret; > > > > - for (i = 0; i < ARRAY_SIZE(component_drivers); i++) { > > - ret = platform_driver_register(component_drivers[i]); > > - if (ret) { > > - while (--i >= 0) > > - platform_driver_unregister(component_drivers[i]); > > - return ret; > > - } > > - } > > return platform_driver_register(&vc4_platform_driver); > Is there any reason why vc4_platform_driver isn't part of the > component_drivers array ? It is separate from the array because the same array is also used to set up the matches for the component_master_add_with_match call from the vc4_platform_driver's probe function. > > } > > > > static void __exit vc4_drm_unregister(void) > > { > > - int i; > > - > > - for (i = ARRAY_SIZE(component_drivers) - 1; i >= 0; i--) > > - platform_driver_unregister(component_drivers[i]); > > - > > + platform_unregister_drivers(component_drivers, > > + ARRAY_SIZE(component_drivers)); > > platform_driver_unregister(&vc4_platform_driver); > Order seems wrong here - shouldn't one unregister vc4_platform_driver > first here ? > Perhaps that 's the reason why it is handled separately in vc4_drm_register. > > There's [seemingly] no comment that covers this so it seems like a > copy/paste mistake. I think it would make sense to unregister the vc4_platform_driver first, but since this was there from the start I don't know if this was done differently on purpose. regards Philipp
Emil Velikov <emil.l.velikov@gmail.com> writes: > Hi Philipp, > > I think you patch is OK, just a small question about the existing code. > It might be better suited for Eric... not sure. > > On 17 March 2017 at 17:00, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> Use platform_register_drivers instead of open coding the iteration over >> component platform drivers in the vc4_drv module. >> >> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >> --- >> drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++-------------- >> 1 file changed, 8 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c >> index 205c1961ffb4c..61e674baf3a6f 100644 >> --- a/drivers/gpu/drm/vc4/vc4_drv.c >> +++ b/drivers/gpu/drm/vc4/vc4_drv.c >> @@ -349,26 +349,20 @@ static struct platform_driver vc4_platform_driver = { >> >> static int __init vc4_drm_register(void) >> { >> - int i, ret; >> + int ret; >> + >> + ret = platform_register_drivers(component_drivers, >> + ARRAY_SIZE(component_drivers)); >> + if (ret) >> + return ret; >> >> - for (i = 0; i < ARRAY_SIZE(component_drivers); i++) { >> - ret = platform_driver_register(component_drivers[i]); >> - if (ret) { >> - while (--i >= 0) >> - platform_driver_unregister(component_drivers[i]); >> - return ret; >> - } >> - } >> return platform_driver_register(&vc4_platform_driver); > Is there any reason why vc4_platform_driver isn't part of the > component_drivers array ? It could be moved in. >> } >> >> static void __exit vc4_drm_unregister(void) >> { >> - int i; >> - >> - for (i = ARRAY_SIZE(component_drivers) - 1; i >= 0; i--) >> - platform_driver_unregister(component_drivers[i]); >> - >> + platform_unregister_drivers(component_drivers, >> + ARRAY_SIZE(component_drivers)); >> platform_driver_unregister(&vc4_platform_driver); > Order seems wrong here - shouldn't one unregister vc4_platform_driver > first here ? > Perhaps that 's the reason why it is handled separately in vc4_drm_register. I don't think it matters either way.
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 205c1961ffb4c..61e674baf3a6f 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -349,26 +349,20 @@ static struct platform_driver vc4_platform_driver = { static int __init vc4_drm_register(void) { - int i, ret; + int ret; + + ret = platform_register_drivers(component_drivers, + ARRAY_SIZE(component_drivers)); + if (ret) + return ret; - for (i = 0; i < ARRAY_SIZE(component_drivers); i++) { - ret = platform_driver_register(component_drivers[i]); - if (ret) { - while (--i >= 0) - platform_driver_unregister(component_drivers[i]); - return ret; - } - } return platform_driver_register(&vc4_platform_driver); } static void __exit vc4_drm_unregister(void) { - int i; - - for (i = ARRAY_SIZE(component_drivers) - 1; i >= 0; i--) - platform_driver_unregister(component_drivers[i]); - + platform_unregister_drivers(component_drivers, + ARRAY_SIZE(component_drivers)); platform_driver_unregister(&vc4_platform_driver); }
Use platform_register_drivers instead of open coding the iteration over component platform drivers in the vc4_drv module. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)