leds: pm8058: Make ledtype pointer sized type

Message ID 20171130030543.1071-1-bjorn.andersson@linaro.org
State New
Headers show
Series
  • leds: pm8058: Make ledtype pointer sized type
Related show

Commit Message

Bjorn Andersson Nov. 30, 2017, 3:05 a.m.
The pointer returned by of_device_get_match_data() doesn't have the same
size as u32 on 64-bit architectures, causing issues when compile testing
the driver on such platform. Make ledtype unsigned long instead, to
solve this problem.

Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/leds/leds-pm8058.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.15.0

Comments

Lee Jones Nov. 30, 2017, 8:31 a.m. | #1
On Thu, 30 Nov 2017, Lee Jones wrote:

> On Wed, 29 Nov 2017, Bjorn Andersson wrote:

> 

> > The pointer returned by of_device_get_match_data() doesn't have the same

> > size as u32 on 64-bit architectures, causing issues when compile testing

> > the driver on such platform. Make ledtype unsigned long instead, to

> > solve this problem.

> > 

> > Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> > Cc: Linus Walleij <linus.walleij@linaro.org>

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > ---

> >  drivers/leds/leds-pm8058.c | 4 ++--

> >  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> Hi Bjorn,

> 

> (Nice to see you) :)

> 

> I'm going to apply this *before* Linus' fix.

> 

> Applied, thanks.


After I rx an Ack from Richard, Jacek or Pavel of course. :)

Will send a pull-request.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linus Walleij Nov. 30, 2017, 9:18 a.m. | #2
On Thu, Nov 30, 2017 at 4:05 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> The pointer returned by of_device_get_match_data() doesn't have the same

> size as u32 on 64-bit architectures, causing issues when compile testing

> the driver on such platform. Make ledtype unsigned long instead, to

> solve this problem.

>

> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> Cc: Linus Walleij <linus.walleij@linaro.org>

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Thanks for covering my ass.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>


Yours,
Linus Walleij
Pavel Machek Nov. 30, 2017, 9:40 a.m. | #3
On Wed 2017-11-29 19:05:43, Bjorn Andersson wrote:
> The pointer returned by of_device_get_match_data() doesn't have the same

> size as u32 on 64-bit architectures, causing issues when compile testing

> the driver on such platform. Make ledtype unsigned long instead, to

> solve this problem.

> 

> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> Cc: Linus Walleij <linus.walleij@linaro.org>

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Ummm... no?

extern const void *of_device_get_match_data(const struct device *dev);


> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c

> index a52674327857..cc2afe81720d 100644

> --- a/drivers/leds/leds-pm8058.c

> +++ b/drivers/leds/leds-pm8058.c

> @@ -29,7 +29,7 @@

>  struct pm8058_led {

>  	struct regmap *map;

>  	u32 reg;

> -	u32 ledtype;

> +	unsigned long ledtype;


Make it void *. u32 is buggy. unsigned long is merely ugly code. void
* is not nice, but certainly better than unsigned long.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Lee Jones Nov. 30, 2017, 9:41 a.m. | #4
On Thu, 30 Nov 2017, Linus Walleij wrote:

> On Thu, Nov 30, 2017 at 4:05 AM, Bjorn Andersson

> <bjorn.andersson@linaro.org> wrote:

> 

> > The pointer returned by of_device_get_match_data() doesn't have the same

> > size as u32 on 64-bit architectures, causing issues when compile testing

> > the driver on such platform. Make ledtype unsigned long instead, to

> > solve this problem.

> >

> > Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> > Cc: Linus Walleij <linus.walleij@linaro.org>

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> 

> Thanks for covering my ass.


:D

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>


I still need a LED Ack if I'm going to take this.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Jacek Anaszewski Nov. 30, 2017, 9:58 p.m. | #5
On 11/30/2017 10:40 AM, Pavel Machek wrote:
> On Wed 2017-11-29 19:05:43, Bjorn Andersson wrote:

>> The pointer returned by of_device_get_match_data() doesn't have the same

>> size as u32 on 64-bit architectures, causing issues when compile testing

>> the driver on such platform. Make ledtype unsigned long instead, to

>> solve this problem.

>>

>> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

>> Cc: Linus Walleij <linus.walleij@linaro.org>

>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> 

> Ummm... no?

> 

> extern const void *of_device_get_match_data(const struct device *dev);

> 

> 

>> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c

>> index a52674327857..cc2afe81720d 100644

>> --- a/drivers/leds/leds-pm8058.c

>> +++ b/drivers/leds/leds-pm8058.c

>> @@ -29,7 +29,7 @@

>>  struct pm8058_led {

>>  	struct regmap *map;

>>  	u32 reg;

>> -	u32 ledtype;

>> +	unsigned long ledtype;

> 

> Make it void *. u32 is buggy. unsigned long is merely ugly code. void

> * is not nice, but certainly better than unsigned long.


unsigned long is correct, see below:

static const struct of_device_id pm8058_leds_id_table[] = {
        {
                .compatible = "qcom,pm8058-led",
                .data = (void *)PM8058_LED_TYPE_COMMON
        },
        {
                .compatible = "qcom,pm8058-keypad-led",
                .data = (void *)PM8058_LED_TYPE_KEYPAD
        },
        {
                .compatible = "qcom,pm8058-flash-led",
                .data = (void *)PM8058_LED_TYPE_FLASH
        },
        { },
};

of_device_get_match_data will return PM8058_LED_TYPE_*
which clearly is a led type identifier.

Thus unsigned long looks reasonable.

-- 
Best regards,
Jacek Anaszewski
Jacek Anaszewski Nov. 30, 2017, 10:02 p.m. | #6
On 11/30/2017 09:31 AM, Lee Jones wrote:
> On Thu, 30 Nov 2017, Lee Jones wrote:

> 

>> On Wed, 29 Nov 2017, Bjorn Andersson wrote:

>>

>>> The pointer returned by of_device_get_match_data() doesn't have the same

>>> size as u32 on 64-bit architectures, causing issues when compile testing

>>> the driver on such platform. Make ledtype unsigned long instead, to

>>> solve this problem.

>>>

>>> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

>>> Cc: Linus Walleij <linus.walleij@linaro.org>

>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

>>> ---

>>>  drivers/leds/leds-pm8058.c | 4 ++--

>>>  1 file changed, 2 insertions(+), 2 deletions(-)

>>

>> Hi Bjorn,

>>

>> (Nice to see you) :)

>>

>> I'm going to apply this *before* Linus' fix.

>>

>> Applied, thanks.

> 

> After I rx an Ack from Richard, Jacek or Pavel of course. :)

> 

> Will send a pull-request.


Huh? This is for LED subsystem AFAICS.

-- 
Best regards,
Jacek Anaszewski
Bjorn Andersson Nov. 30, 2017, 10:18 p.m. | #7
On Thu 30 Nov 01:40 PST 2017, Pavel Machek wrote:

> On Wed 2017-11-29 19:05:43, Bjorn Andersson wrote:

> > The pointer returned by of_device_get_match_data() doesn't have the same

> > size as u32 on 64-bit architectures, causing issues when compile testing

> > the driver on such platform. Make ledtype unsigned long instead, to

> > solve this problem.

> > 

> > Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> > Cc: Linus Walleij <linus.walleij@linaro.org>

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> 

> Ummm... no?

> 

> extern const void *of_device_get_match_data(const struct device *dev);

> 


Right, this returns a pointer, which is of architecture dependent size.

> 

> > diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c

> > index a52674327857..cc2afe81720d 100644

> > --- a/drivers/leds/leds-pm8058.c

> > +++ b/drivers/leds/leds-pm8058.c

> > @@ -29,7 +29,7 @@

