Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] phy: bcm63xx-usbh: Add BCM63xx USBH driver
To:     =?UTF-8?Q?=c3=81lvaro_Fern=c3=a1ndez_Rojas?= <noltari@gmail.com>,
        simon@fire.lp0.eu, jonas.gorski@gmail.com, kishon@ti.com,
        vkoul@kernel.org, robh+dt@kernel.org, f.fainelli@gmail.com,
        bcm-kernel-feedback-list@broadcom.com, p.zabel@pengutronix.de,
        krzk@kernel.org, gregkh@linuxfoundation.org, alcooperx@gmail.com,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
References: <20200616083408.3426435-1-noltari@gmail.com>
 <20200616083408.3426435-3-noltari@gmail.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <338262f0-bfe6-759c-c7f9-af7b62f5144d@gmail.com>
Date:   Tue, 16 Jun 2020 10:22:35 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Firefox/68.0 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <20200616083408.3426435-3-noltari@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@gmail.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