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

[Arnd Bergmann]

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@xxxxxxxx>
>> ---
>> 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@xxxxxxxx/
>> ---
>>  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