Re: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

Re: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[kernel test robot]

Hi Stanley,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus robh/for-next linus/master v6.4-rc6 next-20230614]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanley-Chang/phy-realtek-usb-Add-driver-for-the-Realtek-SoC-USB-2-0-PHY/20230614-173349
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230614092850.21460-2-stanley_chang%40realtek.com
patch subject: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230614/202306142352.e4eBd3HX-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add usb https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
        git fetch usb usb-testing
        git checkout usb/usb-testing
        b4 shazam https://lore.kernel.org/r/20230614092850.21460-2-stanley_chang@realtek.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/phy/realtek/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306142352.e4eBd3HX-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/phy/realtek/phy-rtk-usb2.c: In function 'parse_phy_data':
>> drivers/phy/realtek/phy-rtk-usb2.c:1229:25: warning: variable 'phy_cfg' set but not used [-Wunused-but-set-variable]
    1229 |         struct phy_cfg *phy_cfg;
         |                         ^~~~~~~


vim +/phy_cfg +1229 drivers/phy/realtek/phy-rtk-usb2.c

  1224	
  1225	static int parse_phy_data(struct rtk_phy *rtk_phy)
  1226	{
  1227		struct device *dev = rtk_phy->dev;
  1228		struct device_node *node;
> 1229		struct phy_cfg *phy_cfg;
  1230		struct phy_parameter *phy_parameter;
  1231		int ret = 0;
  1232		int index;
  1233	
  1234		node = dev->of_node;
  1235		phy_cfg = rtk_phy->phy_cfg;
  1236	
  1237		rtk_phy->phy_parameter = devm_kzalloc(dev, sizeof(struct phy_parameter) *
  1238							rtk_phy->num_phy, GFP_KERNEL);
  1239		if (!rtk_phy->phy_parameter)
  1240			return -ENOMEM;
  1241	
  1242		for (index = 0; index < rtk_phy->num_phy; index++) {
  1243			phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
  1244	
  1245			phy_parameter->phy_reg.reg_wrap_vstatus = of_iomap(dev->of_node, 0);
  1246			phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(dev->of_node, 1) + index;
  1247			phy_parameter->phy_reg.vstatus_index = index;
  1248	
  1249			if (of_property_read_bool(node, "realtek,inverse-hstx-sync-clock"))
  1250				phy_parameter->inverse_hstx_sync_clock = true;
  1251			else
  1252				phy_parameter->inverse_hstx_sync_clock = false;
  1253	
  1254			if (of_property_read_u32_index(node, "realtek,driving-level",
  1255						       index, &phy_parameter->driving_level))
  1256				phy_parameter->driving_level = DEFAULT_DC_DRIVING_VALUE;
  1257	
  1258			if (of_property_read_u32_index(node, "realtek,driving-compensate",
  1259						       index, &phy_parameter->driving_compensate))
  1260				phy_parameter->driving_compensate = 0;
  1261	
  1262			if (of_property_read_u32_index(node, "realtek,disconnection-compensate",
  1263						       index, &phy_parameter->disconnection_compensate))
  1264				phy_parameter->disconnection_compensate = 0;
  1265	
  1266			get_phy_data_by_efuse(rtk_phy, phy_parameter, index);
  1267	
  1268			update_dc_driving_level(rtk_phy, phy_parameter);
  1269	
  1270			update_hs_clk_select(rtk_phy, phy_parameter);
  1271		}
  1272	
  1273		return ret;
  1274	}
  1275	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY

[kernel test robot]

