Re: [PATCH V6 4/7] mfd: da9061: MFD core support

[Geert Uytterhoeven]

Hi Lee,

On Wed, Mar 29, 2017 at 10:30 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Tue, 28 Mar 2017, Geert Uytterhoeven wrote:
>> On Tue, Mar 28, 2017 at 12:42 PM, Steve Twiss
>> <stwiss.opensource@xxxxxxxxxxx> wrote:
>> > On 28 March 2017 09:37, Geert Uytterhoeven wrote:
>> >> Subject: Re: [PATCH V6 4/7] mfd: da9061: MFD core support
>> >> On Tue, Mar 28, 2017 at 10:21 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>> >> >> [auto build test WARNING on ljones-mfd/for-mfd-next]
>> >> >> [also build test WARNING on v4.11-rc4 next-20170327]
>> >> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
>> >> >> config: x86_64-randconfig-x009-201713 (attached as .config)
>> >> >> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
>> >> >> reproduce:
>> >> >>         # save the attached .config to linux build tree
>> >> >>         make ARCH=x86_64
>> >> >>
>> >> >> All warnings (new ones prefixed by >>):
>> >> >>
>> >> >>    drivers//mfd/da9062-core.c: In function 'da9062_i2c_probe':
>> >> >> >> drivers//mfd/da9062-core.c:845:21: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>> >> >>       chip->chip_type = (int)match->data;
>> >> >>                        ^
>> >> >
>> >> > Please use longs or enums.
>> >>
>> >> Enums would still give a warning on 64-bit.
>> >> The simple fix is change the cast from (int) to (uintptr_t).
>> >
>> > Hi Lee and Geert,
>> >
>> > How about this? Fix by redefining the enum chip_type to be an int.
>> > Then, just use substitution:
>> > #define COMPAT_TYPE_DA9061  1
>> > #define COMPAT_TYPE_DA9062  2
>> >
>> > That would be simple.
>> > Are there any reasons this would not be acceptable?
>>
>> I don't see how that can help.
>> The warning is caused by casting the "void *" (which is either 32-bit or
>> 64-bit) in of_device_if.data to an integer or enum (which is always 32-bit).
>>
>> The right fix is to cast it to uintptr_t intead of int, like other drivers do.
>
> That's a hack though, isn't it?  chip->chip_type is not of type
> uintptr_t, so all you're doing here is making the compiler happy.

It's a hack, in the sense that using casts is usually a hack :-)
(I would love for C to have casts you can easily grep for, like C++ has)

In this case, it's not really a hack, as the void * is intended to store
arbitrary data, both pointers and integers.

> I see people using (int)(uintptr_t), which I guess keeps the
> compiler happy in the first instance and actually culminates in a
> correct cast to the right type.

The cast to int isn't really needed.

> So how about something like (enum da9062_compatible_types)(long)?

"uintptr_t" is the proper C standard type for conversion between pointers and
integers.  The cast to the enum isn't really needed, so I'd remove it (the
less number of casts, the better!).

Long is the hack that works on ILP32 and LP64 platforms only ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds