diff mbox series

[v3] bus: mhi: core: Indexed MHI controller name

Message ID 1606493121-12540-1-git-send-email-loic.poulain@linaro.org
State New
Headers show
Series [v3] bus: mhi: core: Indexed MHI controller name | expand

Commit Message

Loic Poulain Nov. 27, 2020, 4:05 p.m. UTC 
Today the MHI controller name is simply cloned from the underlying
bus device (its parent), that gives the following device structure
for e.g. a MHI/PCI controller:
devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
...

That's quite misleading/confusing and can cause device registering
issues because of duplicate dev name (e.g. if a PCI device register
two different MHI instances).

This patch changes MHI core to create indexed mhi controller names
(mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).

The previous example becomes:
devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
...

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 v2: move index field at the end of mhi_controller struct (before bool)
     to avoid breaking well packed alignment.
 v3: Align debugfs directory name

 drivers/bus/mhi/core/debugfs.c |  2 +-
 drivers/bus/mhi/core/init.c    | 18 ++++++++++++++++--
 drivers/bus/mhi/core/main.c    |  2 +-
 include/linux/mhi.h            |  2 ++
 4 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.7.4

Comments

Jeffrey Hugo Nov. 27, 2020, 5:39 p.m. UTC | #1 
On 11/27/2020 9:05 AM, Loic Poulain wrote:
> Today the MHI controller name is simply cloned from the underlying

> bus device (its parent), that gives the following device structure

> for e.g. a MHI/PCI controller:

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR

> ...

> 

> That's quite misleading/confusing and can cause device registering

> issues because of duplicate dev name (e.g. if a PCI device register

> two different MHI instances).

> 

> This patch changes MHI core to create indexed mhi controller names

> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).

> 

> The previous example becomes:

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR

> ...

> 

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>


Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Manivannan Sadhasivam Nov. 28, 2020, 6:19 a.m. UTC | #2 
On Fri, Nov 27, 2020 at 05:05:21PM +0100, Loic Poulain wrote:
> Today the MHI controller name is simply cloned from the underlying

> bus device (its parent), that gives the following device structure

> for e.g. a MHI/PCI controller:

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR

> ...

> 

> That's quite misleading/confusing and can cause device registering

> issues because of duplicate dev name (e.g. if a PCI device register

> two different MHI instances).

> 

> This patch changes MHI core to create indexed mhi controller names

> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).

> 

> The previous example becomes:

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0

> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR

> ...

> 

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> ---

>  v2: move index field at the end of mhi_controller struct (before bool)

>      to avoid breaking well packed alignment.

>  v3: Align debugfs directory name

> 

>  drivers/bus/mhi/core/debugfs.c |  2 +-

>  drivers/bus/mhi/core/init.c    | 18 ++++++++++++++++--

>  drivers/bus/mhi/core/main.c    |  2 +-

>  include/linux/mhi.h            |  2 ++

>  4 files changed, 20 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/bus/mhi/core/debugfs.c b/drivers/bus/mhi/core/debugfs.c

> index 7d43138..858d751 100644

> --- a/drivers/bus/mhi/core/debugfs.c

> +++ b/drivers/bus/mhi/core/debugfs.c

> @@ -377,7 +377,7 @@ static struct dentry *mhi_debugfs_root;

>  void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)

