Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] mfd: syscon: Use a unique name with regmap_config
@ 2020-01-27 23:12 Suman Anna
  2020-01-27 23:40 ` David Lechner
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Suman Anna @ 2020-01-27 23:12 UTC (permalink / raw)
  To: Lee Jones
  Cc: Arnd Bergmann, David Lechner, Tony Lindgren, Roger Quadros,
	linux-kernel, linux-arm-kernel, linux-omap, Suman Anna

The DT node full name is currently being used in regmap_config
which in turn is used to create the regmap debugfs directories.
This name however is not guaranteed to be unique and the regmap
debugfs registration can fail in the cases where the syscon nodes
have the same unit-address but are present in different DT node
hierarchies. Replace this logic using the syscon reg resource
address instead (inspired from logic used while creating platform
devices) to ensure a unique name is given for each syscon.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
v2: Fix build warning reported by kbuild test bot
v1: https://patchwork.kernel.org/patch/11346363/

 drivers/mfd/syscon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index e22197c832e8..f0815d8e6e95 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -101,12 +101,14 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 		}
 	}
 
-	syscon_config.name = of_node_full_name(np);
+	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
+				       (u64)res.start);
 	syscon_config.reg_stride = reg_io_width;
 	syscon_config.val_bits = reg_io_width * 8;
 	syscon_config.max_register = resource_size(&res) - reg_io_width;
 
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
+	kfree(syscon_config.name);
 	if (IS_ERR(regmap)) {
 		pr_err("regmap init failed\n");
 		ret = PTR_ERR(regmap);
-- 
2.23.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-01-27 23:12 [PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna
@ 2020-01-27 23:40 ` David Lechner
  2020-01-28  0:27   ` Suman Anna
  2020-01-30 15:34 ` Andy Shevchenko
  2020-02-24 10:00 ` Lee Jones
  2 siblings, 1 reply; 9+ messages in thread
From: David Lechner @ 2020-01-27 23:40 UTC (permalink / raw)
  To: Suman Anna, Lee Jones
  Cc: Arnd Bergmann, Tony Lindgren, Roger Quadros, linux-kernel,
	linux-arm-kernel, linux-omap

On 1/27/20 5:12 PM, Suman Anna wrote:
> The DT node full name is currently being used in regmap_config
> which in turn is used to create the regmap debugfs directories.
> This name however is not guaranteed to be unique and the regmap
> debugfs registration can fail in the cases where the syscon nodes
> have the same unit-address but are present in different DT node
> hierarchies. Replace this logic using the syscon reg resource
> address instead (inspired from logic used while creating platform
> devices) to ensure a unique name is given for each syscon.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Fix build warning reported by kbuild test bot
> v1: https://patchwork.kernel.org/patch/11346363/
> 
>   drivers/mfd/syscon.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index e22197c832e8..f0815d8e6e95 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -101,12 +101,14 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>   		}
>   	}
>   
> -	syscon_config.name = of_node_full_name(np);
> +	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
> +				       (u64)res.start);

Would it make sense to also include the node name along with the
pointer address so that the name is still easily identifiable?

>   	syscon_config.reg_stride = reg_io_width;
>   	syscon_config.val_bits = reg_io_width * 8;
>   	syscon_config.max_register = resource_size(&res) - reg_io_width;
>   
>   	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> +	kfree(syscon_config.name);
>   	if (IS_ERR(regmap)) {
>   		pr_err("regmap init failed\n");
>   		ret = PTR_ERR(regmap);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-01-27 23:40 ` David Lechner
@ 2020-01-28  0:27   ` Suman Anna
  0 siblings, 0 replies; 9+ messages in thread
From: Suman Anna @ 2020-01-28  0:27 UTC (permalink / raw)
  To: David Lechner, Lee Jones
  Cc: Arnd Bergmann, Tony Lindgren, Roger Quadros, linux-kernel,
	linux-arm-kernel, linux-omap

Hi David,

On 1/27/20 5:40 PM, David Lechner wrote:
> On 1/27/20 5:12 PM, Suman Anna wrote:
>> The DT node full name is currently being used in regmap_config
>> which in turn is used to create the regmap debugfs directories.
>> This name however is not guaranteed to be unique and the regmap
>> debugfs registration can fail in the cases where the syscon nodes
>> have the same unit-address but are present in different DT node
>> hierarchies. Replace this logic using the syscon reg resource
>> address instead (inspired from logic used while creating platform
>> devices) to ensure a unique name is given for each syscon.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> v2: Fix build warning reported by kbuild test bot
>> v1: https://patchwork.kernel.org/patch/11346363/
>>
>>   drivers/mfd/syscon.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>> index e22197c832e8..f0815d8e6e95 100644
>> --- a/drivers/mfd/syscon.c
>> +++ b/drivers/mfd/syscon.c
>> @@ -101,12 +101,14 @@ static struct syscon *of_syscon_register(struct
>> device_node *np, bool check_clk)
>>           }
>>       }
>>   -    syscon_config.name = of_node_full_name(np);
>> +    syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
>> +                       (u64)res.start);
> 
> Would it make sense to also include the node name along with the
> pointer address so that the name is still easily identifiable?

I haven't dropped the node name, it is still there, the pOFn part. I am
only replacing the DT unit-address with the bus address, I haven't
changed the name style either.

regards
Suman

> 
>>       syscon_config.reg_stride = reg_io_width;
>>       syscon_config.val_bits = reg_io_width * 8;
>>       syscon_config.max_register = resource_size(&res) - reg_io_width;
>>         regmap = regmap_init_mmio(NULL, base, &syscon_config);
>> +    kfree(syscon_config.name);
>>       if (IS_ERR(regmap)) {
>>           pr_err("regmap init failed\n");
>>           ret = PTR_ERR(regmap);
>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-01-27 23:12 [PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna
  2020-01-27 23:40 ` David Lechner
@ 2020-01-30 15:34 ` Andy Shevchenko
  2020-01-30 17:08   ` Suman Anna
  2020-02-24 10:00 ` Lee Jones
  2 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-30 15:34 UTC (permalink / raw)
  To: Suman Anna
  Cc: Lee Jones, Arnd Bergmann, David Lechner, Tony Lindgren,
	Roger Quadros, Linux Kernel Mailing List, linux-arm Mailing List,
	Linux OMAP Mailing List

On Tue, Jan 28, 2020 at 1:14 AM Suman Anna <s-anna@ti.com> wrote:
>
> The DT node full name is currently being used in regmap_config
> which in turn is used to create the regmap debugfs directories.
> This name however is not guaranteed to be unique and the regmap
> debugfs registration can fail in the cases where the syscon nodes
> have the same unit-address but are present in different DT node
> hierarchies. Replace this logic using the syscon reg resource
> address instead (inspired from logic used while creating platform
> devices) to ensure a unique name is given for each syscon.

> -       syscon_config.name = of_node_full_name(np);
> +       syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
> +                                      (u64)res.start);

Explicit castings in printf() usually tell us that something is not okay.
Yes, for resource_size_t we have %pa.

On top of that, I would rather see %pfwn to avoid modification for
other fwnode types.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-01-30 15:34 ` Andy Shevchenko
@ 2020-01-30 17:08   ` Suman Anna
  2020-01-30 20:43     ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Suman Anna @ 2020-01-30 17:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Lee Jones, Arnd Bergmann, David Lechner, Tony Lindgren,
	Roger Quadros, Linux Kernel Mailing List, linux-arm Mailing List,
	Linux OMAP Mailing List

Hi Andy,

On 1/30/20 9:34 AM, Andy Shevchenko wrote:
> On Tue, Jan 28, 2020 at 1:14 AM Suman Anna <s-anna@ti.com> wrote:
>>
>> The DT node full name is currently being used in regmap_config
>> which in turn is used to create the regmap debugfs directories.
>> This name however is not guaranteed to be unique and the regmap
>> debugfs registration can fail in the cases where the syscon nodes
>> have the same unit-address but are present in different DT node
>> hierarchies. Replace this logic using the syscon reg resource
>> address instead (inspired from logic used while creating platform
>> devices) to ensure a unique name is given for each syscon.
> 
>> -       syscon_config.name = of_node_full_name(np);
>> +       syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
>> +                                      (u64)res.start);
> 
> Explicit castings in printf() usually tell us that something is not okay.

Yes, I agree in general.

> Yes, for resource_size_t we have %pa.

And that was the first thing I tried when doing v2, before moving away
from it. This is not for a console printf statement, but is rather for
the regmap debugfs name. Using a %pa adds the 0x and leading zeros in
the debugfs name, when compared to the name before this patch. The
typecast retains the current format, and replaces the unit-address
without the leading 0s either. Introducing a local-variable to avoid the
typecast is overkill.

> 
> On top of that, I would rather see %pfwn to avoid modification for
> other fwnode types.

Did you mean %pfwP? That can probably be handled when syscon code is
updated to use fwnode API.

regards
Suman

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-01-30 17:08   ` Suman Anna
@ 2020-01-30 20:43     ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-30 20:43 UTC (permalink / raw)
  To: Suman Anna
  Cc: Lee Jones, Arnd Bergmann, David Lechner, Tony Lindgren,
	Roger Quadros, Linux Kernel Mailing List, linux-arm Mailing List,
	Linux OMAP Mailing List

On Thu, Jan 30, 2020 at 7:09 PM Suman Anna <s-anna@ti.com> wrote:
> On 1/30/20 9:34 AM, Andy Shevchenko wrote:
> > On Tue, Jan 28, 2020 at 1:14 AM Suman Anna <s-anna@ti.com> wrote:

...

