| Message ID | 1455016095-13724-1-git-send-email-javier@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-02-09 at 08:08 -0300, Javier Martinez Canillas wrote: > The platform bus_type .match callback attempts to match the platform device > name with an entry on the .id_table if provided and fallbacks to match with > the driver's name if a table is not provided. > > Using a platform device ID to match is more explicit, allows the driver to > support more than one device and also the MODULE_DEVICE_TABLE macro can be > used to export the module aliases information instead of the MODULE_ALIAS. > > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > --- > > drivers/rtc/rtc-mt6397.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > index 06a5c52b292f..46533f11f7fc 100644 > --- a/drivers/rtc/rtc-mt6397.c > +++ b/drivers/rtc/rtc-mt6397.c > @@ -404,6 +404,12 @@ static const struct of_device_id mt6397_rtc_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > > +static const struct platform_device_id mt6397_rtc_id[] = { > + {"mt6397-rtc", 0}, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(platform, mt6397_rtc_id); > + > static struct platform_driver mtk_rtc_driver = { > .driver = { > .name = "mt6397-rtc", > @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { > }, > .probe = mtk_rtc_probe, > .remove = mtk_rtc_remove, > + .id_table = mt6397_rtc_id, > }; > > module_platform_driver(mtk_rtc_driver); > @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); > MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); > -MODULE_ALIAS("platform:mt6397-rtc"); This patch looks good to me, but I am wondering, since we tend to use device tree method to match driver, do we still need support platform device ID ? Eddie
Hello Eddie, On 02/14/2016 10:58 PM, Eddie Huang wrote: [snip] >> @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { >> }, >> .probe = mtk_rtc_probe, >> .remove = mtk_rtc_remove, >> + .id_table = mt6397_rtc_id, >> }; >> >> module_platform_driver(mtk_rtc_driver); >> @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); >> MODULE_LICENSE("GPL v2"); >> MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); >> MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); >> -MODULE_ALIAS("platform:mt6397-rtc"); > > This patch looks good to me, but I am wondering, since we tend to use > device tree method to match driver, do we still need support platform > device ID ? > I'm not familiar with neither this IP block nor the SoC so it is up to you. I just noticed this issue when reviewing a regulator driver for a similar PMIC posted by someone from mediatek. I thought platform device was needed since the driver has a MODULE_ALIAS() but please let me know what you prefer and I can re-spin the patch and just remove the MODULE_ALIAS() if that makes more sense for this platform. > Eddie > > Best regards,
On Monday 15 February 2016 11:50:48 Javier Martinez Canillas wrote: > > On 02/14/2016 10:58 PM, Eddie Huang wrote: > > [snip] > > >> @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { > >> }, > >> .probe = mtk_rtc_probe, > >> .remove = mtk_rtc_remove, > >> + .id_table = mt6397_rtc_id, > >> }; > >> > >> module_platform_driver(mtk_rtc_driver); > >> @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); > >> MODULE_LICENSE("GPL v2"); > >> MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); > >> MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); > >> -MODULE_ALIAS("platform:mt6397-rtc"); > > > > This patch looks good to me, but I am wondering, since we tend to use > > device tree method to match driver, do we still need support platform > > device ID ? > > > > I'm not familiar with neither this IP block nor the SoC so it is up to > you. I just noticed this issue when reviewing a regulator driver for a > similar PMIC posted by someone from mediatek. > > I thought platform device was needed since the driver has a MODULE_ALIAS() > but please let me know what you prefer and I can re-spin the patch and > just remove the MODULE_ALIAS() if that makes more sense for this platform. > > I agree. We can alway add a MODULE_DEVICE_TABLE() if we get multiple users of this driver on architectures that don't use devicetree yet. Arnd
On Tue, 2016-02-16 at 12:37 +0100, Arnd Bergmann wrote: > On Monday 15 February 2016 11:50:48 Javier Martinez Canillas wrote: > > > > On 02/14/2016 10:58 PM, Eddie Huang wrote: > > > > [snip] > > > > >> @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { > > >> }, > > >> .probe = mtk_rtc_probe, > > >> .remove = mtk_rtc_remove, > > >> + .id_table = mt6397_rtc_id, > > >> }; > > >> > > >> module_platform_driver(mtk_rtc_driver); > > >> @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); > > >> MODULE_LICENSE("GPL v2"); > > >> MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); > > >> MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); > > >> -MODULE_ALIAS("platform:mt6397-rtc"); > > > > > > This patch looks good to me, but I am wondering, since we tend to use > > > device tree method to match driver, do we still need support platform > > > device ID ? > > > > > > > I'm not familiar with neither this IP block nor the SoC so it is up to > > you. I just noticed this issue when reviewing a regulator driver for a > > similar PMIC posted by someone from mediatek. > > > > I thought platform device was needed since the driver has a MODULE_ALIAS() > > but please let me know what you prefer and I can re-spin the patch and > > just remove the MODULE_ALIAS() if that makes more sense for this platform. > > > > > > I agree. We can alway add a MODULE_DEVICE_TABLE() if we get multiple > users of this driver on architectures that don't use devicetree yet. > Sure. Thanks the patch to add expandability to this driver. Acked-by: Eddie Huang <eddie.huang@mediatek.com> Eddie
On Tuesday 16 February 2016 21:19:07 Eddie Huang wrote: > On Tue, 2016-02-16 at 12:37 +0100, Arnd Bergmann wrote: > > On Monday 15 February 2016 11:50:48 Javier Martinez Canillas wrote: > > > > > > On 02/14/2016 10:58 PM, Eddie Huang wrote: > > > > > > [snip] > > > > > > >> @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { > > > >> }, > > > >> .probe = mtk_rtc_probe, > > > >> .remove = mtk_rtc_remove, > > > >> + .id_table = mt6397_rtc_id, > > > >> }; > > > >> > > > >> module_platform_driver(mtk_rtc_driver); > > > >> @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); > > > >> MODULE_LICENSE("GPL v2"); > > > >> MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); > > > >> MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); > > > >> -MODULE_ALIAS("platform:mt6397-rtc"); > > > > > > > > This patch looks good to me, but I am wondering, since we tend to use > > > > device tree method to match driver, do we still need support platform > > > > device ID ? > > > > > > > > > > I'm not familiar with neither this IP block nor the SoC so it is up to > > > you. I just noticed this issue when reviewing a regulator driver for a > > > similar PMIC posted by someone from mediatek. > > > > > > I thought platform device was needed since the driver has a MODULE_ALIAS() > > > but please let me know what you prefer and I can re-spin the patch and > > > just remove the MODULE_ALIAS() if that makes more sense for this platform. > > > > > > > > > > I agree. We can alway add a MODULE_DEVICE_TABLE() if we get multiple > > users of this driver on architectures that don't use devicetree yet. > > > > Sure. Thanks the patch to add expandability to this driver. > > Acked-by: Eddie Huang <eddie.huang@mediatek.com> I think we misunderstood one another. I think we can drop both the MODULE_DEVICE_TABLE and the MODULE_ALIAS: there is no need for another driver ID when it is always probed using DT. Arnd
Hello Arnd, On 02/24/2016 01:56 PM, Arnd Bergmann wrote: > On Tuesday 16 February 2016 21:19:07 Eddie Huang wrote: >> On Tue, 2016-02-16 at 12:37 +0100, Arnd Bergmann wrote: >>> On Monday 15 February 2016 11:50:48 Javier Martinez Canillas wrote: >>>> >>>> On 02/14/2016 10:58 PM, Eddie Huang wrote: >>>> >>>> [snip] >>>> >>>>>> @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { >>>>>> }, >>>>>> .probe = mtk_rtc_probe, >>>>>> .remove = mtk_rtc_remove, >>>>>> + .id_table = mt6397_rtc_id, >>>>>> }; >>>>>> >>>>>> module_platform_driver(mtk_rtc_driver); >>>>>> @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); >>>>>> MODULE_LICENSE("GPL v2"); >>>>>> MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); >>>>>> MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); >>>>>> -MODULE_ALIAS("platform:mt6397-rtc"); >>>>> >>>>> This patch looks good to me, but I am wondering, since we tend to use >>>>> device tree method to match driver, do we still need support platform >>>>> device ID ? >>>>> >>>> >>>> I'm not familiar with neither this IP block nor the SoC so it is up to >>>> you. I just noticed this issue when reviewing a regulator driver for a >>>> similar PMIC posted by someone from mediatek. >>>> >>>> I thought platform device was needed since the driver has a MODULE_ALIAS() >>>> but please let me know what you prefer and I can re-spin the patch and >>>> just remove the MODULE_ALIAS() if that makes more sense for this platform. >>>> >>>> >>> >>> I agree. We can alway add a MODULE_DEVICE_TABLE() if we get multiple >>> users of this driver on architectures that don't use devicetree yet. >>> >> >> Sure. Thanks the patch to add expandability to this driver. >> >> Acked-by: Eddie Huang <eddie.huang@mediatek.com> > > I think we misunderstood one another. I think we can drop both the MODULE_DEVICE_TABLE and the MODULE_ALIAS: there is no need for another > driver ID when it is always probed using DT. > That's how I understood but then Eddie said the opposite so I got confused and was waiting for your clarification. I'll re-spin and remove the alias. > Arnd > Best regards,
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c index 06a5c52b292f..46533f11f7fc 100644 --- a/drivers/rtc/rtc-mt6397.c +++ b/drivers/rtc/rtc-mt6397.c @@ -404,6 +404,12 @@ static const struct of_device_id mt6397_rtc_of_match[] = { }; MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); +static const struct platform_device_id mt6397_rtc_id[] = { + {"mt6397-rtc", 0}, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(platform, mt6397_rtc_id); + static struct platform_driver mtk_rtc_driver = { .driver = { .name = "mt6397-rtc", @@ -412,6 +418,7 @@ static struct platform_driver mtk_rtc_driver = { }, .probe = mtk_rtc_probe, .remove = mtk_rtc_remove, + .id_table = mt6397_rtc_id, }; module_platform_driver(mtk_rtc_driver); @@ -419,4 +426,3 @@ module_platform_driver(mtk_rtc_driver); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); -MODULE_ALIAS("platform:mt6397-rtc");
The platform bus_type .match callback attempts to match the platform device name with an entry on the .id_table if provided and fallbacks to match with the driver's name if a table is not provided. Using a platform device ID to match is more explicit, allows the driver to support more than one device and also the MODULE_DEVICE_TABLE macro can be used to export the module aliases information instead of the MODULE_ALIAS. Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- drivers/rtc/rtc-mt6397.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)