| Message ID | 1445184015-24414-3-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sunday 18 October 2015 18:00:13 Eric Auger wrote: > diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > index 619dc7d..4f76b17 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > @@ -29,8 +29,7 @@ > #define DRIVER_VERSION "0.1" > #define DRIVER_AUTHOR "Eric Auger <eric.auger@linaro.org>" > #define DRIVER_DESC "Reset support for Calxeda xgmac vfio platform device" > - > -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac" > +#define COMPAT "calxeda,hb-xgmac" Why the rename? > /* XGMAC Register definitions */ > #define XGMAC_CONTROL 0x00000000 /* MAC Configuration */ > @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev) > } > EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset); > > +static int __init vfio_platform_calxedaxgmac_init(void) > +{ > + int (*register_reset)(struct module *, char*, > + vfio_platform_reset_fn_t); > + int ret; > + > + register_reset = symbol_get(vfio_platform_register_reset); > + if (!register_reset) > + return -EINVAL; I don't understand what you do the symbol_get() here for. Why not just call that function directly? > + ret = register_reset(THIS_MODULE, COMPAT, > + vfio_platform_calxedaxgmac_reset); This is whitespace damaged, the second line needs to be indented to the opening braces. I would also suggest defining register_reset as a macro that implicitly passes the THIS_MODULE argument, as other subsystems do. Arnd
Hi Arnd, On 10/19/2015 03:04 PM, Arnd Bergmann wrote: > On Sunday 18 October 2015 18:00:13 Eric Auger wrote: >> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c >> index 619dc7d..4f76b17 100644 >> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c >> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c >> @@ -29,8 +29,7 @@ >> #define DRIVER_VERSION "0.1" >> #define DRIVER_AUTHOR "Eric Auger <eric.auger@linaro.org>" >> #define DRIVER_DESC "Reset support for Calxeda xgmac vfio platform device" >> - >> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac" >> +#define COMPAT "calxeda,hb-xgmac" > > Why the rename? This define was not used. Since the code is meant to be duplicated from one reset module to the other I thought it did not bring any have a specialized name > >> /* XGMAC Register definitions */ >> #define XGMAC_CONTROL 0x00000000 /* MAC Configuration */ >> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev) >> } >> EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset); >> >> +static int __init vfio_platform_calxedaxgmac_init(void) >> +{ >> + int (*register_reset)(struct module *, char*, >> + vfio_platform_reset_fn_t); >> + int ret; >> + >> + register_reset = symbol_get(vfio_platform_register_reset); >> + if (!register_reset) >> + return -EINVAL; > > I don't understand what you do the symbol_get() here for. > Why not just call that function directly the function is implemented in a separate module. This is just to make sure the vfio-platform module is loaded, in case the end-user attempts to load the reset module without having this latter loaded. > >> + ret = register_reset(THIS_MODULE, COMPAT, >> + vfio_platform_calxedaxgmac_reset); > > This is whitespace damaged, the second line needs to be indented > to the opening braces. ok > > I would also suggest defining register_reset as a macro that > implicitly passes the THIS_MODULE argument, as other subsystems > do. ok Thanks Best Regards Eric > > Arnd >
On Monday 19 October 2015 15:17:30 Eric Auger wrote: > Hi Arnd, > On 10/19/2015 03:04 PM, Arnd Bergmann wrote: > > On Sunday 18 October 2015 18:00:13 Eric Auger wrote: > >> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > >> index 619dc7d..4f76b17 100644 > >> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > >> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c > >> @@ -29,8 +29,7 @@ > >> #define DRIVER_VERSION "0.1" > >> #define DRIVER_AUTHOR "Eric Auger <eric.auger@linaro.org>" > >> #define DRIVER_DESC "Reset support for Calxeda xgmac vfio platform device" > >> - > >> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac" > >> +#define COMPAT "calxeda,hb-xgmac" > > > > Why the rename? > This define was not used. Since the code is meant to be duplicated from > one reset module to the other I thought it did not bring any have a > specialized name I'd say it would be clearer to remove the macro then, and pass the string literal in the function call. > >> /* XGMAC Register definitions */ > >> #define XGMAC_CONTROL 0x00000000 /* MAC Configuration */ > >> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev) > >> } > >> EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset); > >> > >> +static int __init vfio_platform_calxedaxgmac_init(void) > >> +{ > >> + int (*register_reset)(struct module *, char*, > >> + vfio_platform_reset_fn_t); > >> + int ret; > >> + > >> + register_reset = symbol_get(vfio_platform_register_reset); > >> + if (!register_reset) > >> + return -EINVAL; > > > > I don't understand what you do the symbol_get() here for. > > Why not just call that function directly > the function is implemented in a separate module. This is just to make > sure the vfio-platform module is loaded, in case the end-user attempts > to load the reset module without having this latter loaded. The module loader does all this for you. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 10/19/2015 03:25 PM, Arnd Bergmann wrote: > On Monday 19 October 2015 15:17:30 Eric Auger wrote: >> Hi Arnd, >> On 10/19/2015 03:04 PM, Arnd Bergmann wrote: >>> On Sunday 18 October 2015 18:00:13 Eric Auger wrote: >>>> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c >>>> index 619dc7d..4f76b17 100644 >>>> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c >>>> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c >>>> @@ -29,8 +29,7 @@ >>>> #define DRIVER_VERSION "0.1" >>>> #define DRIVER_AUTHOR "Eric Auger <eric.auger@linaro.org>" >>>> #define DRIVER_DESC "Reset support for Calxeda xgmac vfio platform device" >>>> - >>>> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac" >>>> +#define COMPAT "calxeda,hb-xgmac" >>> >>> Why the rename? >> This define was not used. Since the code is meant to be duplicated from >> one reset module to the other I thought it did not bring any have a >> specialized name > > I'd say it would be clearer to remove the macro then, and pass the > string literal in the function call. OK > >>>> /* XGMAC Register definitions */ >>>> #define XGMAC_CONTROL 0x00000000 /* MAC Configuration */ >>>> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev) >>>> } >>>> EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset); >>>> >>>> +static int __init vfio_platform_calxedaxgmac_init(void) >>>> +{ >>>> + int (*register_reset)(struct module *, char*, >>>> + vfio_platform_reset_fn_t); >>>> + int ret; >>>> + >>>> + register_reset = symbol_get(vfio_platform_register_reset); >>>> + if (!register_reset) >>>> + return -EINVAL; >>> >>> I don't understand what you do the symbol_get() here for. >>> Why not just call that function directly >> the function is implemented in a separate module. This is just to make >> sure the vfio-platform module is loaded, in case the end-user attempts >> to load the reset module without having this latter loaded. > > The module loader does all this for you. Ah OK I will respin shortly taking into account all your comments Thanks for your time! Best Regards Eric > > Arnd >
diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c index 619dc7d..4f76b17 100644 --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c @@ -29,8 +29,7 @@ #define DRIVER_VERSION "0.1" #define DRIVER_AUTHOR "Eric Auger <eric.auger@linaro.org>" #define DRIVER_DESC "Reset support for Calxeda xgmac vfio platform device" - -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac" +#define COMPAT "calxeda,hb-xgmac" /* XGMAC Register definitions */ #define XGMAC_CONTROL 0x00000000 /* MAC Configuration */ @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev) } EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset); +static int __init vfio_platform_calxedaxgmac_init(void) +{ + int (*register_reset)(struct module *, char*, + vfio_platform_reset_fn_t); + int ret; + + register_reset = symbol_get(vfio_platform_register_reset); + if (!register_reset) + return -EINVAL; + + ret = register_reset(THIS_MODULE, COMPAT, + vfio_platform_calxedaxgmac_reset); + + symbol_put(vfio_platform_register_reset); + + return ret; +} + +static void __exit vfio_platform_calxedaxgmac_exit(void) +{ + int (*unregister_reset)(char *); + int ret; + + unregister_reset = symbol_get(vfio_platform_unregister_reset); + if (!unregister_reset) + return; + + ret = unregister_reset(COMPAT); + + symbol_put(vfio_platform_unregister_reset); +} + +module_init(vfio_platform_calxedaxgmac_init); +module_exit(vfio_platform_calxedaxgmac_exit); + +MODULE_ALIAS("vfio-reset:" COMPAT); + MODULE_VERSION(DRIVER_VERSION); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR(DRIVER_AUTHOR);
This patch adds the reset function registration/unregistration. Also a MODULE_ALIAS is added. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- .../platform/reset/vfio_platform_calxedaxgmac.c | 40 ++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)