>  {

>  	mhi_cntrl->debugfs_dentry =

> -			debugfs_create_dir(dev_name(mhi_cntrl->cntrl_dev),

> +			debugfs_create_dir(dev_name(&mhi_cntrl->mhi_dev->dev),

>  					   mhi_debugfs_root);


This change doesn't belong to this patch, right? This should be part of
"bus: mhi: core: Fix device hierarchy".

Thanks,
Mani

>  

>  	debugfs_create_file("states", 0444, mhi_cntrl->debugfs_dentry,

> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c

> index c7a7354..4acad28 100644

> --- a/drivers/bus/mhi/core/init.c

> +++ b/drivers/bus/mhi/core/init.c

> @@ -8,6 +8,7 @@

>  #include <linux/device.h>

>  #include <linux/dma-direction.h>

>  #include <linux/dma-mapping.h>

> +#include <linux/idr.h>

>  #include <linux/interrupt.h>

>  #include <linux/list.h>

>  #include <linux/mhi.h>

> @@ -18,6 +19,8 @@

>  #include <linux/wait.h>

>  #include "internal.h"

>  

> +static DEFINE_IDA(mhi_controller_ida);

> +

>  const char * const mhi_ee_str[MHI_EE_MAX] = {

>  	[MHI_EE_PBL] = "PBL",

>  	[MHI_EE_SBL] = "SBL",

> @@ -940,6 +943,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,

>  	mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>

>  					SOC_HW_VERSION_MINOR_VER_SHFT;

>  

> +	mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);

> +	if (mhi_cntrl->index < 0) {

> +		ret = mhi_cntrl->index;

> +		goto error_ida_alloc;

> +	}

> +

>  	/* Register controller with MHI bus */

>  	mhi_dev = mhi_alloc_device(mhi_cntrl);

>  	if (IS_ERR(mhi_dev)) {

> @@ -950,8 +959,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,

>  

>  	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;

>  	mhi_dev->mhi_cntrl = mhi_cntrl;

> -	dev_set_name(&mhi_dev->dev, "%s", dev_name(mhi_cntrl->cntrl_dev));

> -	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);

> +	dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);

> +	mhi_dev->name = dev_name(&mhi_dev->dev);

>  

>  	/* Init wakeup source */

>  	device_init_wakeup(&mhi_dev->dev, true);

> @@ -970,6 +979,9 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,

>  	put_device(&mhi_dev->dev);

>  

>  error_alloc_dev:

> +	ida_free(&mhi_controller_ida, mhi_cntrl->index);

> +

> +error_ida_alloc:

>  	kfree(mhi_cntrl->mhi_cmd);

>  

>  error_alloc_cmd:

> @@ -1004,6 +1016,8 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)

>  

>  	device_del(&mhi_dev->dev);

>  	put_device(&mhi_dev->dev);

> +

> +	ida_free(&mhi_controller_ida, mhi_cntrl->index);

>  }

>  EXPORT_SYMBOL_GPL(mhi_unregister_controller);

>  

> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c

> index 7f3c990..ea39df0 100644

> --- a/drivers/bus/mhi/core/main.c

> +++ b/drivers/bus/mhi/core/main.c

> @@ -350,7 +350,7 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)

>  		/* Channel name is same for both UL and DL */

>  		mhi_dev->name = mhi_chan->name;

>  		dev_set_name(&mhi_dev->dev, "%s_%s",

> -			     dev_name(mhi_cntrl->cntrl_dev),

> +			     dev_name(&mhi_cntrl->mhi_dev->dev),

>  			     mhi_dev->name);

>  

>  		/* Init wakeup source if available */

> diff --git a/include/linux/mhi.h b/include/linux/mhi.h

> index 27078db..30c676d 100644

> --- a/include/linux/mhi.h

> +++ b/include/linux/mhi.h

