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@xxxxxxxxx>
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
