extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

[PATCH] extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

Posted by Uwe Kleine-König 1 year, 4 months ago

These drivers don't use the driver_data member of struct i2c_device_id,
so don't explicitly initialize this member.

This prepares putting driver_data in an anonymous union which requires
either no initialization or named designators. But it's also a nice
cleanup on its own.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/extcon/extcon-fsa9480.c | 2 +-
 drivers/extcon/extcon-ptn5150.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/extcon/extcon-fsa9480.c b/drivers/extcon/extcon-fsa9480.c
index e458ce0c45ab..b11b43171063 100644
--- a/drivers/extcon/extcon-fsa9480.c
+++ b/drivers/extcon/extcon-fsa9480.c
@@ -350,7 +350,7 @@ static const struct dev_pm_ops fsa9480_pm_ops = {
 };
 
 static const struct i2c_device_id fsa9480_id[] = {
-	{ "fsa9480", 0 },
+	{ "fsa9480" },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, fsa9480_id);
diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
index 4616da7e5430..78ad86c4a3be 100644
--- a/drivers/extcon/extcon-ptn5150.c
+++ b/drivers/extcon/extcon-ptn5150.c
@@ -338,7 +338,7 @@ static const struct of_device_id ptn5150_dt_match[] = {
 MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
 
 static const struct i2c_device_id ptn5150_i2c_id[] = {
-	{ "ptn5150", 0 },
+	{ "ptn5150" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);

base-commit: 55bcd2e0d04c1171d382badef1def1fd04ef66c5
-- 
2.45.2

Re: [PATCH] extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

Posted by Uwe Kleine-König 1 year, 2 months ago

Hello,

On Wed, Sep 18, 2024 at 02:31:48PM +0200, Uwe Kleine-König wrote:
> These drivers don't use the driver_data member of struct i2c_device_id,
> so don't explicitly initialize this member.
> 
> This prepares putting driver_data in an anonymous union which requires
> either no initialization or named designators. But it's also a nice
> cleanup on its own.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

That patch got some positive feedback by Krzysztof but wasn't applied.
Is this still on someone's radar for application?

Best regards
Uwe

RE: Re: [PATCH] extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

Posted by MyungJoo Ham 1 year, 2 months ago

>Hello,
>
>On Wed, Sep 18, 2024 at 02:31:48PM +0200, Uwe Kleine-König wrote:
>> These drivers don't use the driver_data member of struct i2c_device_id,
>> so don't explicitly initialize this member.
>> 
>> This prepares putting driver_data in an anonymous union which requires
>> either no initialization or named designators. But it's also a nice
>> cleanup on its own.
>> 
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>
>That patch got some positive feedback by Krzysztof but wasn't applied.
>Is this still on someone's radar for application?
>
>Best regards
>Uwe

Can you provide a link to a commit that requires this change?

For now, I don't see any benefit from this change.

Cheers,
MyungJoo

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmdQMQAACgkQj4D7WH0S
/k5Mnwf6A4HpvVzphlte3vo254+4s8jyamJo4jvUclabkK8Pt8lJ72+cJgjGDe4m
AMxS9D1pUSdDWVYrHpgulg+6WTHRqhbi642UKjuzu5crMJ+j6Rzs9FKJHBvHUzE2
lZdhClZ5o9rlp3erNceF53rbwjmmT+IXGrW3dMsCukz8/4YxuzeRYmsxEr6MEWfN
ELZs7gYIvqbXSVtkV+xWqYKuzRJcINhQwOW+RaNgpsdV7uoU/Jm72lFNW87JKQnL
o9PPAxRXH3/kYPSh21MJ+GnPVD5N2seXcJDqPx0PKzOCSMd5iyXfGFI19Gt6H4eE
bdYT3BLaGTV662ONlAfXNDLk7pK3rA==
=/XKL
-----END PGP SIGNATURE-----

Re: [PATCH] extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

Posted by Uwe Kleine-König 1 year, 2 months ago

Hello MyungJoo,

On Thu, Dec 05, 2024 at 12:58:14PM +0900, MyungJoo Ham wrote:
> >On Wed, Sep 18, 2024 at 02:31:48PM +0200, Uwe Kleine-König wrote:
> >> These drivers don't use the driver_data member of struct i2c_device_id,
> >> so don't explicitly initialize this member.
> >> 
> >> This prepares putting driver_data in an anonymous union which requires
> >> either no initialization or named designators. But it's also a nice
> >> cleanup on its own.
> >> 
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> >
> >That patch got some positive feedback by Krzysztof but wasn't applied.
> >Is this still on someone's radar for application?
> 
> Can you provide a link to a commit that requires this change?
> 
> For now, I don't see any benefit from this change.

So the explicit initialisation of .driver_data that then isn't used
doesn't look strange enough to you to drop it?

Would you prefer 

--- a/drivers/extcon/extcon-fsa9480.c
+++ b/drivers/extcon/extcon-fsa9480.c
@@ -350,7 +350,7 @@ static const struct dev_pm_ops fsa9480_pm_ops = {
 };

 static const struct i2c_device_id fsa9480_id[] = {
-	{ "fsa9480", 0 },
+	{ .name = "fsa9480", .driver_data = 0, },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, fsa9480_id);

then?

Anyhow: The most recent presentation of the quest is at
https://lore.kernel.org/linux-iio/20241204150036.1695824-2-u.kleine-koenig@baylibre.com.

For drivers/extcon/extcon-sm5502.c this would allow the following
cleanup:

diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
index 3f8cca528284..3eb8fe4f4af5 100644
--- a/drivers/extcon/extcon-sm5502.c
+++ b/drivers/extcon/extcon-sm5502.c
@@ -827,9 +827,9 @@ static SIMPLE_DEV_PM_OPS(sm5502_muic_pm_ops,
 			 sm5502_muic_suspend, sm5502_muic_resume);
 
 static const struct i2c_device_id sm5502_i2c_id[] = {
-	{ .name = "sm5502", .driver_data = (kernel_ulong_t)&sm5502_data, },
-	{ .name = "sm5504", .driver_data = (kernel_ulong_t)&sm5504_data, },
-	{ .name = "sm5703-muic", .driver_data = (kernel_ulong_t)&sm5502_data, },
+	{ .name = "sm5502", .driver_data_ptr = &sm5502_data, },
+	{ .name = "sm5504", .driver_data_ptr = &sm5504_data, },
+	{ .name = "sm5703-muic", .driver_data_ptr = &sm5502_data, },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, sm5502_i2c_id);

It doesn't benefit from further cleanups because the ugly cast it relies
on is done in i2c_get_match_data(), which can then be modified to:

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7c810893bfa3..a050ead0b7d2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -129,7 +129,7 @@ const void *i2c_get_match_data(const struct i2c_client *client)
 		if (!match)
 			return NULL;
 
-		data = (const void *)match->driver_data;
+		data = match->driver_data_ptr;
 	}
 
 	return data;

Best regards
Uwe

RE: Re: [PATCH] extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

Posted by MyungJoo Ham 1 year, 2 months ago

>Hello MyungJoo,
>
>On Thu, Dec 05, 2024 at 12:58:14PM +0900, MyungJoo Ham wrote:
>> >On Wed, Sep 18, 2024 at 02:31:48PM +0200, Uwe Kleine-König wrote:
>> >> These drivers don't use the driver_data member of struct i2c_device_id,
>> >> so don't explicitly initialize this member.
>> >> 
>> >> This prepares putting driver_data in an anonymous union which requires
>> >> either no initialization or named designators. But it's also a nice
>> >> cleanup on its own.
>> >> 
>> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>> >
>> >That patch got some positive feedback by Krzysztof but wasn't applied.
>> >Is this still on someone's radar for application?
>> 
>> Can you provide a link to a commit that requires this change?
>> 
>> For now, I don't see any benefit from this change.
>
>So the explicit initialisation of .driver_data that then isn't used
>doesn't look strange enough to you to drop it?
>
>Would you prefer 
>
>--- a/drivers/extcon/extcon-fsa9480.c
>+++ b/drivers/extcon/extcon-fsa9480.c
>@@ -350,7 +350,7 @@ static const struct dev_pm_ops fsa9480_pm_ops = {
> };
>
> static const struct i2c_device_id fsa9480_id[] = {
>-	{ "fsa9480", 0 },
>+	{ .name = "fsa9480", .driver_data = 0, },
> 	{}
> };
> MODULE_DEVICE_TABLE(i2c, fsa9480_id);
>
>then?
>
>Anyhow: The most recent presentation of the quest is at
>https://lore.kernel.org/linux-iio/20241204150036.1695824-2-u.kleine-koenig@baylibre.com.

Hi,

That link explains it.

It'd have been easier to see if that link or the pending changes on i2c_device_id
were mentioned.

Chanwoo: please take a look.

Cheers,
MyungJoo


(For the original "extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0")
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

Re: [PATCH] extcon: Drop explicit initialization of struct i2c_device_id::driver_data to 0

Posted by Krzysztof Kozlowski 1 year, 4 months ago

On 18/09/2024 14:31, Uwe Kleine-König wrote:
> These drivers don't use the driver_data member of struct i2c_device_id,
> so don't explicitly initialize this member.
> 
> This prepares putting driver_data in an anonymous union which requires
> either no initialization or named designators. But it's also a nice
> cleanup on its own.


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof