Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1033832AbbKFUrf (ORCPT ); Fri, 6 Nov 2015 15:47:35 -0500 Received: from mail-yk0-f178.google.com ([209.85.160.178]:35033 "EHLO mail-yk0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030221AbbKFUrb convert rfc822-to-8bit (ORCPT ); Fri, 6 Nov 2015 15:47:31 -0500 MIME-Version: 1.0 In-Reply-To: <1446634498-11902-1-git-send-email-blogic@openwrt.org> References: <1446634498-11902-1-git-send-email-blogic@openwrt.org> Date: Fri, 6 Nov 2015 22:47:30 +0200 Message-ID: Subject: Re: [PATCH] phy: ralink-usb: add driver for Mediatek/Ralink From: Andy Shevchenko To: John Crispin Cc: Kishon Vijay Abraham I , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7562 Lines: 239 On Wed, Nov 4, 2015 at 12:54 PM, John Crispin wrote: > Add a driver to setup the USB phy on Mediatek/Ralink SoCs. > The driver is trivial and only sets the power and host/device mode. > +/* > + * Ralink USB phy driver > + * > + * Copyright (C) 2014 John Crispin > + * > + * Based on code from > + * Allwinner Technology Co., Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define RT_SYSC_REG_SYSCFG1 0x014 > +#define RT_SYSC_REG_CLKCFG1 0x030 > +#define RT_SYSC_REG_USB_PHY_CFG 0x05c > + > +#define RT_RSTCTRL_UDEV BIT(25) > +#define RT_RSTCTRL_UHST BIT(22) > +#define RT_SYSCFG1_USB0_HOST_MODE BIT(10) > + > +#define MT7620_CLKCFG1_UPHY0_CLK_EN BIT(25) > +#define MT7620_CLKCFG1_UPHY1_CLK_EN BIT(22) > +#define RT_CLKCFG1_UPHY1_CLK_EN BIT(20) > +#define RT_CLKCFG1_UPHY0_CLK_EN BIT(18) > + > +#define USB_PHY_UTMI_8B60M BIT(1) > +#define UDEV_WAKEUP BIT(0) > + > +static atomic_t usb_pwr_ref = ATOMIC_INIT(0); > +static struct reset_control *rstdev; > +static struct reset_control *rsthost; > +static u32 phy_clk; > +static struct phy *rt_phy; > + > +static void usb_phy_enable(int state) > +{ > + if (state) > + rt_sysc_m32(0, phy_clk, RT_SYSC_REG_CLKCFG1); > + else > + rt_sysc_m32(phy_clk, 0, RT_SYSC_REG_CLKCFG1); > + mdelay(100); Where 100 comes from? Perhaps comment line? > +} > + > +static int ralink_usb_phy_init(struct phy *_phy) > +{ > + return 0; > +} > + > +static int ralink_usb_phy_exit(struct phy *_phy) > +{ > + return 0; > +} > + > +static int ralink_usb_phy_power_on(struct phy *_phy) _phy -> phy ? > +{ > + if (atomic_inc_return(&usb_pwr_ref) == 1) { if (!atomic_… != 1) return 0; ? > + int host = 1; > + u32 t; > + > + usb_phy_enable(1); > + > + if (host) { > + rt_sysc_m32(0, RT_SYSCFG1_USB0_HOST_MODE, > + RT_SYSC_REG_SYSCFG1); > + if (!IS_ERR(rsthost)) > + reset_control_deassert(rsthost); > + if (!IS_ERR(rstdev)) > + reset_control_deassert(rstdev); > + } else { > + rt_sysc_m32(RT_SYSCFG1_USB0_HOST_MODE, 0, > + RT_SYSC_REG_SYSCFG1); > + if (!IS_ERR(rstdev)) > + reset_control_deassert(rstdev); > + } > + mdelay(100); Same question about 100 as above. Aha, what about usb_phy_enable(host); if (host && !IS_ERR(rsthost)) …; if (!IS_ERR(rstdev)) …; ? > + > + t = rt_sysc_r32(RT_SYSC_REG_USB_PHY_CFG); > + dev_info(&_phy->dev, "remote usb device wakeup %s\n", > + (t & UDEV_WAKEUP) ? ("enabbled") : ("disabled")); > + if (t & USB_PHY_UTMI_8B60M) > + dev_info(&_phy->dev, "UTMI 8bit 60MHz\n"); > + else > + dev_info(&_phy->dev, "UTMI 16bit 30MHz\n"); > + } > + > + return 0; > +} > + > +static int ralink_usb_phy_power_off(struct phy *_phy) > +{ > + if (atomic_dec_return(&usb_pwr_ref) == 0) { > + usb_phy_enable(0); Okay, it seems you have to use atomic variable result as parameter to off() / on(). Can you refactor them to have a common code together? > + if (!IS_ERR(rstdev)) > + reset_control_assert(rstdev); > + if (!IS_ERR(rsthost)) > + reset_control_assert(rsthost); > + } > + > + return 0; > +} > + > +static struct phy_ops ralink_usb_phy_ops = { > + .init = ralink_usb_phy_init, > + .exit = ralink_usb_phy_exit, > + .power_on = ralink_usb_phy_power_on, > + .power_off = ralink_usb_phy_power_off, > + .owner = THIS_MODULE, > +}; > + > +static struct phy *ralink_usb_phy_xlate(struct device *dev, > + struct of_phandle_args *args) > +{ > + return rt_phy; > +} > + > +static const struct of_device_id ralink_usb_phy_of_match[] = { > + { > + .compatible = "ralink,rt3xxx-usbphy", > + .data = (void *) (RT_CLKCFG1_UPHY1_CLK_EN | > + RT_CLKCFG1_UPHY0_CLK_EN) > + }, { > + .compatible = "ralink,mt7620a-usbphy", > + .data = (void *) (MT7620_CLKCFG1_UPHY1_CLK_EN | > + MT7620_CLKCFG1_UPHY0_CLK_EN) }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, ralink_usb_phy_of_match); > + > +static int ralink_usb_phy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct phy_provider *phy_provider; > + const struct of_device_id *match; > + > + match = of_match_device(ralink_usb_phy_of_match, &pdev->dev); if (!match) return -ENODEV; > + phy_clk = (int) match->data; > + > + rsthost = devm_reset_control_get(&pdev->dev, "host"); > + rstdev = devm_reset_control_get(&pdev->dev, "device"); > + > + rt_phy = devm_phy_create(dev, NULL, &ralink_usb_phy_ops, NULL); > + if (IS_ERR(rt_phy)) { > + dev_err(dev, "failed to create PHY\n"); > + return PTR_ERR(rt_phy); > + } > + > + phy_provider = devm_of_phy_provider_register(dev, ralink_usb_phy_xlate); > + > + return PTR_ERR_OR_ZERO(phy_provider); > +} > + > +static struct platform_driver ralink_usb_phy_driver = { > + .probe = ralink_usb_phy_probe, > + .driver = { > + .of_match_table = ralink_usb_phy_of_match, > + .name = "ralink-usb-phy", > + } > +}; > +module_platform_driver(ralink_usb_phy_driver); > + > +MODULE_DESCRIPTION("Ralink USB phy driver"); > +MODULE_AUTHOR("John Crispin "); > +MODULE_LICENSE("GPL v2"); > -- > 1.7.10.4 > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/