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ć <[email protected]>
---
This patch is necessary for my Marvell PXA1908 series to compile successfully
with allyesconfig:
https://lore.kernel.org/all/[email protected]/
---
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;
} 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
--
2.43.0
[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ć <[email protected]>
> ---
> This patch is necessary for my Marvell PXA1908 series to compile successfully
> with allyesconfig:
> https://lore.kernel.org/all/[email protected]/
> ---
> 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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ć <[email protected]>
>> ---
>> This patch is necessary for my Marvell PXA1908 series to compile successfully
>> with allyesconfig:
>> https://lore.kernel.org/all/[email protected]/
>> ---
>> 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ć <[email protected]>
> >> ---
> >> This patch is necessary for my Marvell PXA1908 series to compile successfully
> >> with allyesconfig:
> >> https://lore.kernel.org/all/[email protected]/
> >> ---
> >> 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |