| Message ID | 20210419081726.67867-5-krzysztof.kozlowski@canonical.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/7] mfd: max8997: Simplify getting of_device_id match data | expand |
On 19.04.2021 10:17, Krzysztof Kozlowski wrote: > Use of_device_get_match_data() to make the code slightly smaller. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > drivers/mfd/sec-core.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c > index 8d55992da19e..3126c39f3203 100644 > --- a/drivers/mfd/sec-core.c > +++ b/drivers/mfd/sec-core.c > @@ -10,6 +10,7 @@ > #include <linux/slab.h> > #include <linux/i2c.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_irq.h> > #include <linux/interrupt.h> > #include <linux/pm_runtime.h> > @@ -324,12 +325,8 @@ static inline unsigned long sec_i2c_get_driver_data(struct i2c_client *i2c, > const struct i2c_device_id *id) > { > #ifdef CONFIG_OF > - if (i2c->dev.of_node) { > - const struct of_device_id *match; > - > - match = of_match_node(sec_dt_match, i2c->dev.of_node); > - return (unsigned long)match->data; > - } > + if (i2c->dev.of_node) > + return (unsigned long)of_device_get_match_data(&i2c->dev); > #endif Does it make any sense to keep the #ifdef CONFIG_OF after this change? I would also skip (i2c->dev.of_node) check, because of_device_get_match_data() already does that (although indirectly). > return id->driver_data; > } Best regards
On 20/04/2021 07:25, Marek Szyprowski wrote: > > On 19.04.2021 10:17, Krzysztof Kozlowski wrote: >> Use of_device_get_match_data() to make the code slightly smaller. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >> --- >> drivers/mfd/sec-core.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c >> index 8d55992da19e..3126c39f3203 100644 >> --- a/drivers/mfd/sec-core.c >> +++ b/drivers/mfd/sec-core.c >> @@ -10,6 +10,7 @@ >> #include <linux/slab.h> >> #include <linux/i2c.h> >> #include <linux/of.h> >> +#include <linux/of_device.h> >> #include <linux/of_irq.h> >> #include <linux/interrupt.h> >> #include <linux/pm_runtime.h> >> @@ -324,12 +325,8 @@ static inline unsigned long sec_i2c_get_driver_data(struct i2c_client *i2c, >> const struct i2c_device_id *id) >> { >> #ifdef CONFIG_OF >> - if (i2c->dev.of_node) { >> - const struct of_device_id *match; >> - >> - match = of_match_node(sec_dt_match, i2c->dev.of_node); >> - return (unsigned long)match->data; >> - } >> + if (i2c->dev.of_node) >> + return (unsigned long)of_device_get_match_data(&i2c->dev); >> #endif > > Does it make any sense to keep the #ifdef CONFIG_OF after this change? Good point, it was only to hide usage of of_device_id table. > I would also skip (i2c->dev.of_node) check, because > of_device_get_match_data() already does that (although indirectly). First, the enum sec_device_type would need to be changed so it starts from 1, not 0. It's because the value returned by this function is later assigned to that enum and there is no way currently to differentiate between NULL and S5M8767X. Second, it wouldn't make the code smaller; unsigned long data; data = of_device_get_match_data(&i2c->dev); if (data) return data; Best regards, Krzysztof
On 20.04.2021 09:03, Krzysztof Kozlowski wrote: > On 20/04/2021 07:25, Marek Szyprowski wrote: >> On 19.04.2021 10:17, Krzysztof Kozlowski wrote: >>> Use of_device_get_match_data() to make the code slightly smaller. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>> --- >>> drivers/mfd/sec-core.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c >>> index 8d55992da19e..3126c39f3203 100644 >>> --- a/drivers/mfd/sec-core.c >>> +++ b/drivers/mfd/sec-core.c >>> @@ -10,6 +10,7 @@ >>> #include <linux/slab.h> >>> #include <linux/i2c.h> >>> #include <linux/of.h> >>> +#include <linux/of_device.h> >>> #include <linux/of_irq.h> >>> #include <linux/interrupt.h> >>> #include <linux/pm_runtime.h> >>> @@ -324,12 +325,8 @@ static inline unsigned long sec_i2c_get_driver_data(struct i2c_client *i2c, >>> const struct i2c_device_id *id) >>> { >>> #ifdef CONFIG_OF >>> - if (i2c->dev.of_node) { >>> - const struct of_device_id *match; >>> - >>> - match = of_match_node(sec_dt_match, i2c->dev.of_node); >>> - return (unsigned long)match->data; >>> - } >>> + if (i2c->dev.of_node) >>> + return (unsigned long)of_device_get_match_data(&i2c->dev); >>> #endif >> Does it make any sense to keep the #ifdef CONFIG_OF after this change? > Good point, it was only to hide usage of of_device_id table. > >> I would also skip (i2c->dev.of_node) check, because >> of_device_get_match_data() already does that (although indirectly). > First, the enum sec_device_type would need to be changed so it starts > from 1, not 0. It's because the value returned by this function is later > assigned to that enum and there is no way currently to differentiate > between NULL and S5M8767X. > > Second, it wouldn't make the code smaller; > > unsigned long data; > data = of_device_get_match_data(&i2c->dev); > if (data) > return data; Then maybe one should go further and remove legacy, non-of based initialization, because it is not used at all. This will simplify it even more. Best regards
On 20/04/2021 09:12, Marek Szyprowski wrote: > On 20.04.2021 09:03, Krzysztof Kozlowski wrote: >> On 20/04/2021 07:25, Marek Szyprowski wrote: >>> On 19.04.2021 10:17, Krzysztof Kozlowski wrote: >>>> Use of_device_get_match_data() to make the code slightly smaller. >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>>> --- >>>> drivers/mfd/sec-core.c | 9 +++------ >>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c >>>> index 8d55992da19e..3126c39f3203 100644 >>>> --- a/drivers/mfd/sec-core.c >>>> +++ b/drivers/mfd/sec-core.c >>>> @@ -10,6 +10,7 @@ >>>> #include <linux/slab.h> >>>> #include <linux/i2c.h> >>>> #include <linux/of.h> >>>> +#include <linux/of_device.h> >>>> #include <linux/of_irq.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/pm_runtime.h> >>>> @@ -324,12 +325,8 @@ static inline unsigned long sec_i2c_get_driver_data(struct i2c_client *i2c, >>>> const struct i2c_device_id *id) >>>> { >>>> #ifdef CONFIG_OF >>>> - if (i2c->dev.of_node) { >>>> - const struct of_device_id *match; >>>> - >>>> - match = of_match_node(sec_dt_match, i2c->dev.of_node); >>>> - return (unsigned long)match->data; >>>> - } >>>> + if (i2c->dev.of_node) >>>> + return (unsigned long)of_device_get_match_data(&i2c->dev); >>>> #endif >>> Does it make any sense to keep the #ifdef CONFIG_OF after this change? >> Good point, it was only to hide usage of of_device_id table. >> >>> I would also skip (i2c->dev.of_node) check, because >>> of_device_get_match_data() already does that (although indirectly). >> First, the enum sec_device_type would need to be changed so it starts >> from 1, not 0. It's because the value returned by this function is later >> assigned to that enum and there is no way currently to differentiate >> between NULL and S5M8767X. >> >> Second, it wouldn't make the code smaller; >> >> unsigned long data; >> data = of_device_get_match_data(&i2c->dev); >> if (data) >> return data; > > Then maybe one should go further and remove legacy, non-of based > initialization, because it is not used at all. This will simplify it > even more. Indeed maybe it's the time to get rid of board-file support... Thanks for the feedback! Best regards, Krzysztof
diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c index 8d55992da19e..3126c39f3203 100644 --- a/drivers/mfd/sec-core.c +++ b/drivers/mfd/sec-core.c @@ -10,6 +10,7 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/interrupt.h> #include <linux/pm_runtime.h> @@ -324,12 +325,8 @@ static inline unsigned long sec_i2c_get_driver_data(struct i2c_client *i2c, const struct i2c_device_id *id) { #ifdef CONFIG_OF - if (i2c->dev.of_node) { - const struct of_device_id *match; - - match = of_match_node(sec_dt_match, i2c->dev.of_node); - return (unsigned long)match->data; - } + if (i2c->dev.of_node) + return (unsigned long)of_device_get_match_data(&i2c->dev); #endif return id->driver_data; }
Use of_device_get_match_data() to make the code slightly smaller. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> --- drivers/mfd/sec-core.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)