* [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs
@ 2023-05-22 7:03 Rajat Khandelwal
2023-05-22 7:37 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Rajat Khandelwal @ 2023-05-22 7:03 UTC (permalink / raw)
To: heikki.krogerus, gregkh; +Cc: linux-usb, linux-kernel, Rajat Khandelwal
IOM status has a crucial role during debugging to check the
current state of the type-C port.
There are ways to fetch the status, but all those require the
IOM port status offset, which could change with platform.
Make a debugfs directory for intel_pmc_mux and expose the status
under it per port basis.
Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
v4:
1. Maintain a root directory for PMC module and incorporate devices
under it
2. Add the debugfs module under '/sys/kernel/debug/usb'
3. Use the platform device 'pmc->dev' to assign the device's name
v3: Allocate the debugfs directory name for the platform device with
its ACPI dev name included
v2:
1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
2. Remove explicitly defined one-liner functions
drivers/usb/typec/mux/intel_pmc_mux.c | 54 ++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 34e4188a40ff..f400094c76f2 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -15,6 +15,8 @@
#include <linux/usb/typec_mux.h>
#include <linux/usb/typec_dp.h>
#include <linux/usb/typec_tbt.h>
+#include <linux/debugfs.h>
+#include <linux/usb.h>
#include <asm/intel_scu_ipc.h>
@@ -143,8 +145,14 @@ struct pmc_usb {
struct acpi_device *iom_adev;
void __iomem *iom_base;
u32 iom_port_status_offset;
+
+#ifdef CONFIG_DEBUG_FS
+ struct dentry *dentry;
+#endif
};
+static struct dentry *pmc_mux_debugfs_root;
+
static void update_port_status(struct pmc_usb_port *port)
{
u8 port_num;
@@ -639,6 +647,29 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
return 0;
}
+static int port_iom_status_show(struct seq_file *s, void *unused)
+{
+ struct pmc_usb_port *port = s->private;
+
+ update_port_status(port);
+ seq_printf(s, "0x%08x\n", port->iom_status);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(port_iom_status);
+
+static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port)
+{
+ struct dentry *debugfs_dir;
+ char name[6];
+
+ snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
+
+ debugfs_dir = debugfs_create_dir(name, port->pmc->dentry);
+ debugfs_create_file("iom_status", 0400, debugfs_dir, port,
+ &port_iom_status_fops);
+}
+
static int pmc_usb_probe(struct platform_device *pdev)
{
struct fwnode_handle *fwnode = NULL;
@@ -674,6 +705,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
if (ret)
return ret;
+ pmc->dentry = debugfs_create_dir(dev_name(pmc->dev), pmc_mux_debugfs_root);
+
/*
* For every physical USB connector (USB2 and USB3 combo) there is a
* child ACPI device node under the PMC mux ACPI device object.
@@ -688,6 +721,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
fwnode_handle_put(fwnode);
goto err_remove_ports;
}
+
+ pmc_mux_port_debugfs_init(&pmc->port[i]);
}
platform_set_drvdata(pdev, pmc);
@@ -703,6 +738,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
acpi_dev_put(pmc->iom_adev);
+ debugfs_remove(pmc->dentry);
+
return ret;
}
@@ -719,6 +756,8 @@ static int pmc_usb_remove(struct platform_device *pdev)
acpi_dev_put(pmc->iom_adev);
+ debugfs_remove(pmc->dentry);
+
return 0;
}
@@ -737,7 +776,20 @@ static struct platform_driver pmc_usb_driver = {
.remove = pmc_usb_remove,
};
-module_platform_driver(pmc_usb_driver);
+static int __init pmc_usb_init(void)
+{
+ pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", usb_debug_root);
+
+ return platform_driver_register(&pmc_usb_driver);
+}
+module_init(pmc_usb_init);
+
+static void __exit pmc_usb_exit(void)
+{
+ platform_driver_unregister(&pmc_usb_driver);
+ debugfs_remove(pmc_mux_debugfs_root);
+}
+module_exit(pmc_usb_exit);
MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
MODULE_LICENSE("GPL v2");
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs
2023-05-22 7:03 [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs Rajat Khandelwal
@ 2023-05-22 7:37 ` Greg KH
2023-05-22 9:54 ` Rajat Khandelwal
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2023-05-22 7:37 UTC (permalink / raw)
To: Rajat Khandelwal; +Cc: heikki.krogerus, linux-usb, linux-kernel
On Mon, May 22, 2023 at 12:33:43PM +0530, Rajat Khandelwal wrote:
> IOM status has a crucial role during debugging to check the
> current state of the type-C port.
> There are ways to fetch the status, but all those require the
> IOM port status offset, which could change with platform.
>
> Make a debugfs directory for intel_pmc_mux and expose the status
> under it per port basis.
>
> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>
> v4:
> 1. Maintain a root directory for PMC module and incorporate devices
> under it
> 2. Add the debugfs module under '/sys/kernel/debug/usb'
> 3. Use the platform device 'pmc->dev' to assign the device's name
>
> v3: Allocate the debugfs directory name for the platform device with
> its ACPI dev name included
>
> v2:
> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> 2. Remove explicitly defined one-liner functions
>
> drivers/usb/typec/mux/intel_pmc_mux.c | 54 ++++++++++++++++++++++++++-
> 1 file changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 34e4188a40ff..f400094c76f2 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -15,6 +15,8 @@
> #include <linux/usb/typec_mux.h>
> #include <linux/usb/typec_dp.h>
> #include <linux/usb/typec_tbt.h>
> +#include <linux/debugfs.h>
> +#include <linux/usb.h>
>
> #include <asm/intel_scu_ipc.h>
>
> @@ -143,8 +145,14 @@ struct pmc_usb {
> struct acpi_device *iom_adev;
> void __iomem *iom_base;
> u32 iom_port_status_offset;
> +
> +#ifdef CONFIG_DEBUG_FS
> + struct dentry *dentry;
> +#endif
No need for the #ifdef anymore, right? In fact, I think it will break
the build if you have it this way and CONFIG_DEBUG_FS is not enabled,
right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs
2023-05-22 7:37 ` Greg KH
@ 2023-05-22 9:54 ` Rajat Khandelwal
2023-05-22 16:26 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Rajat Khandelwal @ 2023-05-22 9:54 UTC (permalink / raw)
To: Greg KH; +Cc: heikki.krogerus, linux-usb, linux-kernel
Hi,
On 5/22/2023 1:07 PM, Greg KH wrote:
> On Mon, May 22, 2023 at 12:33:43PM +0530, Rajat Khandelwal wrote:
>> IOM status has a crucial role during debugging to check the
>> current state of the type-C port.
>> There are ways to fetch the status, but all those require the
>> IOM port status offset, which could change with platform.
>>
>> Make a debugfs directory for intel_pmc_mux and expose the status
>> under it per port basis.
>>
>> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> ---
>>
>> v4:
>> 1. Maintain a root directory for PMC module and incorporate devices
>> under it
>> 2. Add the debugfs module under '/sys/kernel/debug/usb'
>> 3. Use the platform device 'pmc->dev' to assign the device's name
>>
>> v3: Allocate the debugfs directory name for the platform device with
>> its ACPI dev name included
>>
>> v2:
>> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
>> 2. Remove explicitly defined one-liner functions
>>
>> drivers/usb/typec/mux/intel_pmc_mux.c | 54 ++++++++++++++++++++++++++-
>> 1 file changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
>> index 34e4188a40ff..f400094c76f2 100644
>> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
>> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
>> @@ -15,6 +15,8 @@
>> #include <linux/usb/typec_mux.h>
>> #include <linux/usb/typec_dp.h>
>> #include <linux/usb/typec_tbt.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/usb.h>
>>
>> #include <asm/intel_scu_ipc.h>
>>
>> @@ -143,8 +145,14 @@ struct pmc_usb {
>> struct acpi_device *iom_adev;
>> void __iomem *iom_base;
>> u32 iom_port_status_offset;
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> + struct dentry *dentry;
>> +#endif
> No need for the #ifdef anymore, right? In fact, I think it will break
> the build if you have it this way and CONFIG_DEBUG_FS is not enabled,
> right?
I guess you're right. Maybe it'd have been fine if the rest of the
debugfs stuff was also enclosed within the conditional macros.
Anyways, removing it seems appropriate now.
Also, is it OK to send you v5 on the public list directly?
Thanks
Rajat
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs
2023-05-22 9:54 ` Rajat Khandelwal
@ 2023-05-22 16:26 ` Greg KH
2023-05-23 7:43 ` Rajat Khandelwal
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2023-05-22 16:26 UTC (permalink / raw)
To: Rajat Khandelwal; +Cc: heikki.krogerus, linux-usb, linux-kernel
On Mon, May 22, 2023 at 03:24:13PM +0530, Rajat Khandelwal wrote:
> Hi,
>
> On 5/22/2023 1:07 PM, Greg KH wrote:
> > On Mon, May 22, 2023 at 12:33:43PM +0530, Rajat Khandelwal wrote:
> > > IOM status has a crucial role during debugging to check the
> > > current state of the type-C port.
> > > There are ways to fetch the status, but all those require the
> > > IOM port status offset, which could change with platform.
> > >
> > > Make a debugfs directory for intel_pmc_mux and expose the status
> > > under it per port basis.
> > >
> > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >
> > > v4:
> > > 1. Maintain a root directory for PMC module and incorporate devices
> > > under it
> > > 2. Add the debugfs module under '/sys/kernel/debug/usb'
> > > 3. Use the platform device 'pmc->dev' to assign the device's name
> > >
> > > v3: Allocate the debugfs directory name for the platform device with
> > > its ACPI dev name included
> > >
> > > v2:
> > > 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> > > 2. Remove explicitly defined one-liner functions
> > >
> > > drivers/usb/typec/mux/intel_pmc_mux.c | 54 ++++++++++++++++++++++++++-
> > > 1 file changed, 53 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > index 34e4188a40ff..f400094c76f2 100644
> > > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > @@ -15,6 +15,8 @@
> > > #include <linux/usb/typec_mux.h>
> > > #include <linux/usb/typec_dp.h>
> > > #include <linux/usb/typec_tbt.h>
> > > +#include <linux/debugfs.h>
> > > +#include <linux/usb.h>
> > > #include <asm/intel_scu_ipc.h>
> > > @@ -143,8 +145,14 @@ struct pmc_usb {
> > > struct acpi_device *iom_adev;
> > > void __iomem *iom_base;
> > > u32 iom_port_status_offset;
> > > +
> > > +#ifdef CONFIG_DEBUG_FS
> > > + struct dentry *dentry;
> > > +#endif
> > No need for the #ifdef anymore, right? In fact, I think it will break
> > the build if you have it this way and CONFIG_DEBUG_FS is not enabled,
> > right?
>
> I guess you're right. Maybe it'd have been fine if the rest of the
> debugfs stuff was also enclosed within the conditional macros.
Which is not needed and not good kernel coding style, so it's right that
they are not there.
> Anyways, removing it seems appropriate now.
>
> Also, is it OK to send you v5 on the public list directly?
Where else would you send it?
confused,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs
2023-05-22 16:26 ` Greg KH
@ 2023-05-23 7:43 ` Rajat Khandelwal
2023-05-23 7:46 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Rajat Khandelwal @ 2023-05-23 7:43 UTC (permalink / raw)
To: Greg KH; +Cc: heikki.krogerus, linux-usb, linux-kernel
Hi,
On 5/22/2023 9:56 PM, Greg KH wrote:
> On Mon, May 22, 2023 at 03:24:13PM +0530, Rajat Khandelwal wrote:
>> Hi,
>>
>> On 5/22/2023 1:07 PM, Greg KH wrote:
>>> On Mon, May 22, 2023 at 12:33:43PM +0530, Rajat Khandelwal wrote:
>>>> IOM status has a crucial role during debugging to check the
>>>> current state of the type-C port.
>>>> There are ways to fetch the status, but all those require the
>>>> IOM port status offset, which could change with platform.
>>>>
>>>> Make a debugfs directory for intel_pmc_mux and expose the status
>>>> under it per port basis.
>>>>
>>>> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
>>>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>> ---
>>>>
>>>> v4:
>>>> 1. Maintain a root directory for PMC module and incorporate devices
>>>> under it
>>>> 2. Add the debugfs module under '/sys/kernel/debug/usb'
>>>> 3. Use the platform device 'pmc->dev' to assign the device's name
>>>>
>>>> v3: Allocate the debugfs directory name for the platform device with
>>>> its ACPI dev name included
>>>>
>>>> v2:
>>>> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
>>>> 2. Remove explicitly defined one-liner functions
>>>>
>>>> drivers/usb/typec/mux/intel_pmc_mux.c | 54 ++++++++++++++++++++++++++-
>>>> 1 file changed, 53 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
>>>> index 34e4188a40ff..f400094c76f2 100644
>>>> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
>>>> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
>>>> @@ -15,6 +15,8 @@
>>>> #include <linux/usb/typec_mux.h>
>>>> #include <linux/usb/typec_dp.h>
>>>> #include <linux/usb/typec_tbt.h>
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/usb.h>
>>>> #include <asm/intel_scu_ipc.h>
>>>> @@ -143,8 +145,14 @@ struct pmc_usb {
>>>> struct acpi_device *iom_adev;
>>>> void __iomem *iom_base;
>>>> u32 iom_port_status_offset;
>>>> +
>>>> +#ifdef CONFIG_DEBUG_FS
>>>> + struct dentry *dentry;
>>>> +#endif
>>> No need for the #ifdef anymore, right? In fact, I think it will break
>>> the build if you have it this way and CONFIG_DEBUG_FS is not enabled,
>>> right?
>> I guess you're right. Maybe it'd have been fine if the rest of the
>> debugfs stuff was also enclosed within the conditional macros.
> Which is not needed and not good kernel coding style, so it's right that
> they are not there.
Sure.
>
>> Anyways, removing it seems appropriate now.
>>
>> Also, is it OK to send you v5 on the public list directly?
> Where else would you send it?
I was under the impression that you'd again require the review
internally under Intel before releasing out to public list.
Anyways, sending out v5.
Thanks
Rajat
>
> confused,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs
2023-05-23 7:43 ` Rajat Khandelwal
@ 2023-05-23 7:46 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2023-05-23 7:46 UTC (permalink / raw)
To: Rajat Khandelwal; +Cc: heikki.krogerus, linux-usb, linux-kernel
On Tue, May 23, 2023 at 01:13:37PM +0530, Rajat Khandelwal wrote:
> Hi,
>
> On 5/22/2023 9:56 PM, Greg KH wrote:
> > On Mon, May 22, 2023 at 03:24:13PM +0530, Rajat Khandelwal wrote:
> > > Hi,
> > >
> > > On 5/22/2023 1:07 PM, Greg KH wrote:
> > > > On Mon, May 22, 2023 at 12:33:43PM +0530, Rajat Khandelwal wrote:
> > > > > IOM status has a crucial role during debugging to check the
> > > > > current state of the type-C port.
> > > > > There are ways to fetch the status, but all those require the
> > > > > IOM port status offset, which could change with platform.
> > > > >
> > > > > Make a debugfs directory for intel_pmc_mux and expose the status
> > > > > under it per port basis.
> > > > >
> > > > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > ---
> > > > >
> > > > > v4:
> > > > > 1. Maintain a root directory for PMC module and incorporate devices
> > > > > under it
> > > > > 2. Add the debugfs module under '/sys/kernel/debug/usb'
> > > > > 3. Use the platform device 'pmc->dev' to assign the device's name
> > > > >
> > > > > v3: Allocate the debugfs directory name for the platform device with
> > > > > its ACPI dev name included
> > > > >
> > > > > v2:
> > > > > 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> > > > > 2. Remove explicitly defined one-liner functions
> > > > >
> > > > > drivers/usb/typec/mux/intel_pmc_mux.c | 54 ++++++++++++++++++++++++++-
> > > > > 1 file changed, 53 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > index 34e4188a40ff..f400094c76f2 100644
> > > > > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > @@ -15,6 +15,8 @@
> > > > > #include <linux/usb/typec_mux.h>
> > > > > #include <linux/usb/typec_dp.h>
> > > > > #include <linux/usb/typec_tbt.h>
> > > > > +#include <linux/debugfs.h>
> > > > > +#include <linux/usb.h>
> > > > > #include <asm/intel_scu_ipc.h>
> > > > > @@ -143,8 +145,14 @@ struct pmc_usb {
> > > > > struct acpi_device *iom_adev;
> > > > > void __iomem *iom_base;
> > > > > u32 iom_port_status_offset;
> > > > > +
> > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > + struct dentry *dentry;
> > > > > +#endif
> > > > No need for the #ifdef anymore, right? In fact, I think it will break
> > > > the build if you have it this way and CONFIG_DEBUG_FS is not enabled,
> > > > right?
> > > I guess you're right. Maybe it'd have been fine if the rest of the
> > > debugfs stuff was also enclosed within the conditional macros.
> > Which is not needed and not good kernel coding style, so it's right that
> > they are not there.
>
> Sure.
>
> >
> > > Anyways, removing it seems appropriate now.
> > >
> > > Also, is it OK to send you v5 on the public list directly?
> > Where else would you send it?
>
> I was under the impression that you'd again require the review
> internally under Intel before releasing out to public list.
As you all know, that is always required. Ideally it would have caught
build breakages like this the first time around, so maybe it isn't
working as well as it should be :(
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-23 7:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-22 7:03 [PATCH v4] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs Rajat Khandelwal
2023-05-22 7:37 ` Greg KH
2023-05-22 9:54 ` Rajat Khandelwal
2023-05-22 16:26 ` Greg KH
2023-05-23 7:43 ` Rajat Khandelwal
2023-05-23 7:46 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox