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

[Florian Fainelli <f.fainelli@gmail.com>]

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