> @@ -356,6 +356,7 @@ struct mhi_controller_config {

>   * @read_reg: Read a MHI register via the physical link (required)

>   * @write_reg: Write a MHI register via the physical link (required)

>   * @buffer_len: Bounce buffer length

> + * @index: Index of the MHI controller instance

>   * @bounce_buf: Use of bounce buffer

>   * @fbc_download: MHI host needs to do complete image transfer (optional)

>   * @pre_init: MHI host needs to do pre-initialization before power up

> @@ -446,6 +447,7 @@ struct mhi_controller {

>  			  u32 val);

>  

>  	size_t buffer_len;

> +	int index;

>  	bool bounce_buf;

>  	bool fbc_download;

>  	bool pre_init;

> -- 

> 2.7.4

>
diff mbox series

Patch

diff --git a/drivers/bus/mhi/core/debugfs.c b/drivers/bus/mhi/core/debugfs.c
index 7d43138..858d751 100644
--- a/drivers/bus/mhi/core/debugfs.c
+++ b/drivers/bus/mhi/core/debugfs.c
@@ -377,7 +377,7 @@  static struct dentry *mhi_debugfs_root;
 void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
 {
 	mhi_cntrl->debugfs_dentry =
-			debugfs_create_dir(dev_name(mhi_cntrl->cntrl_dev),
+			debugfs_create_dir(dev_name(&mhi_cntrl->mhi_dev->dev),
 					   mhi_debugfs_root);
 
 	debugfs_create_file("states", 0444, mhi_cntrl->debugfs_dentry,
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index c7a7354..4acad28 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -8,6 +8,7 @@ 
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
+#include <linux/idr.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
 #include <linux/mhi.h>
@@ -18,6 +19,8 @@ 
 #include <linux/wait.h>
 #include "internal.h"
 
+static DEFINE_IDA(mhi_controller_ida);
+
 const char * const mhi_ee_str[MHI_EE_MAX] = {
 	[MHI_EE_PBL] = "PBL",
 	[MHI_EE_SBL] = "SBL",
@@ -940,6 +943,12 @@  int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>
 					SOC_HW_VERSION_MINOR_VER_SHFT;
 
+	mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
+	if (mhi_cntrl->index < 0) {
+		ret = mhi_cntrl->index;
+		goto error_ida_alloc;
+	}
+
 	/* Register controller with MHI bus */
 	mhi_dev = mhi_alloc_device(mhi_cntrl);
 	if (IS_ERR(mhi_dev)) {
@@ -950,8 +959,8 @@  int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 
 	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
 	mhi_dev->mhi_cntrl = mhi_cntrl;
-	dev_set_name(&mhi_dev->dev, "%s", dev_name(mhi_cntrl->cntrl_dev));
-	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
+	dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);
+	mhi_dev->name = dev_name(&mhi_dev->dev);
 
 	/* Init wakeup source */
 	device_init_wakeup(&mhi_dev->dev, true);
@@ -970,6 +979,9 @@  int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	put_device(&mhi_dev->dev);
 
 error_alloc_dev:
+	ida_free(&mhi_controller_ida, mhi_cntrl->index);
+
+error_ida_alloc:
 	kfree(mhi_cntrl->mhi_cmd);
 
 error_alloc_cmd:
@@ -1004,6 +1016,8 @@  void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 
 	device_del(&mhi_dev->dev);
 	put_device(&mhi_dev->dev);
+
+	ida_free(&mhi_controller_ida, mhi_cntrl->index);
 }
 EXPORT_SYMBOL_GPL(mhi_unregister_controller);
 
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 7f3c990..ea39df0 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -350,7 +350,7 @@  void mhi_create_devices(struct mhi_controller *mhi_cntrl)
 		/* Channel name is same for both UL and DL */
 		mhi_dev->name = mhi_chan->name;
 		dev_set_name(&mhi_dev->dev, "%s_%s",
-			     dev_name(mhi_cntrl->cntrl_dev),
+			     dev_name(&mhi_cntrl->mhi_dev->dev),
 			     mhi_dev->name);
 
 		/* Init wakeup source if available */
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 27078db..30c676d 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -356,6 +356,7 @@  struct mhi_controller_config {
  * @read_reg: Read a MHI register via the physical link (required)
  * @write_reg: Write a MHI register via the physical link (required)
  * @buffer_len: Bounce buffer length
+ * @index: Index of the MHI controller instance
  * @bounce_buf: Use of bounce buffer
  * @fbc_download: MHI host needs to do complete image transfer (optional)
  * @pre_init: MHI host needs to do pre-initialization before power up
@@ -446,6 +447,7 @@  struct mhi_controller {
 			  u32 val);
 
 	size_t buffer_len;
+	int index;
 	bool bounce_buf;
 	bool fbc_download;
 	bool pre_init;