| Message ID | 20240125145007.748295-8-tudor.ambarus@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/28] spi: s3c64xx: explicitly include <linux/io.h> | expand |
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, }, { }, };