Re: [PATCH RFC RESEND] soc: pxa: ssp: Cast to enum pxa_ssp_type instead of int

Arnd Bergmann
2024-01-04T10:04:11Z

On Thu, Jan 4, 2024, at 10:08, Uwe Kleine-König wrote:
> [adding lakml to Cc for wider audience]
>
> On Wed, Jan 03, 2024 at 10:06:03PM +0100, Duje Mihanović wrote:
>> On ARM64 platforms, id->data is a 64-bit value and casting it to a
>> 32-bit integer causes build errors. Cast it to the corresponding enum
>> instead.
>>
>> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
>> ---
>> This patch is necessary for my Marvell PXA1908 series to compile successfully
>> with allyesconfig:
>> https://lore.kernel.org/all/20231102-pxa1908-lkml-v7-0-cabb1a0cb52b@skole.hr/
>> ---
>> drivers/soc/pxa/ssp.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/pxa/ssp.c b/drivers/soc/pxa/ssp.c
>> index a1e8a07f7275..e2ffd8fd7e13 100644
>> --- a/drivers/soc/pxa/ssp.c
>> +++ b/drivers/soc/pxa/ssp.c
>> @@ -152,11 +152,11 @@ static int pxa_ssp_probe(struct platform_device *pdev)
>> if (dev->of_node) {
>> const struct of_device_id *id =
>> of_match_device(of_match_ptr(pxa_ssp_of_ids), dev);
>> - ssp->type = (int) id->data;
>> + ssp->type = (enum pxa_ssp_type) id->data;
>
> I wonder if this is a long-term fix. The error that the compiler throws
> is:
>
> drivers/soc/pxa/ssp.c:155:29: error: cast from pointer to integer of
> different size [-Werror=pointer-to-int-cast]
> 155 | ssp->type = (int) id->data;
> | ^
>
> enum pxa_ssp_type is an integer type, too, and its width could be
> smaller than 64 bit, too.

I would just change the cast to (uintptr_t), which is what we
have elsewhere.

> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index f458469c5ce5..fbe16089e4bb 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -283,7 +283,10 @@ struct of_device_id {
> char name[32];
> char type[32];
> char compatible[128];
> - const void *data;
> + union {
> + const void *data;
> + kernel_ulong_t driver_data;
> + };
> };
>
> /* VIO */
>
> For this driver the change would be nice, as several casts can be
> dropped.

I think this is a nice idea in general, but I would keep
it separate from the bugfix, as we might want to do this on
a wider scale, or run into problems.

In particular, removing tons of casts to (kernel_ulong_t)
in other subsystems is probably more valuable than removing
casts to (void *) for of_device_id, since kernel_ulong_t
is particularly confusing, with a definition that is
completely unrelated to the similarly named __kernel_ulong_t.

Arnd