Re: [PATCH v3 3/5] phy: cix: Add CIX Sky1 USB3 PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] Incorrect format specifier for printing `device_node` pointer.
- [Low] Missing `devm_mutex_init()` for devm-allocated mutex.
- [Low] Missing explicit inclusion of `<linux/property.h>`.
- [Low] Unused data structures and enumerations in the header file.
--

commit 5cad48211eb5758f9a40687789544cb07846cb4a
Author: Hongliang Yang <hongliang.yang@cixtech.com>

phy: cix: Add CIX Sky1 USB3 PHY driver

Add PHY driver for the USB3 PHY found on CIX Sky1 SoCs. The driver
handles initialization, reset, and clock management for the
multi-port USB3 PHY.

> diff --git a/drivers/phy/cix/phy-cix-usb3.c b/drivers/phy/cix/phy-cix-usb3.c
> new file mode 100644
> index 0000000000000..0f074af84e909
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usb3.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * phy driver for cdn_sd0803_t7g_typea
> + *
> + * Author: Hongliang Yang <hongliang.yang@cixtech.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include "phy-cix-usbdp.h"

[Severity: Low]
Since the driver uses device_for_each_child_node_scoped(), should
<linux/property.h> be explicitly included here to avoid relying on implicit
dependencies from other headers?

[ ... ]

> +static int cix_u3phy_probe(struct platform_device *pdev)
> +{
> +	struct cix_u3phy *u3phy;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *child_np;
> +	struct phy *phy;
> +	int ret;
> +
> +	u3phy = devm_kzalloc(dev, sizeof(*u3phy), GFP_KERNEL);
> +	if (!u3phy)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, u3phy);
> +	u3phy->dev = dev;
> +	u3phy->cfg = device_get_match_data(dev);
> +	if (!u3phy->cfg) {
> +		dev_err(dev, "no OF data can be matched with %p node\n", np);

[Severity: Low]
Will this print the raw pointer address instead of the device node name?
Since np is a device_node pointer, should the %pOF format specifier be
used instead?

> +		return -EINVAL;
> +	}
> +	u3phy->reset = devm_reset_control_get_exclusive(dev, "core");

[ ... ]

> +	u3phy->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(u3phy->base))
> +		return PTR_ERR(u3phy->base);
> +	u3phy->phy_regmap = devm_regmap_init(dev, NULL, u3phy, &cix_u3phy_regmap_cfg);
> +	if (IS_ERR(u3phy->phy_regmap)) {
> +		dev_err(dev, "failed to remap phy register\n");
> +		return PTR_ERR(u3phy->phy_regmap);
> +	}
> +	mutex_init(&u3phy->mutex);

[Severity: Low]
Since u3phy is allocated using devm_kzalloc(), the memory will be
automatically freed on driver unbind. When initialized with mutex_init(),
mutex_destroy() won't be called automatically, which skips cleanup steps
expected by the resource management API for devm-allocated mutexes. Should
devm_mutex_init() be used here instead?

> +	device_for_each_child_node_scoped(dev, child_fn) {
> +		const char *name = fwnode_get_name(child_fn);

[ ... ]

> diff --git a/drivers/phy/cix/phy-cix-usbdp.h b/drivers/phy/cix/phy-cix-usbdp.h
> new file mode 100644
> index 0000000000000..6ec7eb5c81aa9
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usbdp.h
> @@ -0,0 +1,310 @@

[ ... ]

> +/* gop status address */
> +#define GOP_STATUS_ADDRESS 0x83E05000
> +#define GOP_STATUS_SIZE 0x04
> +
> +struct gop_status {
> +	unsigned char phy_status[4];
> +};
> +
> +/*
> + * 0: usb
> + * 1: 2 lane usb+ 2 lane dp
> + * 2: usb device
> + * 3: usb2.0 + 4 lane dp
> + */
> +enum phy_role {
> +	USB_ROLE_NONE,
> +	USB_ROLE_HOST,
> +	USB_ROLE_DEVICE,
> +	USB_ROLE_HOST_20,
> +};

[Severity: Low]
Are this struct and enum needed? Neither gop_status nor phy_role appear to
be used anywhere in the provided code.

> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903023521.2064986-1-hongliang.yang@cixtech.com?part=3

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