[RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[David Collins]

Change the format of spmi bus device names from:
  <spmi_bus_number>-<spmi_device_sid>
  Example: 0-01
to this:
  spmi<spmi_bus_number>-<spmi_device_sid>
  Example: spmi0-01

This helps to disambiguate SPMI device regmaps from I2C ones
at /sys/kernel/debug/regmap since I2C devices use a very
similar naming scheme: 0-0000.

Signed-off-by: David Collins <collinsd@codeaurora.org>
---
 drivers/spmi/spmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index c16b60f..ec94439 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2015, 2020, The Linux Foundation. All rights reserved.
  */
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -62,7 +62,7 @@ int spmi_device_add(struct spmi_device *sdev)
 	struct spmi_controller *ctrl = sdev->ctrl;
 	int err;
 
-	dev_set_name(&sdev->dev, "%d-%02x", ctrl->nr, sdev->usid);
+	dev_set_name(&sdev->dev, "spmi%d-%02x", ctrl->nr, sdev->usid);
 
 	err = device_add(&sdev->dev);
 	if (err < 0) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Stephen Boyd]

Quoting David Collins (2020-09-22 15:04:18)
> Change the format of spmi bus device names from:
>   <spmi_bus_number>-<spmi_device_sid>
>   Example: 0-01
> to this:
>   spmi<spmi_bus_number>-<spmi_device_sid>
>   Example: spmi0-01
> 
> This helps to disambiguate SPMI device regmaps from I2C ones
> at /sys/kernel/debug/regmap since I2C devices use a very
> similar naming scheme: 0-0000.

Can regmap debugfs prepend the bus name on the node made in debugfs?
Does it do that already?

> 
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> ---
>  drivers/spmi/spmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index c16b60f..ec94439 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2012-2015, 2020, The Linux Foundation. All rights reserved.
>   */
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> @@ -62,7 +62,7 @@ int spmi_device_add(struct spmi_device *sdev)
>         struct spmi_controller *ctrl = sdev->ctrl;
>         int err;
>  
> -       dev_set_name(&sdev->dev, "%d-%02x", ctrl->nr, sdev->usid);
> +       dev_set_name(&sdev->dev, "spmi%d-%02x", ctrl->nr, sdev->usid);
>  
>         err = device_add(&sdev->dev);
>         if (err < 0) {

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Mark Brown]

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

On Wed, Sep 30, 2020 at 05:07:20PM -0700, Stephen Boyd wrote:
> Quoting David Collins (2020-09-22 15:04:18)

> > This helps to disambiguate SPMI device regmaps from I2C ones
> > at /sys/kernel/debug/regmap since I2C devices use a very
> > similar naming scheme: 0-0000.

> Can regmap debugfs prepend the bus name on the node made in debugfs?
> Does it do that already?

It doesn't do that.  I have to say that given the use of dev_name() in
logging it does feel like it'd be useful to have distinct names for
grepping if we're running into collisions, IIRC the reason I went with
dev_name() was that it's a commonly used human readable handle for
diagnostic infrastrucuture so it makes it easier to follow things around.

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

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Stephen Boyd]

Quoting Mark Brown (2020-10-01 10:43:26)
> On Wed, Sep 30, 2020 at 05:07:20PM -0700, Stephen Boyd wrote:
> > Quoting David Collins (2020-09-22 15:04:18)
> 
> > > This helps to disambiguate SPMI device regmaps from I2C ones
> > > at /sys/kernel/debug/regmap since I2C devices use a very
> > > similar naming scheme: 0-0000.
> 
> > Can regmap debugfs prepend the bus name on the node made in debugfs?
> > Does it do that already?
> 
> It doesn't do that.  I have to say that given the use of dev_name() in
> logging it does feel like it'd be useful to have distinct names for
> grepping if we're running into collisions, IIRC the reason I went with
> dev_name() was that it's a commonly used human readable handle for
> diagnostic infrastrucuture so it makes it easier to follow things around.

To me the dev_name() usage seems fine. Maybe David has some real reason
to change this though?

In general I don't think userspace cares what the SPMI device name is,
i.e. the device name isn't used for dev nodes because SPMI doesn't
support ioctls or read/write APIs on the bus. That could be a nice
feature addition though, to support something like i2c-dev.

Changing it so that regmap debugfs is less likely to collide looks
weird. It doesn't actually collide anyway, so it seems like we're adding
spmi prefix to make it easier to find in debugfs?

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[David Collins]

On 10/1/20 11:51 AM, Stephen Boyd wrote:
> Quoting Mark Brown (2020-10-01 10:43:26)
>> On Wed, Sep 30, 2020 at 05:07:20PM -0700, Stephen Boyd wrote:
>>> Quoting David Collins (2020-09-22 15:04:18)
>>
>>>> This helps to disambiguate SPMI device regmaps from I2C ones
>>>> at /sys/kernel/debug/regmap since I2C devices use a very
>>>> similar naming scheme: 0-0000.
>>
>>> Can regmap debugfs prepend the bus name on the node made in debugfs?
>>> Does it do that already?
>>
>> It doesn't do that.  I have to say that given the use of dev_name() in
>> logging it does feel like it'd be useful to have distinct names for
>> grepping if we're running into collisions, IIRC the reason I went with
>> dev_name() was that it's a commonly used human readable handle for
>> diagnostic infrastrucuture so it makes it easier to follow things around.
> 
> To me the dev_name() usage seems fine. Maybe David has some real reason
> to change this though?
> 
> In general I don't think userspace cares what the SPMI device name is,
> i.e. the device name isn't used for dev nodes because SPMI doesn't
> support ioctls or read/write APIs on the bus. That could be a nice
> feature addition though, to support something like i2c-dev.
> 
> Changing it so that regmap debugfs is less likely to collide looks
> weird. It doesn't actually collide anyway, so it seems like we're adding
> spmi prefix to make it easier to find in debugfs?

Yes, that is correct.  There isn't a collision since I2C uses 0-0000 and
SPMI uses 0-00 naming scheme.  However, those names are very similar and
it is hard for a user to tell which is which inside
/sys/kernel/debug/regmap without a deep understanding of the I2C and SPMI
code.

The SPMI regmap debugfs files are used extensively for testing and debug
purposes internally at Qualcomm and by our customers.  It would be helpful
if the more verbose naming scheme were accepted upstream to avoid
confusion and broken test scripts.

Thanks,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Mark Brown]

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

On Thu, Oct 01, 2020 at 05:45:00PM -0700, David Collins wrote:

> The SPMI regmap debugfs files are used extensively for testing and debug
> purposes internally at Qualcomm and by our customers.  It would be helpful
> if the more verbose naming scheme were accepted upstream to avoid
> confusion and broken test scripts.

...and doing this in the dev_name() should help other diagnostic users
(like dev_printk() for example).

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

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Stephen Boyd]

Quoting Mark Brown (2020-10-02 09:03:24)
> On Thu, Oct 01, 2020 at 05:45:00PM -0700, David Collins wrote:
> 
> > The SPMI regmap debugfs files are used extensively for testing and debug
> > purposes internally at Qualcomm and by our customers.  It would be helpful
> > if the more verbose naming scheme were accepted upstream to avoid
> > confusion and broken test scripts.
> 
> ...and doing this in the dev_name() should help other diagnostic users
> (like dev_printk() for example).

Don't thinks like dev_printk() prefix the bus name? See
dev_driver_string()? So I agree that having the bus name is useful, but
confused why there are testing scripts and things on top of regmap
debugfs

Put another way, why not introduce something similar to i2c-dev where
userspace can read/write registers for devices on the SPMI bus?
Otherwise I presume the test scripts inside Qualcomm are just reading
registers out of regmap?

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Mark Brown]

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

On Fri, Oct 02, 2020 at 10:48:32AM -0700, Stephen Boyd wrote:
> Quoting Mark Brown (2020-10-02 09:03:24)

> > ...and doing this in the dev_name() should help other diagnostic users
> > (like dev_printk() for example).

> Don't thinks like dev_printk() prefix the bus name? See
> dev_driver_string()? So I agree that having the bus name is useful, but
> confused why there are testing scripts and things on top of regmap
> debugfs

Not that I've ever noticed, eg on the console.

> Put another way, why not introduce something similar to i2c-dev where
> userspace can read/write registers for devices on the SPMI bus?
> Otherwise I presume the test scripts inside Qualcomm are just reading
> registers out of regmap?

I know some other vendors use the regmap debugfs for their diagnostic
tools (obviously not with SPMI).  It's generally so they can get the
benefit of the cache, it's a combination of allowing the state to be
inspected while the driver has the device powered down and for devices
on slower buses being much more performant.

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

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[Stephen Boyd]

Quoting Mark Brown (2020-10-02 11:04:30)
> On Fri, Oct 02, 2020 at 10:48:32AM -0700, Stephen Boyd wrote:
> > Quoting Mark Brown (2020-10-02 09:03:24)
> 
> > > ...and doing this in the dev_name() should help other diagnostic users
> > > (like dev_printk() for example).
> 
> > Don't thinks like dev_printk() prefix the bus name? See
> > dev_driver_string()? So I agree that having the bus name is useful, but
> > confused why there are testing scripts and things on top of regmap
> > debugfs
> 
> Not that I've ever noticed, eg on the console.

I see things like this on my console:

[    1.684617] spmi spmi-0: PMIC arbiter version v5 (0x50000000)

and 'spmi' is the bus name I'm thinking about. But I think that's
because there isn't a driver attached. Nothing prints for the 0-00
device by default, so I enabled the debug print for it and I see

[    1.693280] pmic-spmi 0-00: 28: unknown v2.0

Anyway, the device name was written to follow i2c as far as I can tell.

If scripts, i.e. computers, have a hard time figuring out the name of
the device then fix the script?

Re: [RESEND PATCH] spmi: prefix spmi bus device names with "spmi"

[David Collins]

On 10/2/20 2:39 PM, Stephen Boyd wrote:
> I see things like this on my console:
> 
> [    1.684617] spmi spmi-0: PMIC arbiter version v5 (0x50000000)
> 
> and 'spmi' is the bus name I'm thinking about. But I think that's
> because there isn't a driver attached. Nothing prints for the 0-00
> device by default, so I enabled the debug print for it and I see
> 
> [    1.693280] pmic-spmi 0-00: 28: unknown v2.0
> 
> Anyway, the device name was written to follow i2c as far as I can tell.
> 
> If scripts, i.e. computers, have a hard time figuring out the name of
> the device then fix the script?

I agree that we can drop this patch.  There is no technical requirement
for the spmi device naming scheme to be changed.  We will update our
downstream test scripts to use the upstream naming scheme and also
socialize the naming difference internally.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project