2024-01-25 14:52:56

by Tudor Ambarus

Subject: [PATCH v2 07/28] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

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 <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
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,
+ {
+ .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

2024-01-25 19:04:46

by Sam Protsenko

Subject: Re: [PATCH v2 07/28] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaroorg> 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 <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> 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 <[email protected]>

> + {
> + .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
>

2024-01-26 09:16:17

by Tudor Ambarus

Subject: Re: [PATCH v2 07/28] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> 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 <[email protected]>
>
>> + {
>> + .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
>>

2024-01-26 19:40:59

by Sam Protsenko

Subject: Re: [PATCH v2 07/28] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

On Fri, Jan 26, 2024 at 2:24 AM Tudor Ambarus <tudor.ambarus@linaroorg> 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 <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Tudor Ambarus <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >
> >> + {
> >> + .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
> >>