| Message ID | 20170313190726.2988-3-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Monday, March 13, 2017 09:07:25 PM Krzysztof Kozlowski wrote: > Simplify the flow in helper function for getting the driver data by > using of_device_get_match_data() and only one if() branch. > > The code should be equivalent. While you are at it could you remove s3c2410_get_wdt_drv_data() helper? It is used only once during probe and is marked inline anyway.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Tue, Mar 14, 2017 at 3:17 PM, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > Hi, > > On Monday, March 13, 2017 09:07:25 PM Krzysztof Kozlowski wrote: >> Simplify the flow in helper function for getting the driver data by >> using of_device_get_match_data() and only one if() branch. >> >> The code should be equivalent. > > While you are at it could you remove s3c2410_get_wdt_drv_data() > helper? It is used only once during probe and is marked inline > anyway.. Thanks for feedback! The existence of this helper is purely from code readability (thus inline does not matter). The probe is big so splitting some small self-contained part helps. Not much but a little... Best regards, Krzysztof
On 03/14/2017 06:20 AM, Krzysztof Kozlowski wrote: > On Tue, Mar 14, 2017 at 3:17 PM, Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: >> >> Hi, >> >> On Monday, March 13, 2017 09:07:25 PM Krzysztof Kozlowski wrote: >>> Simplify the flow in helper function for getting the driver data by >>> using of_device_get_match_data() and only one if() branch. >>> >>> The code should be equivalent. >> >> While you are at it could you remove s3c2410_get_wdt_drv_data() >> helper? It is used only once during probe and is marked inline >> anyway.. > > Thanks for feedback! > The existence of this helper is purely from code readability (thus > inline does not matter). The probe is big so splitting some small > self-contained part helps. Not much but a little... > Agreed. I don't see value in removing this helper. Guenter
On 03/13/2017 12:07 PM, Krzysztof Kozlowski wrote: > Simplify the flow in helper function for getting the driver data by > using of_device_get_match_data() and only one if() branch. > > The code should be equivalent. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/s3c2410_wdt.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 52b66bcdd1ef..0532dc93e600 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -37,6 +37,7 @@ > #include <linux/slab.h> > #include <linux/err.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > #include <linux/delay.h> > @@ -510,14 +511,16 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt) > static inline const struct s3c2410_wdt_variant * > s3c2410_get_wdt_drv_data(struct platform_device *pdev) > { > - if (pdev->dev.of_node) { > - const struct of_device_id *match; > - match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node); > - return (struct s3c2410_wdt_variant *)match->data; > - } else { > - return (struct s3c2410_wdt_variant *) > - platform_get_device_id(pdev)->driver_data; > + const struct s3c2410_wdt_variant *variant; > + > + variant = of_device_get_match_data(&pdev->dev); > + if (!variant) { > + /* Device matched by platform_device_id */ > + variant = (struct s3c2410_wdt_variant *) > + platform_get_device_id(pdev)->driver_data; > } > + > + return variant; > } > > static int s3c2410wdt_probe(struct platform_device *pdev) >
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index 52b66bcdd1ef..0532dc93e600 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -37,6 +37,7 @@ #include <linux/slab.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> #include <linux/delay.h> @@ -510,14 +511,16 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt) static inline const struct s3c2410_wdt_variant * s3c2410_get_wdt_drv_data(struct platform_device *pdev) { - if (pdev->dev.of_node) { - const struct of_device_id *match; - match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node); - return (struct s3c2410_wdt_variant *)match->data; - } else { - return (struct s3c2410_wdt_variant *) - platform_get_device_id(pdev)->driver_data; + const struct s3c2410_wdt_variant *variant; + + variant = of_device_get_match_data(&pdev->dev); + if (!variant) { + /* Device matched by platform_device_id */ + variant = (struct s3c2410_wdt_variant *) + platform_get_device_id(pdev)->driver_data; } + + return variant; } static int s3c2410wdt_probe(struct platform_device *pdev)
Simplify the flow in helper function for getting the driver data by using of_device_get_match_data() and only one if() branch. The code should be equivalent. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/watchdog/s3c2410_wdt.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)