> >  struct pm8058_led {

> >  	struct regmap *map;

> >  	u32 reg;

> > -	u32 ledtype;

> > +	unsigned long ledtype;

> 

> Make it void *. u32 is buggy. unsigned long is merely ugly code. void

> * is not nice, but certainly better than unsigned long.

> 


unsigned long is the integer type in the kernel that has the same size
as a pointer, similar to the C-standard's intptr_t. So this is the
idiomatic way to pass an integer data type via a pointer variable in the
kernel.

Regards,
Bjorn
Pavel Machek Nov. 30, 2017, 10:34 p.m. | #8
On Thu 2017-11-30 22:58:55, Jacek Anaszewski wrote:
> On 11/30/2017 10:40 AM, Pavel Machek wrote:

> > On Wed 2017-11-29 19:05:43, Bjorn Andersson wrote:

> >> The pointer returned by of_device_get_match_data() doesn't have the same

> >> size as u32 on 64-bit architectures, causing issues when compile testing

> >> the driver on such platform. Make ledtype unsigned long instead, to

> >> solve this problem.

> >>

> >> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> >> Cc: Linus Walleij <linus.walleij@linaro.org>

> >> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > 

> > Ummm... no?

> > 

> > extern const void *of_device_get_match_data(const struct device *dev);

> > 

> > 

> >> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c

> >> index a52674327857..cc2afe81720d 100644

> >> --- a/drivers/leds/leds-pm8058.c

> >> +++ b/drivers/leds/leds-pm8058.c

> >> @@ -29,7 +29,7 @@

> >>  struct pm8058_led {

> >>  	struct regmap *map;

> >>  	u32 reg;

> >> -	u32 ledtype;

> >> +	unsigned long ledtype;

> > 

> > Make it void *. u32 is buggy. unsigned long is merely ugly code. void

> > * is not nice, but certainly better than unsigned long.

> 

> unsigned long is correct, see below:

> 

> static const struct of_device_id pm8058_leds_id_table[] = {

>         {

>                 .compatible = "qcom,pm8058-led",

>                 .data = (void *)PM8058_LED_TYPE_COMMON

>         },

>         {

>                 .compatible = "qcom,pm8058-keypad-led",

>                 .data = (void *)PM8058_LED_TYPE_KEYPAD

>         },

>         {

>                 .compatible = "qcom,pm8058-flash-led",

>                 .data = (void *)PM8058_LED_TYPE_FLASH

>         },

>         { },

> };

> 

> of_device_get_match_data will return PM8058_LED_TYPE_*

> which clearly is a led type identifier.

> 

> Thus unsigned long looks reasonable.


Hmm. Ok. So u32 would actually make even more sense there (because
PM8058_LED_TYPE_* does not really need to be 64-bit), but it would
cause a warning.

I don't like this. This fix actually makes code waste memory and is
uglier.

...but we get a warning fix. So I don't like the patch, but it is an
improvement...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Stephen Rothwell Nov. 30, 2017, 11:14 p.m. | #9
Hi Pavel,

On Thu, 30 Nov 2017 23:34:07 +0100 Pavel Machek <pavel@ucw.cz> wrote:
>

> On Thu 2017-11-30 22:58:55, Jacek Anaszewski wrote:

> > On 11/30/2017 10:40 AM, Pavel Machek wrote:  

> > > On Wed 2017-11-29 19:05:43, Bjorn Andersson wrote:  

> > >> The pointer returned by of_device_get_match_data() doesn't have the same

> > >> size as u32 on 64-bit architectures, causing issues when compile testing

> > >> the driver on such platform. Make ledtype unsigned long instead, to

> > >> solve this problem.

> > >>

> > >> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

> > >> Cc: Linus Walleij <linus.walleij@linaro.org>

> > >> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>  

> > > 

> > > Ummm... no?

> > > 

> > > extern const void *of_device_get_match_data(const struct device *dev);

> > > 

> > >   

> > >> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c

> > >> index a52674327857..cc2afe81720d 100644

> > >> --- a/drivers/leds/leds-pm8058.c

> > >> +++ b/drivers/leds/leds-pm8058.c

> > >> @@ -29,7 +29,7 @@

> > >>  struct pm8058_led {

> > >>  	struct regmap *map;

> > >>  	u32 reg;

> > >> -	u32 ledtype;

> > >> +	unsigned long ledtype;  

> > > 

> > > Make it void *. u32 is buggy. unsigned long is merely ugly code. void

> > > * is not nice, but certainly better than unsigned long.  

> > 

> > unsigned long is correct, see below:

> > 

> > static const struct of_device_id pm8058_leds_id_table[] = {

> >         {

> >                 .compatible = "qcom,pm8058-led",

> >                 .data = (void *)PM8058_LED_TYPE_COMMON

> >         },

> >         {

> >                 .compatible = "qcom,pm8058-keypad-led",

> >                 .data = (void *)PM8058_LED_TYPE_KEYPAD

> >         },

> >         {

> >                 .compatible = "qcom,pm8058-flash-led",

> >                 .data = (void *)PM8058_LED_TYPE_FLASH

> >         },

> >         { },

> > };

> > 

> > of_device_get_match_data will return PM8058_LED_TYPE_*

> > which clearly is a led type identifier.

> > 

> > Thus unsigned long looks reasonable.  

> 

> Hmm. Ok. So u32 would actually make even more sense there (because

> PM8058_LED_TYPE_* does not really need to be 64-bit), but it would

> cause a warning.

> 

> I don't like this. This fix actually makes code waste memory and is

> uglier.

> 

> ...but we get a warning fix. So I don't like the patch, but it is an

> improvement...


I *think* you can get away with (u32)(unsigned long)ptr ...
-- 
Cheers,
Stephen Rothwell
Pavel Machek Dec. 1, 2017, 12:28 a.m. | #10
Hi!

> > > of_device_get_match_data will return PM8058_LED_TYPE_*

> > > which clearly is a led type identifier.

> > > 

> > > Thus unsigned long looks reasonable.  

> > 

> > Hmm. Ok. So u32 would actually make even more sense there (because

> > PM8058_LED_TYPE_* does not really need to be 64-bit), but it would

> > cause a warning.

> > 

> > I don't like this. This fix actually makes code waste memory and is

> > uglier.

> > 

> > ...but we get a warning fix. So I don't like the patch, but it is an

> > improvement...

> 

> I *think* you can get away with (u32)(unsigned long)ptr ...


Yes, that should work. It would actually be my preferred solution.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Jacek Anaszewski Dec. 1, 2017, 8:35 p.m. | #11
On 12/01/2017 08:56 AM, Lee Jones wrote:
> On Thu, 30 Nov 2017, Jacek Anaszewski wrote:

> 

>> On 11/30/2017 09:31 AM, Lee Jones wrote:

>>> On Thu, 30 Nov 2017, Lee Jones wrote:

>>>

>>>> On Wed, 29 Nov 2017, Bjorn Andersson wrote:

>>>>

>>>>> The pointer returned by of_device_get_match_data() doesn't have the same

>>>>> size as u32 on 64-bit architectures, causing issues when compile testing

>>>>> the driver on such platform. Make ledtype unsigned long instead, to

>>>>> solve this problem.

>>>>>

>>>>> Fixes: 7f866986e705 ("leds: add PM8058 LEDs driver")

>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>

>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

>>>>> ---

>>>>>  drivers/leds/leds-pm8058.c | 4 ++--

>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)

>>>>

>>>> Hi Bjorn,

>>>>

>>>> (Nice to see you) :)

>>>>

>>>> I'm going to apply this *before* Linus' fix.

>>>>

>>>> Applied, thanks.

>>>

>>> After I rx an Ack from Richard, Jacek or Pavel of course. :)