Hi Stanley,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus robh/for-next linus/master v6.4-rc6 next-20230614]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanley-Chang/phy-realtek-usb-Add-driver-for-the-Realtek-SoC-USB-2-0-PHY/20230614-173349
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230614092850.21460-3-stanley_chang%40realtek.com
patch subject: [PATCH v4 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230614/202306142340.nvui81i4-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add usb https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
        git fetch usb usb-testing
        git checkout usb/usb-testing
        b4 shazam https://lore.kernel.org/r/20230614092850.21460-3-stanley_chang@realtek.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/phy/realtek/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306142340.nvui81i4-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/phy/realtek/phy-rtk-usb3.c: In function 'rtk_usb3phy_probe':
>> drivers/phy/realtek/phy-rtk-usb3.c:779:29: warning: variable 'node' set but not used [-Wunused-but-set-variable]
     779 |         struct device_node *node;
         |                             ^~~~


vim +/node +779 drivers/phy/realtek/phy-rtk-usb3.c

   774	
   775	static int rtk_usb3phy_probe(struct platform_device *pdev)
   776	{
   777		struct rtk_phy *rtk_phy;
   778		struct device *dev = &pdev->dev;
 > 779		struct device_node *node;
   780		struct phy *generic_phy;
   781		struct phy_provider *phy_provider;
   782		const struct phy_cfg *phy_cfg;
   783		int ret;
   784	
   785		phy_cfg = of_device_get_match_data(dev);
   786		if (!phy_cfg) {
   787			dev_err(dev, "phy config are not assigned!\n");
   788			return -EINVAL;
   789		}
   790	
   791		rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
   792		if (!rtk_phy)
   793			return -ENOMEM;
   794	
   795		rtk_phy->dev			= &pdev->dev;
   796		rtk_phy->phy.dev		= rtk_phy->dev;
   797		rtk_phy->phy.label		= "rtk-usb3phy";
   798		rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status;
   799	
   800		rtk_phy->phy_cfg = devm_kzalloc(dev, sizeof(*phy_cfg), GFP_KERNEL);
   801	
   802		memcpy(rtk_phy->phy_cfg, phy_cfg, sizeof(*phy_cfg));
   803	
   804		node = dev->of_node;
   805	
   806		rtk_phy->num_phy = 1;
   807	
   808		ret = parse_phy_data(rtk_phy);
   809		if (ret)
   810			goto err;
   811	
   812		platform_set_drvdata(pdev, rtk_phy);
   813	
   814		generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
   815		if (IS_ERR(generic_phy))
   816			return PTR_ERR(generic_phy);
   817	
   818		phy_set_drvdata(generic_phy, rtk_phy);
   819	
   820		phy_provider = devm_of_phy_provider_register(rtk_phy->dev, of_phy_simple_xlate);
   821		if (IS_ERR(phy_provider))
   822			return PTR_ERR(phy_provider);
   823	
   824		ret = usb_add_phy_dev(&rtk_phy->phy);
   825		if (ret)
   826			goto err;
   827	
   828		create_debug_files(rtk_phy);
   829	
   830	err:
   831		return ret;
   832	}
   833	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 14/06/2023 11:28, Stanley Chang wrote:
> Add the documentation explain the property about Realtek USB PHY driver.
> 
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 2.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v3 to v4 change:
>     1. Remove the parameter and non hardware properties from dts.
>     2. Using the compatible data included the config and parameter
>        in driver.
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3.
>     2. Add more description about Realtek RTD SoCs architecture.
>     3. Removed parameter v1 support for simplification.
>     4. Revised the compatible name for fallback compatible.
>     5. Remove some properties that can be set in the driver.
> v1 to v2 change:
>     Add phy-cells for generic phy driver
> ---
>  .../bindings/phy/realtek,usb2phy.yaml         | 145 ++++++++++++++++++
>  1 file changed, 145 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml b/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> new file mode 100644
> index 000000000000..cfd77143475c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> @@ -0,0 +1,145 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,usb2phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs USB 2.0 PHY
> +
> +maintainers:
> +  - Stanley Chang <stanley_chang@realtek.com>
> +
> +description:
> +  Realtek USB 2.0 PHY support the digital home center (DHC) RTD series SoCs.
> +  The USB 2.0 PHY driver is designed to support the XHCI controller. The SoCs
> +  support multiple XHCI controllers. One PHY device node maps to one XHCI
> +  controller.
> +
> +  RTD1295/RTD1619 SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on some
> +  controllers.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1395 SoCs USB
> +  The USB architecture includes two XHCI controllers.
> +  The controller#0 has one USB 2.0 PHY. The controller#1 includes two USB 2.0
> +  PHY.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +                               |- phy#1
> +
> +  RTD1319/RTD1619b SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#2.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1319d SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each xhci maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#0.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +  RTD1312c/RTD1315e SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtd1295-usb2phy
> +          - realtek,rtd1312c-usb2phy
> +          - realtek,rtd1315e-usb2phy
> +          - realtek,rtd1319-usb2phy
> +          - realtek,rtd1319d-usb2phy
> +          - realtek,rtd1395-usb2phy
> +          - realtek,rtd1395-usb2phy-2port
> +          - realtek,rtd1619-usb2phy
> +          - realtek,rtd1619b-usb2phy
> +      - const: realtek,usb2phy

That's not what your driver is saying... This patchset has random set of
changes.

I suggest to drop "realtek,usb2phy".

> +
> +  reg:
> +    items:
> +      - description: PHY data registers
> +      - description: PHY control registers
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  nvmem-cells:
> +    maxItems: 2
> +    description:
> +      Phandles to nvmem cell that contains the trimming data.
> +      If unspecified, default value is used.
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: usb-dc-cal
> +      - const: usb-dc-dis
> +    description:
> +      The following names, which correspond to each nvmem-cells.
> +      usb-dc-cal is the driving level for each phy specified via efuse.
> +      usb-dc-dis is the disconnection level for each phy specified via efuse.
> +
> +  realtek,inverse-hstx-sync-clock:
> +    description:
> +      For one of the phys of RTD1619b SoC, the synchronous clock of the
> +      high-speed tx must be inverted. So this property is used to set the
> +      inverted clock.

Drop last sentence, it is redundant.

> +    type: boolean
> +
> +  realtek,driving-level:
> +    description:
> +      Each board or port may have a different driving capability. This
> +      will adjust the driving level value if the value is not the default.

I don't understand it. What is "driving capability" and if each port can
have it different, why do you need property for this?

You mention some default - why it is not expressed as "default: xx"?

What do the values mean?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 31
> +
> +  realtek,driving-compensate:
> +    description:
> +      For RTD1315e SoC, the driving level can be adjusted by reading the
> +      efuse table. Therefore, this property provides drive compensation for
> +      different boards with different drive capabilities.

if driving level can be read from nvmem, why do you have
realtek,driving-level in the first place? Don't you miss here some allOf
making this constrained per variant?

"Therefore" means "for that reason" or "as a consequence". How is this
property a consequence of reading driving level from efuse? Is it then
mutually exclusive with "realtek,driving-level"? But your schema does
not express it.

> +    $ref: /schemas/types.yaml#/definitions/int32
> +    minimum: -8
> +    maximum: 8
> +
> +  realtek,disconnection-compensate:
> +    description:
> +      This adjusts the disconnection level compensation for the different
> +      boards with different disconnection level.
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    minimum: -8
> +    maximum: 8
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    usb_port0_usb2phy: usb-phy@13214 {
> +        compatible = "realtek,rtd1319d-usb2phy", "realtek,usb2phy";
> +        reg = <0x13214 0x4>, <0x28280 0x4>;
> +        #phy-cells = <0>;
> +
> +        realtek,driving-level = <0xe>;

Why this example is so empty? You have at least 6 more properties which
should be shown here.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 14/06/2023 11:28, Stanley Chang wrote:
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 2.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v3 to v4 change:
>     1. Remove the parameter and non hardware properties from dts.
>        Add the compatible data included the config and parameter.
>     2. Fix the warning for checkpatch with strict.
>     3. Remove useless debug messages.
>     4. Fix the incorrect or useless "if() return" which pointer check.
>     5. Remove the phy power domain control due to we control on other
>        driver.
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3 PHY.
>     2. Removed parameter v1 support for simplification.
>     3. Use remove_new for driver remove callback.
> v1 to v2 change:
>     1. Move the drivers to drivers/phy/ for generic phy driver
>     2. Use the generic phy driver api to initialize phy
>     3. Use readl/writel to instead phy_read/phy_write directly.
>     4. Use read_poll_timeout() in function utmi_wait_register.
>     5. Revised some coding styles.
>     6. fix the compiler warning for kernel test robot.
> ---

...

> +/* updated disconnect level at page0 */
> +static void update_dc_disconnect_level_at_page0(struct rtk_phy *rtk_phy,
> +						struct phy_parameter *phy_parameter, bool update)
> +{
> +	struct phy_cfg *phy_cfg;
> +	struct phy_reg *phy_reg;
> +	struct phy_data *phy_data_page;
> +	struct phy_data *phy_data;
> +	u8 addr, data;
> +	int offset = 4;
> +	s32 dc_disconnect_mask;
> +	int i;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +	phy_reg = &phy_parameter->phy_reg;
> +
> +	/* Set page 0 */
> +	phy_data_page = phy_cfg->page0;
> +	rtk_phy_set_page(phy_reg, 0);
> +
> +	i = page_addr_to_array_index(PAGE0_0XE4);
> +	phy_data = phy_data_page + i;
> +	if (!phy_data->addr) {
> +		phy_data->addr = PAGE0_0XE4;
> +		phy_data->data = rtk_phy_read(phy_reg, PAGE0_0XE4);
> +	}
> +
> +	addr = phy_data->addr;
> +	data = phy_data->data;
> +	dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> +
> +	if (update)
> +		data = __updated_dc_disconnect_level_page0_0xe4(phy_cfg, phy_parameter, data);
> +	else
> +		data = (data & ~(dc_disconnect_mask << offset)) |
> +			(DEFAULT_DC_DISCONNECTION_VALUE << offset);
> +
> +	if (rtk_phy_write(phy_reg, addr, data))
> +		dev_err(rtk_phy->dev,
> +			"[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> +			__func__, __LINE__,
> +			addr, data);

Is addr a kernel address or any memory (not SFR) address? If so, you
cannot print it.

> +}
> +
> +static u8 __updated_dc_disconnect_level_page1_0xe2(struct phy_cfg *phy_cfg,
> +						   struct phy_parameter *phy_parameter, u8 data)
> +{
> +	u8 val;
> +	s32 __val;
> +	s32 dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> +
> +	if (phy_cfg->check_efuse_version == CHECK_EFUSE_V1) {
> +		__val = (s32)(data & dc_disconnect_mask)
> +			    + phy_parameter->efuse_usb_dc_dis
> +			    + phy_parameter->disconnection_compensate;
> +	} else { /* for CHECK_EFUSE_V2 or no efuse */
> +		if (phy_parameter->efuse_usb_dc_dis)
> +			__val = (s32)(phy_parameter->efuse_usb_dc_dis +
> +				    phy_parameter->disconnection_compensate);
> +		else
> +			__val = (s32)((data & dc_disconnect_mask) +
> +				    phy_parameter->disconnection_compensate);
> +	}
> +
> +	if (__val > dc_disconnect_mask)
> +		__val = dc_disconnect_mask;
> +	else if (__val < 0)
> +		__val = 0;
> +
> +	val = (data & (~dc_disconnect_mask)) | (__val & dc_disconnect_mask);
> +
> +	return val;
> +}
> +
> +/* updated disconnect level at page1 */
> +static void update_dc_disconnect_level_at_page1(struct rtk_phy *rtk_phy,
> +						struct phy_parameter *phy_parameter, bool update)
> +{
> +	struct phy_cfg *phy_cfg;
> +	struct phy_data *phy_data_page;
> +	struct phy_data *phy_data;
> +	struct phy_reg *phy_reg;
> +	u8 addr, data;
> +	s32 dc_disconnect_mask;
> +	int i;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +	phy_reg = &phy_parameter->phy_reg;
> +
> +	/* Set page 1 */
> +	phy_data_page = phy_cfg->page1;
> +	rtk_phy_set_page(phy_reg, 1);
> +
> +	i = page_addr_to_array_index(PAGE1_0XE2);
> +	phy_data = phy_data_page + i;
> +	if (!phy_data->addr) {
> +		phy_data->addr = PAGE1_0XE2;
> +		phy_data->data = rtk_phy_read(phy_reg, PAGE1_0XE2);
> +	}
> +
> +	addr = phy_data->addr;
> +	data = phy_data->data;
> +	dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> +
> +	if (update)
> +		data = __updated_dc_disconnect_level_page1_0xe2(phy_cfg, phy_parameter, data);
> +	else
> +		data = (data & ~dc_disconnect_mask) | DEFAULT_DC_DISCONNECTION_VALUE;
> +
> +	if (rtk_phy_write(phy_reg, addr, data))
> +		dev_err(rtk_phy->dev,
> +			"[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> +			__func__, __LINE__,
> +			addr, data);

Ditto and in all other places.

> +}
> +
> +static void update_dc_disconnect_level(struct rtk_phy *rtk_phy,
> +				       struct phy_parameter *phy_parameter, bool update)
> +{
> +	struct phy_cfg *phy_cfg = rtk_phy->phy_cfg;
> +
> +	if (phy_cfg->usb_dc_disconnect_at_page0)
> +		update_dc_disconnect_level_at_page0(rtk_phy, phy_parameter, update);
> +	else
> +		update_dc_disconnect_level_at_page1(rtk_phy, phy_parameter, update);
> +}
> +
> +static u8 __update_dc_driving_page0_0xe4(struct phy_cfg *phy_cfg,
> +					 struct phy_parameter *phy_parameter, u8 data)
> +{
> +	s32 driving_compensate = phy_parameter->driving_compensate;
> +	s32 dc_driving_mask = phy_cfg->dc_driving_mask;
> +	s32 __val;
> +	u8 val;

Two variables with the same name. No, it is not readable code.

...

> +static void update_dc_driving_level(struct rtk_phy *rtk_phy,
> +				    struct phy_parameter *phy_parameter)
> +{
> +	struct phy_cfg *phy_cfg;
> +	struct phy_reg *phy_reg;
> +

...
> +
> +static const struct phy_ops ops = {
> +	.init		= rtk_phy_init,
> +	.exit		= rtk_phy_exit,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static void rtk_phy_toggle(struct usb_phy *usb2_phy, bool connect, int port)
> +{
> +	int index = port;
> +	struct rtk_phy *rtk_phy = NULL;
> +
> +	rtk_phy = dev_get_drvdata(usb2_phy->dev);
> +
> +	if (index > rtk_phy->num_phy) {
> +		pr_err("%s %d ERROR! port=%d > num_phy=%d\n",

dev_err

> +		       __func__, __LINE__, index, rtk_phy->num_phy);

all these func and LINE point to poor code quality and poor debugging
practices. These are added dugin development, not for production code,
because error message should be obvious. Your usage of pr_err, func,
LINE and some unprecise messages suggests this is not ready.

Fix all your error messages to be meaningful.

> +		return
> +	}
> +
> +	do_rtk_phy_toggle(rtk_phy, index, connect);
> +}
> +
> +static int rtk_phy_notify_port_status(struct usb_phy *x, int port,
> +				      u16 portstatus, u16 portchange)
> +{
> +	bool connect = false;
> +
> +	pr_debug("%s port=%d portstatus=0x%x portchange=0x%x\n",
> +		 __func__, port, (int)portstatus, (int)portchange);
> +	if (portstatus & USB_PORT_STAT_CONNECTION)
> +		connect = true;
> +
> +	if (portchange & USB_PORT_STAT_C_CONNECTION)
> +		rtk_phy_toggle(x, connect, port);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +static struct dentry *create_phy_debug_root(void)
> +{
> +	struct dentry *phy_debug_root;
> +
> +	phy_debug_root = debugfs_lookup("phy", usb_debug_root);
> +	if (!phy_debug_root)
> +		phy_debug_root = debugfs_create_dir("phy", usb_debug_root);
> +
> +	return phy_debug_root;
> +}
> +
> +static int rtk_usb2_parameter_show(struct seq_file *s, void *unused)
> +{
> +	struct rtk_phy *rtk_phy = s->private;
> +	struct phy_cfg *phy_cfg;
> +	int i, index;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +
> +	seq_puts(s, "Property:\n");
> +	seq_printf(s, "  check_efuse: %s\n",
> +		   phy_cfg->check_efuse ? "Enable" : "Disable");
> +	seq_printf(s, "  check_efuse_version: %d\n",
> +		   phy_cfg->check_efuse_version);
> +	seq_printf(s, "  efuse_dc_driving_rate: %d\n",
> +		   phy_cfg->efuse_dc_driving_rate);
> +	seq_printf(s, "  dc_driving_mask: 0x%x\n",
> +		   phy_cfg->dc_driving_mask);
> +	seq_printf(s, "  efuse_dc_disconnect_rate: %d\n",
> +		   phy_cfg->efuse_dc_disconnect_rate);
> +	seq_printf(s, "  dc_disconnect_mask: 0x%x\n",
> +		   phy_cfg->dc_disconnect_mask);
> +	seq_printf(s, "  usb_dc_disconnect_at_page0: %s\n",
> +		   phy_cfg->usb_dc_disconnect_at_page0 ? "true" : "false");
> +	seq_printf(s, "  do_toggle: %s\n",
> +		   phy_cfg->do_toggle ? "Enable" : "Disable");
> +	seq_printf(s, "  do_toggle_driving: %s\n",
> +		   phy_cfg->do_toggle_driving ? "Enable" : "Disable");
> +	seq_printf(s, "  driving_updated_for_dev_dis: 0x%x\n",
> +		   phy_cfg->driving_updated_for_dev_dis);
> +	seq_printf(s, "  use_default_parameter: %s\n",
> +		   phy_cfg->use_default_parameter ? "Enable" : "Disable");
> +	seq_printf(s, "  is_double_sensitivity_mode: %s\n",
> +		   phy_cfg->is_double_sensitivity_mode ? "Enable" : "Disable");
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		struct phy_parameter *phy_parameter;
> +		struct phy_reg *phy_reg;
> +		struct phy_data *phy_data_page;
> +
> +		phy_parameter =  &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +		phy_reg = &phy_parameter->phy_reg;
> +
> +		seq_printf(s, "PHY %d:\n", index);
> +
> +		seq_puts(s, "Page 0:\n");
> +		/* Set page 0 */
> +		phy_data_page = phy_cfg->page0;
> +		rtk_phy_set_page(phy_reg, 0);
> +
> +		for (i = 0; i < phy_cfg->page0_size; i++) {
> +			struct phy_data *phy_data = phy_data_page + i;
> +			u8 addr = array_index_to_page_addr(i);
> +			u8 data = phy_data->data;
> +			u8 value = rtk_phy_read(phy_reg, addr);
> +
> +			if (phy_data->addr)
> +				seq_printf(s, "  Page 0: addr=0x%x data=0x%02x ==> read value=0x%02x\n",
> +					   addr, data, value);
> +			else
> +				seq_printf(s, "  Page 0: addr=0x%x data=none ==> read value=0x%02x\n",
> +					   addr, value);
> +		}
> +
> +		seq_puts(s, "Page 1:\n");
> +		/* Set page 1 */
> +		phy_data_page = phy_cfg->page1;
> +		rtk_phy_set_page(phy_reg, 1);
> +
> +		for (i = 0; i < phy_cfg->page1_size; i++) {
> +			struct phy_data *phy_data = phy_data_page + i;
> +			u8 addr = array_index_to_page_addr(i);
> +			u8 data = phy_data->data;
> +			u8 value = rtk_phy_read(phy_reg, addr);
> +
> +			if (phy_data->addr)
> +				seq_printf(s, "  Page 1: addr=0x%x data=0x%02x ==> read value=0x%02x\n",
> +					   addr, data, value);
> +			else
> +				seq_printf(s, "  Page 1: addr=0x%x data=none ==> read value=0x%02x\n",
> +					   addr, value);
> +		}
> +
> +		if (phy_cfg->page2_size == 0)
> +			goto out;
> +
> +		seq_puts(s, "Page 2:\n");
> +		/* Set page 2 */
> +		phy_data_page = phy_cfg->page2;
> +		rtk_phy_set_page(phy_reg, 2);
> +
> +		for (i = 0; i < phy_cfg->page2_size; i++) {
> +			struct phy_data *phy_data = phy_data_page + i;
> +			u8 addr = array_index_to_page_addr(i);
> +			u8 data = phy_data->data;
> +			u8 value = rtk_phy_read(phy_reg, addr);
> +
> +			if (phy_data->addr)
> +				seq_printf(s, "  Page 2: addr=0x%x data=0x%02x ==> read value=0x%02x\n",
> +					   addr, data, value);
> +			else
> +				seq_printf(s, "  Page 2: addr=0x%x data=none ==> read value=0x%02x\n",
> +					   addr, value);
> +		}
> +
> +out:
> +		seq_puts(s, "PHY Property:\n");
> +		seq_printf(s, "  efuse_usb_dc_cal: %d\n",
> +			   (int)phy_parameter->efuse_usb_dc_cal);
> +		seq_printf(s, "  efuse_usb_dc_dis: %d\n",
> +			   (int)phy_parameter->efuse_usb_dc_dis);
> +		seq_printf(s, "  inverse_hstx_sync_clock: %s\n",
> +			   phy_parameter->inverse_hstx_sync_clock ? "Enable" : "Disable");
> +		seq_printf(s, "  driving_level: %d\n",
> +			   phy_parameter->driving_level);
> +		seq_printf(s, "  driving_compensate: %d\n",
> +			   phy_parameter->driving_compensate);
> +		seq_printf(s, "  disconnection_compensate: %d\n",
> +			   phy_parameter->disconnection_compensate);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtk_usb2_parameter_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, rtk_usb2_parameter_show, inode->i_private);
> +}
> +
> +static const struct file_operations rtk_usb2_parameter_fops = {
> +	.open			= rtk_usb2_parameter_open,
> +	.read			= seq_read,
> +	.llseek			= seq_lseek,
> +	.release		= single_release,
> +};
> +
> +static int __get_parameter_at_page(struct seq_file *s,
> +				   struct rtk_phy *rtk_phy,
> +				   struct phy_data *phy_data_page,
> +				   const char *phy_page, const char *phy_addr)
> +{
> +	struct phy_data *phy_data;
> +	u32 addr;
> +	int i, ret;
> +
> +	ret = kstrtouint(phy_addr, 16, &addr);
> +	if (ret < 0) {
> +		pr_err("%s::kstrtouint() failed\n", __func__);
> +		return -EINVAL;
> +	}
> +	i = page_addr_to_array_index(addr);
> +	phy_data = (phy_data_page + i);
> +
> +	if (phy_data->addr)
> +		seq_printf(s, "Now Parameter %s addr 0x%02x = 0x%02x\n",
> +			   phy_page, phy_data->addr, phy_data->data);
> +	else
> +		seq_printf(s, "Now Parameter %s addr 0x%02x is default\n",
> +			   phy_page, addr);
> +
> +	return 0;
> +}
> +
> +static int __set_parameter_at_page(struct rtk_phy *rtk_phy,
> +				   struct phy_reg *phy_reg, struct phy_parameter *phy_parameter,
> +				   struct phy_data *phy_data_page,
> +				   const char *phy_page, const char *phy_addr,
> +				   const char *phy_value)
> +{
> +	struct phy_data *phy_data;
> +	u32 addr, value;
> +	int i, ret;
> +
> +	ret = kstrtouint(phy_addr, 16, &addr);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	ret = kstrtouint(phy_value, 16, &value);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	i = page_addr_to_array_index(addr);
> +	phy_data = (phy_data_page + i);
> +
> +	if (phy_data->addr) {
> +		phy_data->data = value;
> +	} else {
> +		phy_data->addr = addr;
> +		phy_data->data = value;
> +	}
> +
> +	if (rtk_phy_write(phy_reg, addr, value))
> +		dev_err(rtk_phy->dev,
> +			"[%s:%d] Error: addr=0x%02x value=0x%02x\n",
> +			__func__, __LINE__, addr, value);
> +
> +	return 0;
> +}
> +
> +static int rtk_usb2_set_parameter_show(struct seq_file *s, void *unused)
> +{
> +	const struct file *file = s->file;
> +	const char *file_name = file_dentry(file)->d_iname;
> +	struct dentry *p_dentry = file_dentry(file)->d_parent;
> +	const char *dir_name = p_dentry->d_iname;
> +	struct dentry *pp_dentry = p_dentry->d_parent;
> +	const char *phy_dir_name = pp_dentry->d_iname;
> +	struct rtk_phy *rtk_phy = s->private;
> +	struct phy_cfg *phy_cfg;
> +	struct phy_parameter *phy_parameter = NULL;
> +	int ret = 0;
> +	int index;
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		size_t sz = 30;
> +		char name[30] = {0};
> +
> +		snprintf(name, sz, "phy%d", index);
> +		if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> +			phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +			break;
> +		}
> +	}
> +
> +	if (!phy_parameter)
> +		return -EINVAL;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +
> +	if (strcmp("page0", dir_name) == 0)
> +		ret = __get_parameter_at_page(s, rtk_phy, phy_cfg->page0,
> +					      dir_name, file_name);
> +	else if (strcmp("page1", dir_name) == 0)
> +		ret = __get_parameter_at_page(s, rtk_phy, phy_cfg->page1,
> +					      dir_name, file_name);
> +	else if (strcmp("page2", dir_name) == 0)
> +		ret = __get_parameter_at_page(s, rtk_phy, phy_cfg->page2,
> +					      dir_name, file_name);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	seq_puts(s, "Set phy parameter by following command\n");
> +	seq_printf(s, "echo \"value\" > %s/%s/%s\n",
> +		   phy_dir_name, dir_name, file_name);
> +
> +	return 0;
> +}
> +
> +static int rtk_usb2_set_parameter_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, rtk_usb2_set_parameter_show, inode->i_private);
> +}
> +
> +static ssize_t rtk_usb2_set_parameter_write(struct file *file,
> +					    const char __user *ubuf, size_t count,
> +					    loff_t *ppos)
> +{
> +	const char *file_name = file_dentry(file)->d_iname;
> +	struct dentry *p_dentry = file_dentry(file)->d_parent;
> +	const char *dir_name = p_dentry->d_iname;
> +	struct dentry *pp_dentry = p_dentry->d_parent;
> +	const char *phy_dir_name = pp_dentry->d_iname;
> +	struct seq_file *s = file->private_data;
> +	struct rtk_phy *rtk_phy = s->private;
> +	struct phy_reg *phy_reg;
> +	struct phy_cfg *phy_cfg;
> +	struct phy_parameter *phy_parameter = NULL;
> +	int ret = 0;
> +	char buffer[40] = {0};
> +	int index;
> +
> +	if (copy_from_user(&buffer, ubuf,
> +			   min_t(size_t, sizeof(buffer) - 1, count)))
> +		return -EFAULT;
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		size_t sz = 30;
> +		char name[30] = {0};
> +
> +		snprintf(name, sz, "phy%d", index);
> +		if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> +			phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +			break;
> +		}
> +	}
> +
> +	if (!phy_parameter)
> +		return -EINVAL;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +	phy_reg = &phy_parameter->phy_reg;
> +
> +	if (strcmp("page0", dir_name) == 0) {
> +		rtk_phy_set_page(phy_reg, 0);
> +		ret = __set_parameter_at_page(rtk_phy, phy_reg, phy_parameter,
> +					      phy_cfg->page0, dir_name, file_name, buffer);
> +	} else if (strcmp("page1", dir_name) == 0) {
> +		rtk_phy_set_page(phy_reg, 1);
> +		ret = __set_parameter_at_page(rtk_phy, phy_reg, phy_parameter,
> +					      phy_cfg->page1, dir_name, file_name, buffer);
> +	} else if (strcmp("page2", dir_name) == 0) {
> +		rtk_phy_set_page(phy_reg, 2);
> +		ret = __set_parameter_at_page(rtk_phy, phy_reg, phy_parameter,
> +					      phy_cfg->page2, dir_name, file_name, buffer);
> +	}
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static const struct file_operations rtk_usb2_set_parameter_fops = {
> +	.open			= rtk_usb2_set_parameter_open,
> +	.write			= rtk_usb2_set_parameter_write,

NAK. You just created user interface via debugfs. You cannot. Reading
for some debug is okay, but configuring device via undocumented debugfs
is a source of troubles.

Drop all writes to debugfs.

...

> +
> +static int parse_phy_data(struct rtk_phy *rtk_phy)
> +{
> +	struct device *dev = rtk_phy->dev;
> +	struct device_node *node;

By convention:
s/node/np/

> +	struct phy_cfg *phy_cfg;
> +	struct phy_parameter *phy_parameter;
> +	int ret = 0;
> +	int index;
> +
> +	node = dev->of_node;

Keep it in variable definition.

> +	phy_cfg = rtk_phy->phy_cfg;
> +
> +	rtk_phy->phy_parameter = devm_kzalloc(dev, sizeof(struct phy_parameter) *
> +						rtk_phy->num_phy, GFP_KERNEL);
> +	if (!rtk_phy->phy_parameter)
> +		return -ENOMEM;
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +
> +		phy_parameter->phy_reg.reg_wrap_vstatus = of_iomap(dev->of_node, 0);
> +		phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(dev->of_node, 1) + index;
> +		phy_parameter->phy_reg.vstatus_index = index;
> +
> +		if (of_property_read_bool(node, "realtek,inverse-hstx-sync-clock"))
> +			phy_parameter->inverse_hstx_sync_clock = true;
> +		else
> +			phy_parameter->inverse_hstx_sync_clock = false;
> +
> +		if (of_property_read_u32_index(node, "realtek,driving-level",
> +					       index, &phy_parameter->driving_level))
> +			phy_parameter->driving_level = DEFAULT_DC_DRIVING_VALUE;
> +
> +		if (of_property_read_u32_index(node, "realtek,driving-compensate",
> +					       index, &phy_parameter->driving_compensate))
> +			phy_parameter->driving_compensate = 0;
> +
> +		if (of_property_read_u32_index(node, "realtek,disconnection-compensate",
> +					       index, &phy_parameter->disconnection_compensate))
> +			phy_parameter->disconnection_compensate = 0;
> +
> +		get_phy_data_by_efuse(rtk_phy, phy_parameter, index);
> +
> +		update_dc_driving_level(rtk_phy, phy_parameter);
> +
> +		update_hs_clk_select(rtk_phy, phy_parameter);
> +	}
> +
> +	return ret;
> +}
> +
> +static int rtk_usb2phy_probe(struct platform_device *pdev)
> +{
> +	struct rtk_phy *rtk_phy;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	const struct phy_cfg *phy_cfg;
> +	int ret = 0;
> +
> +	phy_cfg = of_device_get_match_data(dev);
> +	if (!phy_cfg) {
> +		dev_err(dev, "phy config are not assigned!\n");
> +		return -EINVAL;
> +	}
> +
> +	rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
> +	if (!rtk_phy)
> +		return -ENOMEM;
> +
> +	rtk_phy->dev			= &pdev->dev;
> +	rtk_phy->phy.dev		= rtk_phy->dev;
> +	rtk_phy->phy.label		= "rtk-usb2phy";
> +	rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status;
> +
> +	rtk_phy->phy_cfg = devm_kzalloc(dev, sizeof(*phy_cfg), GFP_KERNEL);
> +
> +	memcpy(rtk_phy->phy_cfg, phy_cfg, sizeof(*phy_cfg));
> +
> +	node = dev->of_node;

Drop it. Useless assignment.

> +
> +	if (of_device_is_compatible(node, "realtek,rtd1395-usb2phy-2port"))

No, customize variant with driver_data. Don't embed compatibles in the code.


> +		rtk_phy->num_phy = 2;
> +	else
> +		rtk_phy->num_phy = 1;
> +
> +	ret = parse_phy_data(rtk_phy);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, rtk_phy);
> +
> +	generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, rtk_phy);
> +
> +	phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
> +						     of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	ret = usb_add_phy_dev(&rtk_phy->phy);
> +	if (ret)
> +		goto err;
> +
> +	create_debug_files(rtk_phy);
> +
> +err:
> +	dev_dbg(dev, "Probe RTK USB 2.0 PHY (ret=%d)\n", ret);

NAK. I made it pretty clear last time.


This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> +
> +	return ret;
> +}
> +
> +static void rtk_usb2phy_remove(struct platform_device *pdev)
> +{
> +	struct rtk_phy *rtk_phy = platform_get_drvdata(pdev);
> +
> +	remove_debug_files(rtk_phy);
> +
> +	usb_remove_phy(&rtk_phy->phy);
> +}

...

> +
> +static const struct of_device_id usbphy_rtk_dt_match[] = {
> +	{ .compatible = "realtek,rtd1295-usb2phy", .data = &rtd1295_phy_cfg },
> +	{ .compatible = "realtek,rtd1312c-usb2phy", .data = &rtd1312c_phy_cfg },
> +	{ .compatible = "realtek,rtd1315e-usb2phy", .data = &rtd1315e_phy_cfg },
> +	{ .compatible = "realtek,rtd1319-usb2phy", .data = &rtd1319_phy_cfg },
> +	{ .compatible = "realtek,rtd1319d-usb2phy", .data = &rtd1319d_phy_cfg },
> +	{ .compatible = "realtek,rtd1395-usb2phy", .data = &rtd1395_phy_cfg },
> +	{ .compatible = "realtek,rtd1395-usb2phy-2port", .data = &rtd1395_phy_cfg },
> +	{ .compatible = "realtek,rtd1619-usb2phy", .data = &rtd1619_phy_cfg },
> +	{ .compatible = "realtek,rtd1619b-usb2phy", .data = &rtd1619b_phy_cfg },
> +	{ .compatible = "realtek,usb2phy", .data = &rtk_phy_cfg },

This is now even more suprising. Drop "realtek,usb2phy"

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, usbphy_rtk_dt_match);
> +
> +static struct platform_driver rtk_usb2phy_driver = {
> +	.probe		= rtk_usb2phy_probe,
> +	.remove_new	= rtk_usb2phy_remove,
> +	.driver		= {
> +		.name	= "rtk-usb2phy",
> +		.owner	= THIS_MODULE,

??? Didn't you base your driver on some really, really ancient code
(like 5 years old)? If so, please don't.

Run coccicenelle/coccicheck, smatch and sparse, to avoid common mistakes.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY

[Krzysztof Kozlowski]

On 14/06/2023 11:28, Stanley Chang wrote:
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 3.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---

All my comments apply here as well.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 5/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 3.0 PHY

[Krzysztof Kozlowski]

On 14/06/2023 11:28, Stanley Chang wrote:
> Add the documentation explain the property about Realtek USB PHY driver.
> 
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the  USB 3.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v3 to v4 change:
>     1. Remove the parameter and non hardware properties from dts.
>     2. Using the compatible data included the config and parameter
>        in driver.
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3.
>     2. Add more description about Realtek RTD SoCs architecture.
>     3. Removed parameter v1 support for simplification.
>     4. Revised the compatible name for fallback compatible.
>     5. Remove some properties that can be set in the driver.
> v1 to v2 change:
>     Add phy-cells for generic phy driver
> ---
>  .../bindings/phy/realtek,usb3phy.yaml         | 105 ++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml b/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> new file mode 100644
> index 000000000000..0f849cf942e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,usb3phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs USB 3.0 PHY
> +
> +maintainers:
> +  - Stanley Chang <stanley_chang@realtek.com>
> +
> +description:
> +  Realtek USB 3.0 PHY support the digital home center (DHC) RTD series SoCs.
> +  The USB 3.0 PHY driver is designed to support the XHCI controller. The SoCs
> +  support multiple XHCI controllers. One PHY device node maps to one XHCI
> +  controller.
> +
> +  RTD1295/RTD1619 SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on some
> +  controllers.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1319/RTD1619b SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#2.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1319d SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each xhci maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#0.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtd1295-usb3phy
> +          - realtek,rtd1319-usb3phy
> +          - realtek,rtd1319d-usb3phy
> +          - realtek,rtd1619-usb3phy
> +          - realtek,rtd1619b-usb3phy
> +      - const: realtek,usb3phy

Drop last compatible, it is not used now. Does not make sense.

> +
> +  reg:
> +    description: PHY data registers

Drop description, it's obvious.

> +    maxItems: 1
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  nvmem-cells:
> +    maxItems: 1
> +    description: A phandle to the tx lfps swing trim data provided by
> +      a nvmem device, if unspecified, default values shall be used.
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: usb_u3_tx_lfps_swing_trim
> +
> +  realtek,amplitude-control-coarse-tuning:
> +    description:
> +      This adjusts the signal amplitude for normal operation and beacon LFPS.
> +      This value is a parameter for coarse tuning.
> +      For different boards, if the default value is inappropriate, this
> +      property can be assigned to adjust.

default:

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 255
> +
> +  realtek,amplitude-control-fine-tuning:
> +    description:
> +      This adjusts the signal amplitude for normal operation and beacon LFPS.
> +      This value is used for fine-tuning parameters.
> +    $ref: /schemas/types.yaml#/definitions/uint32

default:

> +    minimum: 0
> +    maximum: 65535
> +


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy