| Message ID | 1600812258-17722-1-git-send-email-collinsd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] spmi: prefix spmi bus device names with "spmi" | expand |
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) {
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.
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?
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
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).
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?
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.
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?
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
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) {
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(-)