| Message ID | 20240125145007.748295-8-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: s3c64xx: winter cleanup and gs101 support | expand |
On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > of_device_id::data is an opaque pointer. No explicit cast is needed. > Remove unneeded (void *) casts in of_match_table. While here align the > compatible and data members. > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++---------------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 230fda2b3417..137faf9f2697 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = { > }; > > static const struct of_device_id s3c64xx_spi_dt_match[] = { > - { .compatible = "samsung,s3c2443-spi", > - .data = (void *)&s3c2443_spi_port_config, I support removing (void *) cast. But this new braces style: }, { seems to bloat the code a bit. For my taste, having something like }, { on the same line would be more compact, and more canonical so to speak. Or even preserving the existing style would be ok too, for that matter. Assuming the braces style is fixed, you can add: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > + { > + .compatible = "samsung,s3c2443-spi", > + .data = &s3c2443_spi_port_config, > }, > - { .compatible = "samsung,s3c6410-spi", > - .data = (void *)&s3c6410_spi_port_config, > + { > + .compatible = "samsung,s3c6410-spi", > + .data = &s3c6410_spi_port_config, > }, > - { .compatible = "samsung,s5pv210-spi", > - .data = (void *)&s5pv210_spi_port_config, > + { > + .compatible = "samsung,s5pv210-spi", > + .data = &s5pv210_spi_port_config, > }, > - { .compatible = "samsung,exynos4210-spi", > - .data = (void *)&exynos4_spi_port_config, > + { > + .compatible = "samsung,exynos4210-spi", > + .data = &exynos4_spi_port_config, > }, > - { .compatible = "samsung,exynos7-spi", > - .data = (void *)&exynos7_spi_port_config, > + { > + .compatible = "samsung,exynos7-spi", > + .data = &exynos7_spi_port_config, > }, > - { .compatible = "samsung,exynos5433-spi", > - .data = (void *)&exynos5433_spi_port_config, > + { > + .compatible = "samsung,exynos5433-spi", > + .data = &exynos5433_spi_port_config, > }, > - { .compatible = "samsung,exynos850-spi", > - .data = (void *)&exynos850_spi_port_config, > + { > + .compatible = "samsung,exynos850-spi", > + .data = &exynos850_spi_port_config, > }, > - { .compatible = "samsung,exynosautov9-spi", > - .data = (void *)&exynosautov9_spi_port_config, > + { > + .compatible = "samsung,exynosautov9-spi", > + .data = &exynosautov9_spi_port_config, > }, > - { .compatible = "tesla,fsd-spi", > - .data = (void *)&fsd_spi_port_config, > + { > + .compatible = "tesla,fsd-spi", > + .data = &fsd_spi_port_config, > }, > { }, > }; > -- > 2.43.0.429.g432eaa2c6b-goog >
Thanks for the review feedback, Sam, great catches so far! On 1/25/24 19:04, Sam Protsenko wrote: > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> of_device_id::data is an opaque pointer. No explicit cast is needed. >> Remove unneeded (void *) casts in of_match_table. While here align the >> compatible and data members. >> >> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++---------------- >> 1 file changed, 27 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 230fda2b3417..137faf9f2697 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = { >> }; >> >> static const struct of_device_id s3c64xx_spi_dt_match[] = { >> - { .compatible = "samsung,s3c2443-spi", >> - .data = (void *)&s3c2443_spi_port_config, > > I support removing (void *) cast. But this new braces style: > > }, > { this style was there before my patch. > > seems to bloat the code a bit. For my taste, having something like }, > { on the same line would be more compact, and more canonical so to I don't lean towards neither of the styles, I'm ok with both > speak. Or even preserving the existing style would be ok too, for that > matter. > seeing .compatible and .data unaligned hurt my eyes and I think that aligning them while dropping the cast is fine. I don't really want to do the style change unless you, Andi or Mark insist. Would you please come with a patch on top if you really want them changed? > Assuming the braces style is fixed, you can add: > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > >> + { >> + .compatible = "samsung,s3c2443-spi", >> + .data = &s3c2443_spi_port_config, >> }, >> - { .compatible = "samsung,s3c6410-spi", >> - .data = (void *)&s3c6410_spi_port_config, >> + { >> + .compatible = "samsung,s3c6410-spi", >> + .data = &s3c6410_spi_port_config, >> }, >> - { .compatible = "samsung,s5pv210-spi", >> - .data = (void *)&s5pv210_spi_port_config, >> + { >> + .compatible = "samsung,s5pv210-spi", >> + .data = &s5pv210_spi_port_config, >> }, >> - { .compatible = "samsung,exynos4210-spi", >> - .data = (void *)&exynos4_spi_port_config, >> + { >> + .compatible = "samsung,exynos4210-spi", >> + .data = &exynos4_spi_port_config, >> }, >> - { .compatible = "samsung,exynos7-spi", >> - .data = (void *)&exynos7_spi_port_config, >> + { >> + .compatible = "samsung,exynos7-spi", >> + .data = &exynos7_spi_port_config, >> }, >> - { .compatible = "samsung,exynos5433-spi", >> - .data = (void *)&exynos5433_spi_port_config, >> + { >> + .compatible = "samsung,exynos5433-spi", >> + .data = &exynos5433_spi_port_config, >> }, >> - { .compatible = "samsung,exynos850-spi", >> - .data = (void *)&exynos850_spi_port_config, >> + { >> + .compatible = "samsung,exynos850-spi", >> + .data = &exynos850_spi_port_config, >> }, >> - { .compatible = "samsung,exynosautov9-spi", >> - .data = (void *)&exynosautov9_spi_port_config, >> + { >> + .compatible = "samsung,exynosautov9-spi", >> + .data = &exynosautov9_spi_port_config, >> }, >> - { .compatible = "tesla,fsd-spi", >> - .data = (void *)&fsd_spi_port_config, >> + { >> + .compatible = "tesla,fsd-spi", >> + .data = &fsd_spi_port_config, >> }, >> { }, >> }; >> -- >> 2.43.0.429.g432eaa2c6b-goog >>
On Fri, Jan 26, 2024 at 2:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Thanks for the review feedback, Sam, great catches so far! > > On 1/25/24 19:04, Sam Protsenko wrote: > > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> of_device_id::data is an opaque pointer. No explicit cast is needed. > >> Remove unneeded (void *) casts in of_match_table. While here align the > >> compatible and data members. > >> > >> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> --- > >> drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++---------------- > >> 1 file changed, 27 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > >> index 230fda2b3417..137faf9f2697 100644 > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = { > >> }; > >> > >> static const struct of_device_id s3c64xx_spi_dt_match[] = { > >> - { .compatible = "samsung,s3c2443-spi", > >> - .data = (void *)&s3c2443_spi_port_config, > > > > I support removing (void *) cast. But this new braces style: > > > > }, > > { > > this style was there before my patch. > > > > seems to bloat the code a bit. For my taste, having something like }, > > { on the same line would be more compact, and more canonical so to > > I don't lean towards neither of the styles, I'm ok with both > > > speak. Or even preserving the existing style would be ok too, for that > > matter. > > > > seeing .compatible and .data unaligned hurt my eyes and I think that > aligning them while dropping the cast is fine. I don't really want to do > the style change unless you, Andi or Mark insist. Would you please come > with a patch on top if you really want them changed? > But that would completely undermine the whole point of the review? I'd prefer this style: ... = { { .compatible = .data = }, { .compatible = .data = }, { /* sentinel */ }, }; That seems more canonical to me, and more compact too, with no contradictions to your preference about alignment too. But that's only my opinion, as a reviewer. > > Assuming the braces style is fixed, you can add: > > > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > > > >> + { > >> + .compatible = "samsung,s3c2443-spi", > >> + .data = &s3c2443_spi_port_config, > >> }, > >> - { .compatible = "samsung,s3c6410-spi", > >> - .data = (void *)&s3c6410_spi_port_config, > >> + { > >> + .compatible = "samsung,s3c6410-spi", > >> + .data = &s3c6410_spi_port_config, > >> }, > >> - { .compatible = "samsung,s5pv210-spi", > >> - .data = (void *)&s5pv210_spi_port_config, > >> + { > >> + .compatible = "samsung,s5pv210-spi", > >> + .data = &s5pv210_spi_port_config, > >> }, > >> - { .compatible = "samsung,exynos4210-spi", > >> - .data = (void *)&exynos4_spi_port_config, > >> + { > >> + .compatible = "samsung,exynos4210-spi", > >> + .data = &exynos4_spi_port_config, > >> }, > >> - { .compatible = "samsung,exynos7-spi", > >> - .data = (void *)&exynos7_spi_port_config, > >> + { > >> + .compatible = "samsung,exynos7-spi", > >> + .data = &exynos7_spi_port_config, > >> }, > >> - { .compatible = "samsung,exynos5433-spi", > >> - .data = (void *)&exynos5433_spi_port_config, > >> + { > >> + .compatible = "samsung,exynos5433-spi", > >> + .data = &exynos5433_spi_port_config, > >> }, > >> - { .compatible = "samsung,exynos850-spi", > >> - .data = (void *)&exynos850_spi_port_config, > >> + { > >> + .compatible = "samsung,exynos850-spi", > >> + .data = &exynos850_spi_port_config, > >> }, > >> - { .compatible = "samsung,exynosautov9-spi", > >> - .data = (void *)&exynosautov9_spi_port_config, > >> + { > >> + .compatible = "samsung,exynosautov9-spi", > >> + .data = &exynosautov9_spi_port_config, > >> }, > >> - { .compatible = "tesla,fsd-spi", > >> - .data = (void *)&fsd_spi_port_config, > >> + { > >> + .compatible = "tesla,fsd-spi", > >> + .data = &fsd_spi_port_config, > >> }, > >> { }, > >> }; > >> -- > >> 2.43.0.429.g432eaa2c6b-goog > >>
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 230fda2b3417..137faf9f2697 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = { }; static const struct of_device_id s3c64xx_spi_dt_match[] = { - { .compatible = "samsung,s3c2443-spi", - .data = (void *)&s3c2443_spi_port_config, + { + .compatible = "samsung,s3c2443-spi", + .data = &s3c2443_spi_port_config, }, - { .compatible = "samsung,s3c6410-spi", - .data = (void *)&s3c6410_spi_port_config, + { + .compatible = "samsung,s3c6410-spi", + .data = &s3c6410_spi_port_config, }, - { .compatible = "samsung,s5pv210-spi", - .data = (void *)&s5pv210_spi_port_config, + { + .compatible = "samsung,s5pv210-spi", + .data = &s5pv210_spi_port_config, }, - { .compatible = "samsung,exynos4210-spi", - .data = (void *)&exynos4_spi_port_config, + { + .compatible = "samsung,exynos4210-spi", + .data = &exynos4_spi_port_config, }, - { .compatible = "samsung,exynos7-spi", - .data = (void *)&exynos7_spi_port_config, + { + .compatible = "samsung,exynos7-spi", + .data = &exynos7_spi_port_config, }, - { .compatible = "samsung,exynos5433-spi", - .data = (void *)&exynos5433_spi_port_config, + { + .compatible = "samsung,exynos5433-spi", + .data = &exynos5433_spi_port_config, }, - { .compatible = "samsung,exynos850-spi", - .data = (void *)&exynos850_spi_port_config, + { + .compatible = "samsung,exynos850-spi", + .data = &exynos850_spi_port_config, }, - { .compatible = "samsung,exynosautov9-spi", - .data = (void *)&exynosautov9_spi_port_config, + { + .compatible = "samsung,exynosautov9-spi", + .data = &exynosautov9_spi_port_config, }, - { .compatible = "tesla,fsd-spi", - .data = (void *)&fsd_spi_port_config, + { + .compatible = "tesla,fsd-spi", + .data = &fsd_spi_port_config, }, { }, };