| Message ID | 1482495889-6201-5-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote: > Replace custom code with generic helper. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c > index 4d9262051ff1..a6c2ea74e0f3 100644 > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c > @@ -27,6 +27,7 @@ > #include <linux/err.h> > #include <linux/gpio.h> > #include <linux/irqdomain.h> > +#include <linux/of_device.h> > #include <linux/spinlock.h> > #include <linux/syscore_ops.h> > > @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, > return 0; > } > > -static const struct of_device_id samsung_pinctrl_dt_match[]; > - > /* retrieve the soc specific data */ > static const struct samsung_pin_ctrl * > samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d, > struct platform_device *pdev) > { > int id; > - const struct of_device_id *match; > + const struct samsung_pin_ctrl *match_data; > struct device_node *node = pdev->dev.of_node; > struct device_node *np; > const struct samsung_pin_bank_data *bdata; > @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, > dev_err(&pdev->dev, "failed to get alias id\n"); > return ERR_PTR(-ENOENT); > } > - match = of_match_node(samsung_pinctrl_dt_match, node); > - ctrl = (struct samsung_pin_ctrl *)match->data + id; > + match_data = of_device_get_match_data(&pdev->dev); > + ctrl = match_data + id; Either you need a check for match_data != NULL or just remove match_data and: ctrl = of_device_get_match_data(); ctrl += id; Actually match_data can be removed even with the check for non-NULL... Best regards, Krzysztof > > d->suspend = ctrl->suspend; > d->resume = ctrl->resume; > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-25 21:56 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote: >> Replace custom code with generic helper. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c >> index 4d9262051ff1..a6c2ea74e0f3 100644 >> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c >> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c >> @@ -27,6 +27,7 @@ >> #include <linux/err.h> >> #include <linux/gpio.h> >> #include <linux/irqdomain.h> >> +#include <linux/of_device.h> >> #include <linux/spinlock.h> >> #include <linux/syscore_ops.h> >> >> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, >> return 0; >> } >> >> -static const struct of_device_id samsung_pinctrl_dt_match[]; >> - >> /* retrieve the soc specific data */ >> static const struct samsung_pin_ctrl * >> samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d, >> struct platform_device *pdev) >> { >> int id; >> - const struct of_device_id *match; >> + const struct samsung_pin_ctrl *match_data; >> struct device_node *node = pdev->dev.of_node; >> struct device_node *np; >> const struct samsung_pin_bank_data *bdata; >> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, >> dev_err(&pdev->dev, "failed to get alias id\n"); >> return ERR_PTR(-ENOENT); >> } >> - match = of_match_node(samsung_pinctrl_dt_match, node); >> - ctrl = (struct samsung_pin_ctrl *)match->data + id; >> + match_data = of_device_get_match_data(&pdev->dev); >> + ctrl = match_data + id; > > Either you need a check for match_data != NULL or just remove match_data > and: > ctrl = of_device_get_match_data(); > ctrl += id; > > Actually match_data can be removed even with the check for non-NULL... I don't think this function can ever return NULL if the match array contains a non-NULL value for each compatible string and the driver can be probed only by DT. But you still need to cast the match data pointer to the correct type and using a variable for it IMHO makes the code cleaner. Acked-by: Tomasz Figa <tomasz.figa@gmail.com> Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 26, 2016 at 02:44:26PM +0900, Tomasz Figa wrote: > 2016-12-25 21:56 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > > On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote: > >> Replace custom code with generic helper. > >> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++----- > >> 1 file changed, 4 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c > >> index 4d9262051ff1..a6c2ea74e0f3 100644 > >> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c > >> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c > >> @@ -27,6 +27,7 @@ > >> #include <linux/err.h> > >> #include <linux/gpio.h> > >> #include <linux/irqdomain.h> > >> +#include <linux/of_device.h> > >> #include <linux/spinlock.h> > >> #include <linux/syscore_ops.h> > >> > >> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, > >> return 0; > >> } > >> > >> -static const struct of_device_id samsung_pinctrl_dt_match[]; > >> - > >> /* retrieve the soc specific data */ > >> static const struct samsung_pin_ctrl * > >> samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d, > >> struct platform_device *pdev) > >> { > >> int id; > >> - const struct of_device_id *match; > >> + const struct samsung_pin_ctrl *match_data; > >> struct device_node *node = pdev->dev.of_node; > >> struct device_node *np; > >> const struct samsung_pin_bank_data *bdata; > >> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, > >> dev_err(&pdev->dev, "failed to get alias id\n"); > >> return ERR_PTR(-ENOENT); > >> } > >> - match = of_match_node(samsung_pinctrl_dt_match, node); > >> - ctrl = (struct samsung_pin_ctrl *)match->data + id; > >> + match_data = of_device_get_match_data(&pdev->dev); > >> + ctrl = match_data + id; > > > > Either you need a check for match_data != NULL or just remove match_data > > and: > > ctrl = of_device_get_match_data(); > > ctrl += id; > > > > Actually match_data can be removed even with the check for non-NULL... > > I don't think this function can ever return NULL if the match array > contains a non-NULL value for each compatible string and the driver > can be probed only by DT. Practically it cannot (or it should not) but defensive coding is a good practice... > But you still need to cast the match data pointer to the correct type > and using a variable for it IMHO makes the code cleaner. What do you mean by casting through variable? match_data and ctrl are the same type so there is no change by intermediate variable. It just obfuscates the code. BR, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Monday, December 26, 2016 11:41:42 AM Krzysztof Kozlowski wrote: > On Mon, Dec 26, 2016 at 02:44:26PM +0900, Tomasz Figa wrote: > > 2016-12-25 21:56 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > > > On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote: > > >> Replace custom code with generic helper. > > >> > > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > >> --- > > >> drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++----- > > >> 1 file changed, 4 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c > > >> index 4d9262051ff1..a6c2ea74e0f3 100644 > > >> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c > > >> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c > > >> @@ -27,6 +27,7 @@ > > >> #include <linux/err.h> > > >> #include <linux/gpio.h> > > >> #include <linux/irqdomain.h> > > >> +#include <linux/of_device.h> > > >> #include <linux/spinlock.h> > > >> #include <linux/syscore_ops.h> > > >> > > >> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, > > >> return 0; > > >> } > > >> > > >> -static const struct of_device_id samsung_pinctrl_dt_match[]; > > >> - > > >> /* retrieve the soc specific data */ > > >> static const struct samsung_pin_ctrl * > > >> samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d, > > >> struct platform_device *pdev) > > >> { > > >> int id; > > >> - const struct of_device_id *match; > > >> + const struct samsung_pin_ctrl *match_data; > > >> struct device_node *node = pdev->dev.of_node; > > >> struct device_node *np; > > >> const struct samsung_pin_bank_data *bdata; > > >> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, > > >> dev_err(&pdev->dev, "failed to get alias id\n"); > > >> return ERR_PTR(-ENOENT); > > >> } > > >> - match = of_match_node(samsung_pinctrl_dt_match, node); > > >> - ctrl = (struct samsung_pin_ctrl *)match->data + id; > > >> + match_data = of_device_get_match_data(&pdev->dev); > > >> + ctrl = match_data + id; > > > > > > Either you need a check for match_data != NULL or just remove match_data > > > and: > > > ctrl = of_device_get_match_data(); > > > ctrl += id; > > > > > > Actually match_data can be removed even with the check for non-NULL... > > > > I don't think this function can ever return NULL if the match array > > contains a non-NULL value for each compatible string and the driver > > can be probed only by DT. > > Practically it cannot (or it should not) but defensive coding is a good > practice... Defensive coding is not a good practice, please don't recommend it. It usually just obfuscates code and hides underlying issue. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c index 4d9262051ff1..a6c2ea74e0f3 100644 --- a/drivers/pinctrl/samsung/pinctrl-samsung.c +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c @@ -27,6 +27,7 @@ #include <linux/err.h> #include <linux/gpio.h> #include <linux/irqdomain.h> +#include <linux/of_device.h> #include <linux/spinlock.h> #include <linux/syscore_ops.h> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, return 0; } -static const struct of_device_id samsung_pinctrl_dt_match[]; - /* retrieve the soc specific data */ static const struct samsung_pin_ctrl * samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d, struct platform_device *pdev) { int id; - const struct of_device_id *match; + const struct samsung_pin_ctrl *match_data; struct device_node *node = pdev->dev.of_node; struct device_node *np; const struct samsung_pin_bank_data *bdata; @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev, dev_err(&pdev->dev, "failed to get alias id\n"); return ERR_PTR(-ENOENT); } - match = of_match_node(samsung_pinctrl_dt_match, node); - ctrl = (struct samsung_pin_ctrl *)match->data + id; + match_data = of_device_get_match_data(&pdev->dev); + ctrl = match_data + id; d->suspend = ctrl->suspend; d->resume = ctrl->resume;
Replace custom code with generic helper. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)