On 11/25/2020 8:43 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@xxxxxxxxxx>
Based on various discussions, I have no objections. I think now is a
good time to sort out this ABI. However I do have one nit below.
---
drivers/bus/mhi/core/init.c | 16 +++++++++++++++-
drivers/bus/mhi/core/main.c | 2 +-
include/linux/mhi.h | 2 ++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index c7a7354..ecfffb0 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,7 +959,7 @@ 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));
+ dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);
mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
/* Init wakeup source */
@@ -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 188501c0..4818f42 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -349,7 +349,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..2a89533 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -297,6 +297,7 @@ struct mhi_controller_config {
* @cntrl_dev: Pointer to the struct device of physical bus acting as the MHI
* controller (required)
* @mhi_dev: MHI device instance for the controller
+ * @index: Index of the MHI controller instance
* @debugfs_dentry: MHI controller debugfs directory
* @regs: Base address of MHI MMIO register space (required)
* @bhi: Points to base of MHI BHI register space
@@ -377,6 +378,7 @@ struct mhi_controller_config {
struct mhi_controller {
struct device *cntrl_dev;
struct mhi_device *mhi_dev;
+ int index;
struct dentry *debugfs_dentry;
void __iomem *regs;
void __iomem *bhi;
Is there a good reason to have this in the middle of the struct? Seems
like it would cause some padding because its a 32-bit value in the
middle of a series of 64-bit values.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