>>>

>>> Will send a pull-request.

>>

>> Huh? This is for LED subsystem AFAICS.

> 

> Right, hence why I said I'd sent out a pull-request.

> 

> The problem, however, arose due to a change in its parent driver's

> Kconfig entry, which has been applied to the MFD tree.  We need to

> ensure this patch is applied *before* the other 'fix' to quash the

> warning before it starts, so to speak.


Ah, I hadn't seen the MFD patch and understood that you was
talking about pull request for 4.15-rc2, which looked a bit
strange out of context.

For v2:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>


-- 
Best regards,
Jacek Anaszewski

Patch

diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c
index a52674327857..cc2afe81720d 100644
--- a/drivers/leds/leds-pm8058.c
+++ b/drivers/leds/leds-pm8058.c
@@ -29,7 +29,7 @@ 
 struct pm8058_led {
 	struct regmap *map;
 	u32 reg;
-	u32 ledtype;
+	unsigned long ledtype;
 	struct led_classdev cdev;
 };
 
@@ -106,7 +106,7 @@  static int pm8058_led_probe(struct platform_device *pdev)
 	if (!led)
 		return -ENOMEM;
 
-	led->ledtype = (u32)of_device_get_match_data(&pdev->dev);
+	led->ledtype = (unsigned long)of_device_get_match_data(&pdev->dev);
 
 	map = dev_get_regmap(pdev->dev.parent, NULL);
 	if (!map) {