linux-kernel - Re: [PATCH 2/2] phy: bcm63xx-usbh: Add BCM63xx USBH driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <338262f0-bfe6-759c-c7f9-af7b62f5144d@gmail.com>
Date:   Tue, 16 Jun 2020 10:22:35 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Álvaro Fernández Rojas <noltari@...il.com>,
        simon@...e.lp0.eu, jonas.gorski@...il.com, kishon@...com,
        vkoul@...nel.org, robh+dt@...nel.org, f.fainelli@...il.com,
        bcm-kernel-feedback-list@...adcom.com, p.zabel@...gutronix.de,
        krzk@...nel.org, gregkh@...uxfoundation.org, alcooperx@...il.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] phy: bcm63xx-usbh: Add BCM63xx USBH driver



On 6/16/2020 1:34 AM, Álvaro Fernández Rojas wrote:
> Add BCM63xx USBH PHY driver for BMIPS.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>

This looks good to me at first glance, just a few comments below.

> ---
>  drivers/phy/broadcom/Kconfig            |  10 +
>  drivers/phy/broadcom/Makefile           |   1 +
>  drivers/phy/broadcom/phy-bcm63xx-usbh.c | 463 ++++++++++++++++++++++++
>  3 files changed, 474 insertions(+)
>  create mode 100644 drivers/phy/broadcom/phy-bcm63xx-usbh.c
> 
> diff --git a/drivers/phy/broadcom/Kconfig b/drivers/phy/broadcom/Kconfig
> index b29f11c19155..896506c7b1f8 100644
> --- a/drivers/phy/broadcom/Kconfig
> +++ b/drivers/phy/broadcom/Kconfig
> @@ -2,6 +2,16 @@
>  #
>  # Phy drivers for Broadcom platforms
>  #
> +config PHY_BCM63XX_USBH
> +	tristate "BCM63xx USBH PHY driver"
> +	depends on BMIPS_GENERIC || COMPILE_TEST
> +	depends on OF

I do not think you need to add the depends on OF here if you use
device_get_match_data() instead of of_device_get_match_data() and
devm_of_phy_provider_register() has an inline stub provided when
CONFIG_OF=n.

[snip]

> +static int __init bcm63xx_usbh_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bcm63xx_usbh_phy	*usbh;
> +	const struct bcm63xx_usbh_phy_variant *variant;
> +	struct resource *res;
> +	struct phy *phy;
> +	struct phy_provider *phy_provider;
> +
> +	usbh = devm_kzalloc(dev, sizeof(*usbh), GFP_KERNEL);
> +	if (!usbh)
> +		return -ENOMEM;
> +
> +	variant = of_device_get_match_data(dev);

We can use device_get_match_data() to be OF independent.

> +	if (!variant)
> +		return -EINVAL;
> +	memcpy(&usbh->variant, variant, sizeof(*variant));

I would just avoid marking the variant tables with __initconst, and just
reference them directly here.

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	usbh->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(usbh->base))
> +		return PTR_ERR(usbh->base);
> +
> +	usbh->reset = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(usbh->reset)) {
> +		if (PTR_ERR(usbh->reset) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get reset\n");
> +		return PTR_ERR(usbh->reset);
> +	}
> +
> +	if (variant->has_usb_clk) {
> +		usbh->usbh_clk = devm_clk_get(dev, "usbh");

You can use devm_clk_get_optional() which would save you from having to
record whether the clock is needed or not.

> +		if (IS_ERR(usbh->usbh_clk)) {
> +			if (PTR_ERR(usbh->usbh_clk) != -EPROBE_DEFER)
> +				dev_err(dev, "failed to get usbh clock\n");
> +			return PTR_ERR(usbh->usbh_clk);
> +		}
> +	} else {
> +		usbh->usbh_clk = NULL;
> +	}
> +
> +	if (variant->has_usb_ref_clk) {
> +		usbh->usb_ref_clk = devm_clk_get(dev, "usb_ref");

Likewise.
-- 
Florian