| Message ID | 20240103210604.16877-1-duje.mihanovic@skole.hr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,RESEND] soc: pxa: ssp: Cast to enum pxa_ssp_type instead of int | expand |
[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. The following would also help: diff --git a/drivers/soc/pxa/ssp.c b/drivers/soc/pxa/ssp.c index a1e8a07f7275..095d997eb886 100644 --- a/drivers/soc/pxa/ssp.c +++ b/drivers/soc/pxa/ssp.c @@ -96,13 +96,13 @@ EXPORT_SYMBOL(pxa_ssp_free); #ifdef CONFIG_OF static const struct of_device_id pxa_ssp_of_ids[] = { - { .compatible = "mrvl,pxa25x-ssp", .data = (void *) PXA25x_SSP }, - { .compatible = "mvrl,pxa25x-nssp", .data = (void *) PXA25x_NSSP }, - { .compatible = "mrvl,pxa27x-ssp", .data = (void *) PXA27x_SSP }, - { .compatible = "mrvl,pxa3xx-ssp", .data = (void *) PXA3xx_SSP }, - { .compatible = "mvrl,pxa168-ssp", .data = (void *) PXA168_SSP }, - { .compatible = "mrvl,pxa910-ssp", .data = (void *) PXA910_SSP }, - { .compatible = "mrvl,ce4100-ssp", .data = (void *) CE4100_SSP }, + { .compatible = "mrvl,pxa25x-ssp", .driver_data = PXA25x_SSP }, + { .compatible = "mvrl,pxa25x-nssp", .driver_data = PXA25x_NSSP }, + { .compatible = "mrvl,pxa27x-ssp", .driver_data = PXA27x_SSP }, + { .compatible = "mrvl,pxa3xx-ssp", .driver_data = PXA3xx_SSP }, + { .compatible = "mvrl,pxa168-ssp", .driver_data = PXA168_SSP }, + { .compatible = "mrvl,pxa910-ssp", .driver_data = PXA910_SSP }, + { .compatible = "mrvl,ce4100-ssp", .driver_data = CE4100_SSP }, { }, }; MODULE_DEVICE_TABLE(of, pxa_ssp_of_ids); @@ -152,7 +152,7 @@ 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 = id->driver_data; } else { const struct platform_device_id *id = platform_get_device_id(pdev); 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. > } else { > const struct platform_device_id *id = > platform_get_device_id(pdev); > - ssp->type = (int) id->driver_data; > + ssp->type = (enum pxa_ssp_type) id->driver_data; This one isn't problematic in my build configuration and you could just drop the cast completely. Best regards Uwe
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
Hello Arnd, On Thu, Jan 04, 2024 at 11:03:41AM +0100, Arnd Bergmann wrote: > 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. Sure, I didn't intend to put the diff into a commit as is. Before doing that change it would probably be sensible to check how this field is used, I guess most drivers use an integer value and not a pointer there. Also while touching that making the same change with the same names for the other *_id structs might be nice. Currently we have (from include/linux/mod_devicetable.h): pci_device_id kernel_ulong_t driver_data ieee1394_device_id kernel_ulong_t driver_data usb_device_id kernel_ulong_t driver_info hid_device_id kernel_ulong_t driver_data ccw_device_id kernel_ulong_t driver_info ap_device_id kernel_ulong_t driver_info css_device_id kernel_ulong_t driver_data acpi_device_id kernel_ulong_t driver_data pnp_device_id kernel_ulong_t driver_data pnp_card_device_id kernel_ulong_t driver_data serio_device_id - hda_device_id unsigned long driver_data sdw_device_id kernel_ulong_t driver_data of_device_id const void *data vio_device_id - pcmcia_device_id kernel_ulong_t driver_info input_device_id kernel_ulong_t driver_info eisa_device_id kernel_ulong_t driver_data parisc_device_id - sdio_device_id kernel_ulong_t driver_data ssb_device_id - bcma_device_id - virtio_device_id - hv_vmbus_device_id kernel_ulong_t driver_data rpmsg_device_id kernel_ulong_t driver_data i2c_device_id kernel_ulong_t driver_data pci_epf_device_id kernel_ulong_t driver_data i3c_device_id const void *data spi_device_id kernel_ulong_t driver_data slim_device_id - apr_device_id kernel_ulong_t driver_data spmi_device_id kernel_ulong_t driver_data dmi_system_id void *driver_data platform_device_id kernel_ulong_t driver_data mdio_device_id - zorro_device_id kernel_ulong_t driver_data isapnp_device_id kernel_ulong_t driver_data amba_id void *data mips_cdmm_device_id - x86_cpu_id kernel_ulong_t driver_data ipack_device_id - mei_cl_device_id kernel_ulong_t driver_info rio_device_id - mcb_device_id kernel_ulong_t driver_data ulpi_device_id kernel_ulong_t driver_data fsl_mc_device_id - tb_service_id kernel_ulong_t driver_data typec_device_id kernel_ulong_t driver_data tee_client_device_id - wmi_device_id const void *context mhi_device_id kernel_ulong_t driver_data auxiliary_device_id kernel_ulong_t driver_data ssam_device_id kernel_ulong_t driver_data dfl_device_id kernel_ulong_t driver_data ishtp_device_id kernel_ulong_t driver_data cdx_device_id - vchiq_device_id - Note driver_data vs driver_info which is probably not worth to unify. > 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. I'll add that to my list of ideas for big projects :-) Best regards Uwe
Hello Uwe and Arnd, for v2 I will change the first cast to (uintptr_t) as Arnd suggested and drop the second cast completely as Uwe suggested. As a long-term solution (at least for this particular driver), the few PXA platforms and drivers still depending on or using this driver at first seem trivial to convert to pxa2xx-spi, while the navpoint driver seemingly could be removed entirely as all boards using it have been removed. If there are no objections I am willing to do this conversion. -- Regards, Duje
Hello Duje, On Thu, Jan 04, 2024 at 03:23:09PM +0100, Duje Mihanović wrote: > for v2 I will change the first cast to (uintptr_t) as Arnd suggested and drop > the second cast completely as Uwe suggested. > > As a long-term solution (at least for this particular driver), the few PXA > platforms and drivers still depending on or using this driver at first seem > trivial to convert to pxa2xx-spi, while the navpoint driver seemingly could be > removed entirely as all boards using it have been removed. If there are no > objections I am willing to do this conversion. In case you need encouragement: Sounds like a nice plan; no objections from my side. Best regards Uwe
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; } else { const struct platform_device_id *id = platform_get_device_id(pdev); - ssp->type = (int) id->driver_data; + ssp->type = (enum pxa_ssp_type) id->driver_data; /* PXA2xx/3xx SSP ports starts from 1 and the internal pdev->id * starts from 0, do a translation here
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(-)