[PATCH 1/2] leds: simatic-ipc-leds: Make simatic_ipc_led_mem_res static

[PATCH 1/2] leds: simatic-ipc-leds: Make simatic_ipc_led_mem_res static

[Hans de Goede]

simatic_ipc_led_mem_res is not used outside of the driver, make it static.

Cc: Henning Schild <henning.schild@siemens.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/simple/simatic-ipc-leds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
index ff2c96e73241..179110448659 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -39,7 +39,7 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
 };
 
 /* the actual start will be discovered with PCI, 0 is a placeholder */
-struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
 
 static void *simatic_ipc_led_memory;
 
-- 
2.33.1

[PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Hans de Goede]

Sparse (rightly) currently gives the following warning:

drivers/leds/simple/simatic-ipc-leds.c:155:40:
 sparse: sparse: incorrect type in assignment (different address spaces)
 expected void *static [toplevel] simatic_ipc_led_memory
 got void [noderef] __iomem *

Fix this by changing the type of simatic_ipc_led_memory to void __iomem *
and use readl()/writel() to access it.

Cc: Henning Schild <henning.schild@siemens.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note this is not tested on actual hw, since I do not have the hw in question
---
 drivers/leds/simple/simatic-ipc-leds.c | 32 +++++++++++++++-----------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
index 179110448659..078d43f5ba38 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -41,7 +41,7 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
 /* the actual start will be discovered with PCI, 0 is a placeholder */
 static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
 
-static void *simatic_ipc_led_memory;
+static void __iomem *simatic_ipc_led_memory;
 
 static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
 	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
@@ -92,21 +92,22 @@ static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
 				    enum led_brightness brightness)
 {
 	struct simatic_ipc_led *led = cdev_to_led(led_cd);
+	void __iomem *reg = simatic_ipc_led_memory + led->value;
+	u32 val;
 
-	u32 *p;
-
-	p = simatic_ipc_led_memory + led->value;
-	*p = (*p & ~1) | (brightness == LED_OFF);
+	val = readl(reg);
+	val = (val & ~1) | (brightness == LED_OFF);
+	writel(val, reg);
 }
 
 static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
 {
 	struct simatic_ipc_led *led = cdev_to_led(led_cd);
+	void __iomem *reg = simatic_ipc_led_memory + led->value;
+	u32 val;
 
-	u32 *p;
-
-	p = simatic_ipc_led_memory + led->value;
-	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
+	val = readl(reg);
+	return (val & 1) ? LED_OFF : led_cd->max_brightness;
 }
 
 static int simatic_ipc_leds_probe(struct platform_device *pdev)
@@ -116,8 +117,9 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 	struct simatic_ipc_led *ipcled;
 	struct led_classdev *cdev;
 	struct resource *res;
+	void __iomem *reg;
 	int err, type;
-	u32 *p;
+	u32 val;
 
 	switch (plat->devmode) {
 	case SIMATIC_IPC_DEVICE_227D:
@@ -157,11 +159,13 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
 			return PTR_ERR(simatic_ipc_led_memory);
 
 		/* initialize power/watchdog LED */
-		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
-		*p = (*p & ~1);
-		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
-		*p = (*p | 1);
+		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
+		val = readl(reg);
+		writel(val & ~1, reg);
 
+		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
+		val = readl(reg);
+		writel(val | 1, reg);
 		break;
 	default:
 		return -ENODEV;
-- 
2.33.1

Re: [PATCH 1/2] leds: simatic-ipc-leds: Make simatic_ipc_led_mem_res static

[Henning Schild]

LGTM

Am Mon, 17 Jan 2022 12:21:08 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> simatic_ipc_led_mem_res is not used outside of the driver, make it
> static.
> 
> Cc: Henning Schild <henning.schild@siemens.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/simple/simatic-ipc-leds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> ff2c96e73241..179110448659 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -39,7 +39,7 @@ static
> struct simatic_ipc_led simatic_ipc_leds_io[] = { };
>  
>  /* the actual start will be discovered with PCI, 0 is a placeholder
> */ -struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
> SZ_4K, KBUILD_MODNAME); +static struct resource
> simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K,
> KBUILD_MODNAME); static void *simatic_ipc_led_memory;
>  

