diff mbox series

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

Message ID 20240125145007.748295-8-tudor.ambarus@linaro.org (mailing list archive)
State New
Headers show
Series spi: s3c64xx: winter cleanup and gs101 support | expand

Commit Message

Tudor Ambarus Jan. 25, 2024, 2:49 p.m. UTC
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(-)

Comments

Sam Protsenko Jan. 25, 2024, 7:04 p.m. UTC | #1
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
>
Tudor Ambarus Jan. 26, 2024, 8:24 a.m. UTC | #2
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
>>
Sam Protsenko Jan. 26, 2024, 7:40 p.m. UTC | #3
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 mbox series

Patch

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,
 	},
 	{ },
 };