> >> -       syscon_config.name = of_node_full_name(np);
> >> +       syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
> >> +                                      (u64)res.start);
> >
> > Explicit castings in printf() usually tell us that something is not okay.
>
> Yes, I agree in general.
>
> > Yes, for resource_size_t we have %pa.
>
> And that was the first thing I tried when doing v2, before moving away
> from it. This is not for a console printf statement, but is rather for
> the regmap debugfs name. Using a %pa adds the 0x and leading zeros in
> the debugfs name, when compared to the name before this patch. The
> typecast retains the current format, and replaces the unit-address
> without the leading 0s either. Introducing a local-variable to avoid the
> typecast is overkill.

Thanks for the clarification.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-01-27 23:12 [PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna
  2020-01-27 23:40 ` David Lechner
  2020-01-30 15:34 ` Andy Shevchenko
@ 2020-02-24 10:00 ` Lee Jones
  2020-03-06  0:29   ` Suman Anna
  2020-07-24 16:06   ` Lee Jones
  2 siblings, 2 replies; 9+ messages in thread
From: Lee Jones @ 2020-02-24 10:00 UTC (permalink / raw)
  To: Suman Anna
  Cc: Arnd Bergmann, David Lechner, Tony Lindgren, Roger Quadros,
	linux-kernel, linux-arm-kernel, linux-omap

On Mon, 27 Jan 2020, Suman Anna wrote:

> The DT node full name is currently being used in regmap_config
> which in turn is used to create the regmap debugfs directories.
> This name however is not guaranteed to be unique and the regmap
> debugfs registration can fail in the cases where the syscon nodes
> have the same unit-address but are present in different DT node
> hierarchies. Replace this logic using the syscon reg resource
> address instead (inspired from logic used while creating platform
> devices) to ensure a unique name is given for each syscon.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Fix build warning reported by kbuild test bot
> v1: https://patchwork.kernel.org/patch/11346363/
> 
>  drivers/mfd/syscon.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Waiting for Arnd to review.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-02-24 10:00 ` Lee Jones
@ 2020-03-06  0:29   ` Suman Anna
  2020-07-24 16:06   ` Lee Jones
  1 sibling, 0 replies; 9+ messages in thread
From: Suman Anna @ 2020-03-06  0:29 UTC (permalink / raw)
  To: Lee Jones, Arnd Bergmann
  Cc: David Lechner, Tony Lindgren, Roger Quadros, linux-kernel,
	linux-arm-kernel, linux-omap

On 2/24/20 4:00 AM, Lee Jones wrote:
> On Mon, 27 Jan 2020, Suman Anna wrote:
> 
>> The DT node full name is currently being used in regmap_config
>> which in turn is used to create the regmap debugfs directories.
>> This name however is not guaranteed to be unique and the regmap
>> debugfs registration can fail in the cases where the syscon nodes
>> have the same unit-address but are present in different DT node
>> hierarchies. Replace this logic using the syscon reg resource
>> address instead (inspired from logic used while creating platform
>> devices) to ensure a unique name is given for each syscon.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> v2: Fix build warning reported by kbuild test bot
>> v1: https://patchwork.kernel.org/patch/11346363/
>>
>>  drivers/mfd/syscon.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Waiting for Arnd to review.
> 

Hi Arnd,

Gentle ping, any comments?

regards
Suman


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mfd: syscon: Use a unique name with regmap_config
  2020-02-24 10:00 ` Lee Jones
  2020-03-06  0:29   ` Suman Anna
@ 2020-07-24 16:06   ` Lee Jones
  1 sibling, 0 replies; 9+ messages in thread
From: Lee Jones @ 2020-07-24 16:06 UTC (permalink / raw)
  To: Suman Anna
  Cc: Arnd Bergmann, David Lechner, Tony Lindgren, Roger Quadros,
	linux-kernel, linux-arm-kernel, linux-omap

On Mon, 24 Feb 2020, Lee Jones wrote:

> On Mon, 27 Jan 2020, Suman Anna wrote:
> 
> > The DT node full name is currently being used in regmap_config
> > which in turn is used to create the regmap debugfs directories.
> > This name however is not guaranteed to be unique and the regmap
> > debugfs registration can fail in the cases where the syscon nodes
> > have the same unit-address but are present in different DT node
> > hierarchies. Replace this logic using the syscon reg resource
> > address instead (inspired from logic used while creating platform
> > devices) to ensure a unique name is given for each syscon.
> > 
> > Signed-off-by: Suman Anna <s-anna@ti.com>
> > ---
> > v2: Fix build warning reported by kbuild test bot
> > v1: https://patchwork.kernel.org/patch/11346363/
> > 
> >  drivers/mfd/syscon.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Waiting for Arnd to review.

Still waiting for some input from Arnd.

Might be worth submitting a [RESEND].

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-27 23:12 [PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna
2020-01-27 23:40 ` David Lechner
2020-01-28  0:27   ` Suman Anna
2020-01-30 15:34 ` Andy Shevchenko
2020-01-30 17:08   ` Suman Anna
2020-01-30 20:43     ` Andy Shevchenko
2020-02-24 10:00 ` Lee Jones
2020-03-06  0:29   ` Suman Anna
2020-07-24 16:06   ` Lee Jones

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git