Re: [PATCH v6 3/3] phy: mediatek: tphy: add debugfs files

[Chunfeng Yun (云春峰)]

On Fri, 2023-01-13 at 23:45 +0530, Vinod Koul wrote:
> On 04-01-23, 21:26, Chunfeng Yun wrote:
> > These debugfs files are mainly used to make eye diagram test
> > easier,
> > especially helpful to do HQA test for a new IC without efuse
> > enabled.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> > ---
> > v6: no changes
> > 
> > v5: using common debugfs config CONFIG_DEBUG_FS
> > 
> > v4: fix build warning of sometimes uninitialized variable
> > 
> > v3: fix typo of "debugfs" suggested by AngeloGioacchino
> > 
> > v2: add CONFIG_PHY_MTK_TPHY_DEBUGFS suggested by AngeloGioacchino
> > ---
> >  drivers/phy/mediatek/phy-mtk-tphy.c | 405
> > +++++++++++++++++++++++++++-
> >  1 file changed, 404 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c
> > b/drivers/phy/mediatek/phy-mtk-tphy.c
> > index e906a82791bd..923e5ee119f3 100644
> > --- a/drivers/phy/mediatek/phy-mtk-tphy.c
> > +++ b/drivers/phy/mediatek/phy-mtk-tphy.c
> > @@ -7,6 +7,7 @@
> >  
> >  #include <dt-bindings/phy/phy.h>
> >  #include <linux/clk.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/delay.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/mfd/syscon.h>
> > @@ -264,6 +265,8 @@
> >  <skip>

> > +
> > +static void tphy_debugfs_init(struct mtk_tphy *tphy, struct
> > mtk_phy_instance *inst)
> > +{
> > +	char name[16];
> > +
> > +	snprintf(name, sizeof(name) - 1, "phy.%d", inst->index);
> 
> I wouold suggest driver name/ device name rather than phy.foo...
> again
> folks needs to see what is foo
> 
I compare the way you suggested again on mt2712 platform that have two
phy controller, t-phy@11290000 [1] and t-phy@112e0000 [2],
[1]: 
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi#L827
[2]:

https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi#L891


Each phy controller driver will created a root folder according its
device name under "/sys/kernel/debug/phy", like as:
#ls /sys/kernel/debug/phy
t-phy@11290000  t-phy@112e0000

The phy controller has some subphy node, each one indicate a phy
struct, if use the phy device name to create a folder, it shows as
below:
#cd /sys/kernel/debug/phy
# ls *
t-phy@11290000:
phy-t-phy@11290000.0  phy-t-phy@11290000.1

t-phy@112e0000:
phy-t-phy@112e0000.3  phy-t-phy@112e0000.4  phy-t-phy@112e0000.5

phy's device name is created as:
"dev_set_name(&phy->dev, "phy-%s.%d", dev_name(dev), id)"

dev_name(dev) is the parent name, it's t-phy@11290000 and 
t-phy@112e0000 in example, id is the phy->id.

seems no need to provide parent name again.
another problem is that the id may vary if other phys are enabled, that
means the path may vary, it's difficult to write scripts to do test.

when use the current way,
"snprintf(name, sizeof(name) - 1, "phy.%d", inst->index)"
it shows as:
# ls *
t-phy@11290000:
phy.0  phy.1

t-phy@112e0000:
phy.0  phy.1  phy.2

@inst->index is the sequence number of subphy node in each phy
controller's node, when the dts is fixed, the @index is also fixed,
then for each phy, the path is also fixed, seems more convenient to
write scripts.

So I prefer to leave the code unchanged.

Thanks a lot

> 
> > +	inst->dbgfs = debugfs_create_dir(name, tphy->dbgfs_root);
> > +
> > +	debugfs_create_file("type", 0444, inst->dbgfs, inst,
> > &tphy_type_fops);
> > +
> > +	switch (inst->type) {
> > +	case PHY_TYPE_USB2:
> > +		u2_phy_dbgfs_files_create(inst);
> > +		break;
> > +	case PHY_TYPE_USB3:
> > +	case PHY_TYPE_PCIE:
> > +		u3_phy_dbgfs_files_create(inst);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> > +static void tphy_debugfs_exit(struct mtk_phy_instance *inst)
> > +{
> > +	debugfs_remove_recursive(inst->dbgfs);
> > +	inst->dbgfs = NULL;
> > +}
> > +
> > +static void tphy_debugfs_root_create(struct mtk_tphy *tphy)
> > +{
> > +	tphy->dbgfs_root = debugfs_create_dir(dev_name(tphy->dev),
> > phy_debug_root);
> > +}
> > +
> > +static void tphy_debugfs_root_remove(struct mtk_tphy *tphy)
> > +{
> > +	debugfs_remove_recursive(tphy->dbgfs_root);
> > +	tphy->dbgfs_root = NULL;
> > +}
> > +
> > +#else
> > +<skip>