Re: [PATCH 3/3] i2c: rcar: Fix Wvoid-pointer-to-enum-cast warning

[Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>]

On 27/11/2025 13:52, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Thu, 27 Nov 2025 at 12:48, Krzysztof Kozlowski
> <krzysztof.kozlowski@oss.qualcomm.com> wrote:
>> On 27/11/2025 12:42, Krzysztof Kozlowski wrote:
>>>>> --- a/drivers/i2c/busses/i2c-rcar.c
>>>>> +++ b/drivers/i2c/busses/i2c-rcar.c
>>>>> @@ -1141,7 +1141,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>>>>>         if (IS_ERR(priv->io))
>>>>>                 return PTR_ERR(priv->io);
>>>>>
>>>>> -       priv->devtype = (enum rcar_i2c_type)of_device_get_match_data(dev);
>>>>> +       priv->devtype = (kernel_ulong_t)of_device_get_match_data(dev);
>>>>
>>>> Any specific reason you picked "kernel_ulong_t" instead of "unsigned long"?
>>>> The former seems to be the least common option.
>>>
>>> As I wrote in the first patch, because to my knowledge it is the
> 
> I don't see that written in the first patch; it has the same patch description
> as the other patches in the series (mutatis mutandis)?

https://lore.kernel.org/all/20251126182257.157439-4-krzysztof.kozlowski@oss.qualcomm.com/

I meant that brief statement in the patch changelog.

> 
>>> preferred form for holding driver data which are in general pointers. We
>>> do not store pointers as unsigned long. It is also already used for the
>>> driver data types - see include/linux/mod_devicetable.h.
>>
>> ... and in case it is still not obvious: I do not cast here to final
>> type, because that cast is the reason for the warning.
>>
>> I cast to an intermediary type to help compiler suppressing the warning
> 
> I know...
> 
>> - to integer representing the pointer. Unsigned long is not representing
>> pointers in the kernel. Integer type representing the pointer is
> 
> "unsigned long" indeed does not represent a pointer, but in the Linux
> kernel, it is guaranteed to be the same size as a pointer.
> 
> '"unsigned long" is superior'
> https://lore.kernel.org/CAHk-=wj2OHy-5e+srG1fy+ZU00TmZ1NFp6kFLbVLMXHe7A1d-g@mail.gmail.com
> (he doesn't seem to have said anything about kernel_ulong_t)

Yes, that was again comparing to uintptr_t, so we both agree.

> 
>> kernel_ulong_t, I think.
> 
> include/linux/mod_devicetable.h:typedef unsigned long kernel_ulong_t;
> 
> IIRC, it was introduced originally because "unsigned long" might have
> a different size in userspace.  Nowadays (for x32), uapi uses
> __kernel_ulong_t, though.

Maybe, but if you look at the data structures all have kernel_ulong_t,
so this fits the logic I was following here - I cast to the type which
is both representing pointer and is already used for driver data,
because this match data serves similar purpose as mentioned driver data.

I don't mind casting via unsigned long, but:
1. these are old and trivial issues,
2. they are quite annoying when people want to compile test with W=1,
3. I was trying to fix them (although not sure if for I2C) already,
4. and some others probably as well were trying to fix them,
so how many people need to send them and debate before we fix them finally?

unsigned long vs kernel_ulong_t is a nit-style discussion IMO, unless we
have here more concrete arguments, e.g. statement from Linus that
kernel_ulong_t is wrong.

If maintainers of this code want unsigned long, please apply the patch
and fix directly, but for the sake, let's get it merged...

Best regards,
Krzysztof