Re: [PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Henning Schild]

Hi Hans,

this looks fine but also looks like someone should test it. What is the
timeline on that? I would want that tested but will need a few more
days to actually sit next to boxes and look at LEDs.

Gerd maybe you can try that? Should affect 127E only and can probably
be applied onto our out-of-tree repo.

regards,
Henning

Am Mon, 17 Jan 2022 12:21:09 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Sparse (rightly) currently gives the following warning:
> 
> drivers/leds/simple/simatic-ipc-leds.c:155:40:
>  sparse: sparse: incorrect type in assignment (different address
> spaces) expected void *static [toplevel] simatic_ipc_led_memory
>  got void [noderef] __iomem *
> 
> Fix this by changing the type of simatic_ipc_led_memory to void
> __iomem * and use readl()/writel() to access it.
> 
> Cc: Henning Schild <henning.schild@siemens.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note this is not tested on actual hw, since I do not have the hw in
> question ---
>  drivers/leds/simple/simatic-ipc-leds.c | 32
> +++++++++++++++----------- 1 file changed, 18 insertions(+), 14
> deletions(-)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> 179110448659..078d43f5ba38 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -41,7 +41,7 @@ static
> struct simatic_ipc_led simatic_ipc_leds_io[] = { /* the actual start
> will be discovered with PCI, 0 is a placeholder */ static struct
> resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K,
> KBUILD_MODNAME); -static void *simatic_ipc_led_memory;
> +static void __iomem *simatic_ipc_led_memory;
>  
>  static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
>  	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> @@ -92,21 +92,22 @@ static void simatic_ipc_led_set_mem(struct
> led_classdev *led_cd, enum led_brightness brightness)
>  {
>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
> +	u32 val;
>  
> -	u32 *p;
> -
> -	p = simatic_ipc_led_memory + led->value;
> -	*p = (*p & ~1) | (brightness == LED_OFF);
> +	val = readl(reg);
> +	val = (val & ~1) | (brightness == LED_OFF);
> +	writel(val, reg);
>  }
>  
>  static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) {
>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
> +	u32 val;
>  
> -	u32 *p;
> -
> -	p = simatic_ipc_led_memory + led->value;
> -	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
> +	val = readl(reg);
> +	return (val & 1) ? LED_OFF : led_cd->max_brightness;
>  }
>  
>  static int simatic_ipc_leds_probe(struct platform_device *pdev)
> @@ -116,8 +117,9 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) struct simatic_ipc_led *ipcled;
>  	struct led_classdev *cdev;
>  	struct resource *res;
> +	void __iomem *reg;
>  	int err, type;
> -	u32 *p;
> +	u32 val;
>  
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_227D:
> @@ -157,11 +159,13 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) return PTR_ERR(simatic_ipc_led_memory);
>  
>  		/* initialize power/watchdog LED */
> -		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> -		*p = (*p & ~1);
> -		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> -		*p = (*p | 1);
> +		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> +		val = readl(reg);
> +		writel(val & ~1, reg);
>  
> +		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> +		val = readl(reg);
> +		writel(val | 1, reg);
>  		break;
>  	default:
>  		return -ENODEV;

Re: [PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Hans de Goede]

Hi Henning,

On 1/28/22 10:30, Henning Schild wrote:
> Hi Hans,
> 
> this looks fine but also looks like someone should test it. What is the
> timeline on that? I would want that tested but will need a few more
> days to actually sit next to boxes and look at LEDs.

It would be nice to get this fix in before say 5.17-rc5, so testing
this a couple of days from now is fine.

Talking about merging this...

Pavel, since the original patch has landed through the pdx86 tree
in 5.17-rc1 and since these 2 patches are LED patches I was sorta
expecting you to pick these up.  But if it is easier for you,
I would also happy to make these patches part of a pdx86 fixes
pull-req for 5.17 .

Regards,

Hans






> 
> Gerd maybe you can try that? Should affect 127E only and can probably
> be applied onto our out-of-tree repo.
> 
> regards,
> Henning
> 
> Am Mon, 17 Jan 2022 12:21:09 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Sparse (rightly) currently gives the following warning:
>>
>> drivers/leds/simple/simatic-ipc-leds.c:155:40:
>>  sparse: sparse: incorrect type in assignment (different address
>> spaces) expected void *static [toplevel] simatic_ipc_led_memory
>>  got void [noderef] __iomem *
>>
>> Fix this by changing the type of simatic_ipc_led_memory to void
>> __iomem * and use readl()/writel() to access it.
>>
>> Cc: Henning Schild <henning.schild@siemens.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note this is not tested on actual hw, since I do not have the hw in
>> question ---
>>  drivers/leds/simple/simatic-ipc-leds.c | 32
>> +++++++++++++++----------- 1 file changed, 18 insertions(+), 14
>> deletions(-)
>>
>> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
>> b/drivers/leds/simple/simatic-ipc-leds.c index
>> 179110448659..078d43f5ba38 100644 ---
>> a/drivers/leds/simple/simatic-ipc-leds.c +++
>> b/drivers/leds/simple/simatic-ipc-leds.c @@ -41,7 +41,7 @@ static
>> struct simatic_ipc_led simatic_ipc_leds_io[] = { /* the actual start
>> will be discovered with PCI, 0 is a placeholder */ static struct
>> resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K,
>> KBUILD_MODNAME); -static void *simatic_ipc_led_memory;
>> +static void __iomem *simatic_ipc_led_memory;
>>  
>>  static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
>>  	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
>> @@ -92,21 +92,22 @@ static void simatic_ipc_led_set_mem(struct
>> led_classdev *led_cd, enum led_brightness brightness)
>>  {
>>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
>> +	u32 val;
>>  
>> -	u32 *p;
>> -
>> -	p = simatic_ipc_led_memory + led->value;
>> -	*p = (*p & ~1) | (brightness == LED_OFF);
>> +	val = readl(reg);
>> +	val = (val & ~1) | (brightness == LED_OFF);
>> +	writel(val, reg);
>>  }
>>  
>>  static enum led_brightness simatic_ipc_led_get_mem(struct
>> led_classdev *led_cd) {
>>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
>> +	u32 val;
>>  
>> -	u32 *p;
>> -
>> -	p = simatic_ipc_led_memory + led->value;
>> -	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
>> +	val = readl(reg);
>> +	return (val & 1) ? LED_OFF : led_cd->max_brightness;
>>  }
>>  
>>  static int simatic_ipc_leds_probe(struct platform_device *pdev)
>> @@ -116,8 +117,9 @@ static int simatic_ipc_leds_probe(struct
>> platform_device *pdev) struct simatic_ipc_led *ipcled;
>>  	struct led_classdev *cdev;
>>  	struct resource *res;
>> +	void __iomem *reg;
>>  	int err, type;
>> -	u32 *p;
>> +	u32 val;
>>  
>>  	switch (plat->devmode) {
>>  	case SIMATIC_IPC_DEVICE_227D:
>> @@ -157,11 +159,13 @@ static int simatic_ipc_leds_probe(struct
>> platform_device *pdev) return PTR_ERR(simatic_ipc_led_memory);
>>  
>>  		/* initialize power/watchdog LED */
>> -		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>> PM_WDT_OUT */
>> -		*p = (*p & ~1);
>> -		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>> PM_BIOS_BOOT_N */
>> -		*p = (*p | 1);
>> +		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>> PM_WDT_OUT */
>> +		val = readl(reg);
>> +		writel(val & ~1, reg);
>>  
>> +		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>> PM_BIOS_BOOT_N */
>> +		val = readl(reg);
>> +		writel(val | 1, reg);
>>  		break;
>>  	default:
>>  		return -ENODEV;
> 

Re: [PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Henning Schild]

Am Fri, 28 Jan 2022 10:37:36 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi Henning,
> 
> On 1/28/22 10:30, Henning Schild wrote:
> > Hi Hans,
> > 
> > this looks fine but also looks like someone should test it. What is
> > the timeline on that? I would want that tested but will need a few
> > more days to actually sit next to boxes and look at LEDs.  
> 
> It would be nice to get this fix in before say 5.17-rc5, so testing
> this a couple of days from now is fine.

Gerd was so kind to just it done quickly today. He confirms its working
as expected.

Thanks guys. I guess you could add a 

Tested-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>

if you want.

regards,
Henning
 
> Talking about merging this...
> 
> Pavel, since the original patch has landed through the pdx86 tree
> in 5.17-rc1 and since these 2 patches are LED patches I was sorta
> expecting you to pick these up.  But if it is easier for you,
> I would also happy to make these patches part of a pdx86 fixes
> pull-req for 5.17 .
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> > 
> > Gerd maybe you can try that? Should affect 127E only and can
> > probably be applied onto our out-of-tree repo.
> > 
> > regards,
> > Henning
> > 
> > Am Mon, 17 Jan 2022 12:21:09 +0100
> > schrieb Hans de Goede <hdegoede@redhat.com>:
> >   
> >> Sparse (rightly) currently gives the following warning:
> >>
> >> drivers/leds/simple/simatic-ipc-leds.c:155:40:
> >>  sparse: sparse: incorrect type in assignment (different address
> >> spaces) expected void *static [toplevel] simatic_ipc_led_memory
> >>  got void [noderef] __iomem *
> >>
> >> Fix this by changing the type of simatic_ipc_led_memory to void
> >> __iomem * and use readl()/writel() to access it.
> >>
> >> Cc: Henning Schild <henning.schild@siemens.com>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Note this is not tested on actual hw, since I do not have the hw in
> >> question ---
> >>  drivers/leds/simple/simatic-ipc-leds.c | 32
> >> +++++++++++++++----------- 1 file changed, 18 insertions(+), 14
> >> deletions(-)
> >>
> >> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> >> b/drivers/leds/simple/simatic-ipc-leds.c index
> >> 179110448659..078d43f5ba38 100644 ---
> >> a/drivers/leds/simple/simatic-ipc-leds.c +++
> >> b/drivers/leds/simple/simatic-ipc-leds.c @@ -41,7 +41,7 @@ static
> >> struct simatic_ipc_led simatic_ipc_leds_io[] = { /* the actual
> >> start will be discovered with PCI, 0 is a placeholder */ static
> >> struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
> >> SZ_4K, KBUILD_MODNAME); -static void *simatic_ipc_led_memory;
> >> +static void __iomem *simatic_ipc_led_memory;
> >>  
> >>  static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> >>  	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> >> @@ -92,21 +92,22 @@ static void simatic_ipc_led_set_mem(struct
> >> led_classdev *led_cd, enum led_brightness brightness)
> >>  {
> >>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> >> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
> >> +	u32 val;
> >>  
> >> -	u32 *p;
> >> -
> >> -	p = simatic_ipc_led_memory + led->value;
> >> -	*p = (*p & ~1) | (brightness == LED_OFF);
> >> +	val = readl(reg);
> >> +	val = (val & ~1) | (brightness == LED_OFF);
> >> +	writel(val, reg);
> >>  }
> >>  
> >>  static enum led_brightness simatic_ipc_led_get_mem(struct
> >> led_classdev *led_cd) {
> >>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> >> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
> >> +	u32 val;
> >>  
> >> -	u32 *p;
> >> -
> >> -	p = simatic_ipc_led_memory + led->value;
> >> -	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
> >> +	val = readl(reg);
> >> +	return (val & 1) ? LED_OFF : led_cd->max_brightness;
> >>  }
> >>  
> >>  static int simatic_ipc_leds_probe(struct platform_device *pdev)
> >> @@ -116,8 +117,9 @@ static int simatic_ipc_leds_probe(struct
> >> platform_device *pdev) struct simatic_ipc_led *ipcled;
> >>  	struct led_classdev *cdev;
> >>  	struct resource *res;
> >> +	void __iomem *reg;
> >>  	int err, type;
> >> -	u32 *p;
> >> +	u32 val;
> >>  
> >>  	switch (plat->devmode) {
> >>  	case SIMATIC_IPC_DEVICE_227D:
> >> @@ -157,11 +159,13 @@ static int simatic_ipc_leds_probe(struct
> >> platform_device *pdev) return PTR_ERR(simatic_ipc_led_memory);
> >>  
> >>  		/* initialize power/watchdog LED */
> >> -		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> >> PM_WDT_OUT */
> >> -		*p = (*p & ~1);
> >> -		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> >> PM_BIOS_BOOT_N */
> >> -		*p = (*p | 1);
> >> +		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> >> PM_WDT_OUT */
> >> +		val = readl(reg);
> >> +		writel(val & ~1, reg);
> >>  
> >> +		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> >> PM_BIOS_BOOT_N */
> >> +		val = readl(reg);
> >> +		writel(val | 1, reg);
> >>  		break;
> >>  	default:
> >>  		return -ENODEV;  
> >   
> 

Re: [PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Hans de Goede]

Hi,

On 1/28/22 16:26, Henning Schild wrote:
> Am Fri, 28 Jan 2022 10:37:36 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi Henning,
>>
>> On 1/28/22 10:30, Henning Schild wrote:
>>> Hi Hans,
>>>
>>> this looks fine but also looks like someone should test it. What is
>>> the timeline on that? I would want that tested but will need a few
>>> more days to actually sit next to boxes and look at LEDs.  
>>
>> It would be nice to get this fix in before say 5.17-rc5, so testing
>> this a couple of days from now is fine.
> 
> Gerd was so kind to just it done quickly today. He confirms its working
> as expected.
> 
> Thanks guys. I guess you could add a 
> 
> Tested-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> 
> if you want.
> 
> regards,
> Henning
>  
>> Talking about merging this...
>>
>> Pavel, since the original patch has landed through the pdx86 tree
>> in 5.17-rc1 and since these 2 patches are LED patches I was sorta
>> expecting you to pick these up.  But if it is easier for you,
>> I would also happy to make these patches part of a pdx86 fixes
>> pull-req for 5.17 .

Pavel, since this has also been tested now, I would be happy to pick
this series up as fixes for 5.17 (since the original simatic-ipc-leds
driver was merged for 5.17 through the pdx86 tree).

Since the original driver is in 5.17-rc1 this could also go upstream
through the leds tree though. Regardless this should be queued up
as fixes for 5.17 to fix the sparse warnings introduced by the new
driver.

Pavel, please let me know how you want to proceed with this ?

Regards,

Hans



>>> Am Mon, 17 Jan 2022 12:21:09 +0100
>>> schrieb Hans de Goede <hdegoede@redhat.com>:
>>>   
>>>> Sparse (rightly) currently gives the following warning:
>>>>
>>>> drivers/leds/simple/simatic-ipc-leds.c:155:40:
>>>>  sparse: sparse: incorrect type in assignment (different address
>>>> spaces) expected void *static [toplevel] simatic_ipc_led_memory
>>>>  got void [noderef] __iomem *
>>>>
>>>> Fix this by changing the type of simatic_ipc_led_memory to void
>>>> __iomem * and use readl()/writel() to access it.
>>>>
>>>> Cc: Henning Schild <henning.schild@siemens.com>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note this is not tested on actual hw, since I do not have the hw in
>>>> question ---
>>>>  drivers/leds/simple/simatic-ipc-leds.c | 32
>>>> +++++++++++++++----------- 1 file changed, 18 insertions(+), 14
>>>> deletions(-)
>>>>
>>>> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
>>>> b/drivers/leds/simple/simatic-ipc-leds.c index
>>>> 179110448659..078d43f5ba38 100644 ---
>>>> a/drivers/leds/simple/simatic-ipc-leds.c +++
>>>> b/drivers/leds/simple/simatic-ipc-leds.c @@ -41,7 +41,7 @@ static
>>>> struct simatic_ipc_led simatic_ipc_leds_io[] = { /* the actual
>>>> start will be discovered with PCI, 0 is a placeholder */ static
>>>> struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
>>>> SZ_4K, KBUILD_MODNAME); -static void *simatic_ipc_led_memory;
>>>> +static void __iomem *simatic_ipc_led_memory;
>>>>  
>>>>  static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
>>>>  	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
>>>> @@ -92,21 +92,22 @@ static void simatic_ipc_led_set_mem(struct
>>>> led_classdev *led_cd, enum led_brightness brightness)
>>>>  {
>>>>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>>>> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
>>>> +	u32 val;
>>>>  
>>>> -	u32 *p;
>>>> -
>>>> -	p = simatic_ipc_led_memory + led->value;
>>>> -	*p = (*p & ~1) | (brightness == LED_OFF);
>>>> +	val = readl(reg);
>>>> +	val = (val & ~1) | (brightness == LED_OFF);
>>>> +	writel(val, reg);
>>>>  }
>>>>  
>>>>  static enum led_brightness simatic_ipc_led_get_mem(struct
>>>> led_classdev *led_cd) {
>>>>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>>>> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
>>>> +	u32 val;
>>>>  
>>>> -	u32 *p;
>>>> -
>>>> -	p = simatic_ipc_led_memory + led->value;
>>>> -	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
>>>> +	val = readl(reg);
>>>> +	return (val & 1) ? LED_OFF : led_cd->max_brightness;
>>>>  }
>>>>  
>>>>  static int simatic_ipc_leds_probe(struct platform_device *pdev)
>>>> @@ -116,8 +117,9 @@ static int simatic_ipc_leds_probe(struct
>>>> platform_device *pdev) struct simatic_ipc_led *ipcled;
>>>>  	struct led_classdev *cdev;
>>>>  	struct resource *res;
>>>> +	void __iomem *reg;
>>>>  	int err, type;
>>>> -	u32 *p;
>>>> +	u32 val;
>>>>  
>>>>  	switch (plat->devmode) {
>>>>  	case SIMATIC_IPC_DEVICE_227D:
>>>> @@ -157,11 +159,13 @@ static int simatic_ipc_leds_probe(struct
>>>> platform_device *pdev) return PTR_ERR(simatic_ipc_led_memory);
>>>>  
>>>>  		/* initialize power/watchdog LED */
>>>> -		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>>>> PM_WDT_OUT */
>>>> -		*p = (*p & ~1);
>>>> -		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>>>> PM_BIOS_BOOT_N */
>>>> -		*p = (*p | 1);
>>>> +		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>>>> PM_WDT_OUT */
>>>> +		val = readl(reg);
>>>> +		writel(val & ~1, reg);
>>>>  
>>>> +		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>>>> PM_BIOS_BOOT_N */
>>>> +		val = readl(reg);
>>>> +		writel(val | 1, reg);
>>>>  		break;
>>>>  	default:
>>>>  		return -ENODEV;  
>>>   
>>
> 

Re: [PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Hans de Goede]

Hi All,

Pavel, ping? What is the status of this series?

Regards,

Hans


On 1/28/22 16:26, Henning Schild wrote:
> Am Fri, 28 Jan 2022 10:37:36 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi Henning,
>>
>> On 1/28/22 10:30, Henning Schild wrote:
>>> Hi Hans,
>>>
>>> this looks fine but also looks like someone should test it. What is
>>> the timeline on that? I would want that tested but will need a few
>>> more days to actually sit next to boxes and look at LEDs.  
>>
>> It would be nice to get this fix in before say 5.17-rc5, so testing
>> this a couple of days from now is fine.
> 
> Gerd was so kind to just it done quickly today. He confirms its working
> as expected.
> 
> Thanks guys. I guess you could add a 
> 
> Tested-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> 
> if you want.
> 
> regards,
> Henning
>  
>> Talking about merging this...
>>
>> Pavel, since the original patch has landed through the pdx86 tree
>> in 5.17-rc1 and since these 2 patches are LED patches I was sorta
>> expecting you to pick these up.  But if it is easier for you,
>> I would also happy to make these patches part of a pdx86 fixes
>> pull-req for 5.17 .
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>
>>>
>>> Gerd maybe you can try that? Should affect 127E only and can
>>> probably be applied onto our out-of-tree repo.
>>>
>>> regards,
>>> Henning
>>>
>>> Am Mon, 17 Jan 2022 12:21:09 +0100
>>> schrieb Hans de Goede <hdegoede@redhat.com>:
>>>   
>>>> Sparse (rightly) currently gives the following warning:
>>>>
>>>> drivers/leds/simple/simatic-ipc-leds.c:155:40:
>>>>  sparse: sparse: incorrect type in assignment (different address
>>>> spaces) expected void *static [toplevel] simatic_ipc_led_memory
>>>>  got void [noderef] __iomem *
>>>>
>>>> Fix this by changing the type of simatic_ipc_led_memory to void
>>>> __iomem * and use readl()/writel() to access it.
>>>>
>>>> Cc: Henning Schild <henning.schild@siemens.com>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note this is not tested on actual hw, since I do not have the hw in
>>>> question ---
>>>>  drivers/leds/simple/simatic-ipc-leds.c | 32
>>>> +++++++++++++++----------- 1 file changed, 18 insertions(+), 14
>>>> deletions(-)
>>>>
>>>> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
>>>> b/drivers/leds/simple/simatic-ipc-leds.c index
>>>> 179110448659..078d43f5ba38 100644 ---
>>>> a/drivers/leds/simple/simatic-ipc-leds.c +++
>>>> b/drivers/leds/simple/simatic-ipc-leds.c @@ -41,7 +41,7 @@ static
>>>> struct simatic_ipc_led simatic_ipc_leds_io[] = { /* the actual
>>>> start will be discovered with PCI, 0 is a placeholder */ static
>>>> struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
>>>> SZ_4K, KBUILD_MODNAME); -static void *simatic_ipc_led_memory;
>>>> +static void __iomem *simatic_ipc_led_memory;
>>>>  
>>>>  static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
>>>>  	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
>>>> @@ -92,21 +92,22 @@ static void simatic_ipc_led_set_mem(struct
>>>> led_classdev *led_cd, enum led_brightness brightness)
>>>>  {
>>>>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>>>> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
>>>> +	u32 val;
>>>>  
>>>> -	u32 *p;
>>>> -
>>>> -	p = simatic_ipc_led_memory + led->value;
>>>> -	*p = (*p & ~1) | (brightness == LED_OFF);
>>>> +	val = readl(reg);
>>>> +	val = (val & ~1) | (brightness == LED_OFF);
>>>> +	writel(val, reg);
>>>>  }
>>>>  
>>>>  static enum led_brightness simatic_ipc_led_get_mem(struct
>>>> led_classdev *led_cd) {
>>>>  	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>>>> +	void __iomem *reg = simatic_ipc_led_memory + led->value;
>>>> +	u32 val;
>>>>  
>>>> -	u32 *p;
>>>> -
>>>> -	p = simatic_ipc_led_memory + led->value;
>>>> -	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
>>>> +	val = readl(reg);
>>>> +	return (val & 1) ? LED_OFF : led_cd->max_brightness;
>>>>  }
>>>>  
>>>>  static int simatic_ipc_leds_probe(struct platform_device *pdev)
>>>> @@ -116,8 +117,9 @@ static int simatic_ipc_leds_probe(struct
>>>> platform_device *pdev) struct simatic_ipc_led *ipcled;
>>>>  	struct led_classdev *cdev;
>>>>  	struct resource *res;
>>>> +	void __iomem *reg;
>>>>  	int err, type;
>>>> -	u32 *p;
>>>> +	u32 val;
>>>>  
>>>>  	switch (plat->devmode) {
>>>>  	case SIMATIC_IPC_DEVICE_227D:
>>>> @@ -157,11 +159,13 @@ static int simatic_ipc_leds_probe(struct
>>>> platform_device *pdev) return PTR_ERR(simatic_ipc_led_memory);
>>>>  
>>>>  		/* initialize power/watchdog LED */
>>>> -		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>>>> PM_WDT_OUT */
>>>> -		*p = (*p & ~1);
>>>> -		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>>>> PM_BIOS_BOOT_N */
>>>> -		*p = (*p | 1);
>>>> +		reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>>>> PM_WDT_OUT */
>>>> +		val = readl(reg);
>>>> +		writel(val & ~1, reg);
>>>>  
>>>> +		reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>>>> PM_BIOS_BOOT_N */
>>>> +		val = readl(reg);
>>>> +		writel(val | 1, reg);
>>>>  		break;
>>>>  	default:
>>>>  		return -ENODEV;  
>>>   
>>
> 

Re: [PATCH 2/2] leds: simatic-ipc-leds: Don't directly deref ioremap_resource() returned ptr

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]

Hi!

> Pavel, ping? What is the status of this series?

I applied the series. Thank you,
							Pavel
							
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]