From: Cixi Geng <cixi.geng@linux.dev>
To: Baolin Wang <baolin.wang@linux.alibaba.com>,
gregkh@linuxfoundation.org, orsonzhai@gmail.com,
zhang.lyra@gmail.com, arnd@arndb.de, tony@atomide.com,
felipe.balbi@linux.intel.com, paul@crapouillou.net,
linus.walleij@linaro.org, cixi.geng1@unisoc.com,
gengcixi@gmail.com
Cc: linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [RFC PATCH v1] usb/phy add sprd ums512 usbphy
Date: Sat, 18 Mar 2023 23:37:28 +0800 [thread overview]
Message-ID: <be5ddb68b247b8d3b7305ef46d703d0ba2b3753a.camel@linux.dev> (raw)
In-Reply-To: <01d7b3c7-1514-5d8c-fc88-11b3d806496f@linux.alibaba.com>
On Mon, 2023-03-13 at 17:11 +0800, Baolin Wang wrote:
>
>
> On 3/13/2023 1:14 AM, Cixi Geng wrote:
> > From: Cixi Geng <cixi.geng1@unisoc.com>
> >
> > This driver is support USB2 phy for Spreadtrum UMS512 SOC's,
> >
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > ---
> > drivers/usb/phy/Kconfig | 10 +
> > drivers/usb/phy/Makefile | 1 +
> > drivers/usb/phy/phy-sprd-ums512.c | 511
> > ++++++++++++++++++++++++++++++
> > drivers/usb/phy/phy-sprd-ums512.h | 39 +++
> > 4 files changed, 561 insertions(+)
> > create mode 100644 drivers/usb/phy/phy-sprd-ums512.c
> > create mode 100644 drivers/usb/phy/phy-sprd-ums512.h
> >
> > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> > index 5f629d7cad64..fa5564e6f3a3 100644
> > --- a/drivers/usb/phy/Kconfig
> > +++ b/drivers/usb/phy/Kconfig
> > @@ -158,6 +158,16 @@ config USB_TEGRA_PHY
> > This driver provides PHY support for the USB controllers
> > found
> > on NVIDIA Tegra SoC's.
> >
> > +config USB_SPRD_UMS512_PHY
> > + tristate "Spreadtrum ums512 USB2 PHY Driver"
> > + depends on ARCH_SPRD || COMPILE_TEST
> > + select USB_PHY
> > + select EXTCON_USB_GPIO
> > + help
> > + Enable this to support the SPRD ums512 USB2 PHY that is
> > part of SOC.
> > + This driver takes care of all the PHY functionality,
> > normally paired with
> > + DesignWare USB20 (DRD) Controller.
> > +
> > config USB_ULPI
> > bool "Generic ULPI Transceiver Driver"
> > depends on ARM || ARM64 || COMPILE_TEST
> > diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> > index e5d619b4d8f6..ce45ee0f12a8 100644
> > --- a/drivers/usb/phy/Makefile
> > +++ b/drivers/usb/phy/Makefile
> > @@ -22,4 +22,5 @@ obj-$(CONFIG_USB_MV_OTG) += phy-mv-
> > usb.o
> > obj-$(CONFIG_USB_MXS_PHY) += phy-mxs-usb.o
> > obj-$(CONFIG_USB_ULPI) += phy-ulpi.o
> > obj-$(CONFIG_USB_ULPI_VIEWPORT) += phy-ulpi-
> > viewport.o
> > +obj-$(CONFIG_USB_SPRD_UMS512_PHY) += phy-sprd-ums512.o
> > obj-$(CONFIG_KEYSTONE_USB_PHY) += phy-keystone.o
> > diff --git a/drivers/usb/phy/phy-sprd-ums512.c
> > b/drivers/usb/phy/phy-sprd-ums512.c
> > new file mode 100644
> > index 000000000000..025f45e8d509
> > --- /dev/null
> > +++ b/drivers/usb/phy/phy-sprd-ums512.c
> > @@ -0,0 +1,511 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Unisoc USB PHY driver
> > + *
> > + * Copyright (C) 2023 Unisoc Inc.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/otg.h>
> > +#include <linux/usb/phy.h>
> > +#include <uapi/linux/usb/charger.h>
> > +
> > +#include "phy-sprd-ums512.h"
> > +
> > +struct sprd_hsphy {
> > + struct device *dev;
> > + struct usb_phy phy;
> > + struct regulator *vdd;
> > + struct regmap *hsphy_glb;
> > + struct regmap *ana_g2;
> > + struct regmap *pmic;
> > + u32 vdd_vol;
> > + atomic_t reset;
> > + atomic_t inited;
> > + bool is_host;
> > +};
> > +
> > +#define TUNEHSAMP_2_6MA (3 << 25)
> > +#define TFREGRES_TUNE_VALUE (0x14 << 19)
> > +#define SC2730_CHARGE_STATUS 0x1b9c
> > +#define BIT_CHG_DET_DONE BIT(11)
> > +#define BIT_SDP_INT BIT(7)
> > +#define BIT_DCP_INT BIT(6)
> > +#define BIT_CDP_INT BIT(5)
> > +
> > +static enum usb_charger_type sc27xx_charger_detect(struct regmap
> > *regmap)
> > +{
> > + enum usb_charger_type type;
> > + u32 status = 0, val;
> > + int ret, cnt = 10;
> > +
> > + do {
> > + ret = regmap_read(regmap, SC2730_CHARGE_STATUS,
> > &val);
> > + if (ret)
> > + return UNKNOWN_TYPE;
> > +
> > + if (val & BIT_CHG_DET_DONE) {
> > + status = val & (BIT_CDP_INT | BIT_DCP_INT |
> > BIT_SDP_INT);
> > + break;
> > + }
> > +
> > + msleep(200);
> > + } while (--cnt > 0);
> > +
> > + switch (status) {
> > + case BIT_CDP_INT:
> > + type = CDP_TYPE;
> > + break;
> > + case BIT_DCP_INT:
> > + type = DCP_TYPE;
> > + break;
> > + case BIT_SDP_INT:
> > + type = SDP_TYPE;
> > + break;
> > + default:
> > + type = UNKNOWN_TYPE;
> > + }
> > +
> > + return type;
> > +}
>
> Please do not add duplicate code, and use the
> sprd_pmic_detect_charger_type() instead, which had been exported in
> the
> mainline.
ok, I will remove this next patchset.
>
> > +
> > +static inline void sprd_hsphy_enable(struct sprd_hsphy *sprd_phy)
> > +{
> > + u32 reg, msk;
> > +
> > + /* enable usb module */
> > + reg = msk = (MASK_AON_APB_OTG_UTMI_EB |
> > MASK_AON_APB_ANA_EB);
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_APB_EB1, msk, reg);
> > + reg = msk = MASK_AON_APB_CGM_OTG_REF_EN |
> > + MASK_AON_APB_CGM_DPHY_REF_EN;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_CGM_REG1, msk, reg);
> > +}
> > +
> > +static inline void sprd_hsphy_power_down(struct sprd_hsphy
> > *sprd_phy)
> > +{
> > + u32 reg, msk;
> > +
> > + /* usb power down */
> > + reg = msk = (MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_PS_PD_L |
> > + MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_PS_PD_S);
> > + regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_BATTER_PLL, msk,
> > reg);
> > +}
> > +
> > +static inline void sprd_hsphy_reset_core(struct sprd_hsphy
> > *sprd_phy)
> > +{
> > + u32 reg, msk;
> > +
> > + /* Reset PHY */
> > + reg = msk = MASK_AON_APB_OTG_PHY_SOFT_RST |
> > MASK_AON_APB_OTG_UTMI_SOFT_RST;
> > +
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_APB_RST1, msk, reg);
> > + /* USB PHY reset need to delay 20ms~30ms */
> > + usleep_range(20000, 30000);
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_APB_RST1, msk, 0);
> > +}
> > +
> > +static int sprd_hostphy_set(struct usb_phy *u_phy, int on)
> > +{
> > + struct sprd_hsphy *sprd_phy = container_of(u_phy, struct
> > sprd_hsphy, phy);
> > + u32 reg, msk;
> > + int ret = 0;
> > +
> > + if (on) {
> > + msk = MASK_AON_APB_USB2_PHY_IDDIG;
> > + ret |= regmap_update_bits(sprd_phy->hsphy_glb,
> > + REG_AON_APB_OTG_PHY_CTRL, msk, 0);
> > +
> > + msk =
> > MASK_ANLG_PHY_G2_DBG_SEL_ANALOG_USB20_USB20_DMPULLDOWN |
> > + MASK_ANLG_PHY_G2_DBG_SEL_ANALOG_USB20_USB20
> > _DPPULLDOWN;
> > + ret |= regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_REG_SEL_CFG_0,
> > + msk, msk);
> > +
> > + msk =
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DMPULLDOWN |
> > + MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DPPULLD
> > OWN;
> > + ret |= regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL
> > 2,
> > + msk, msk);
> > +
> > + reg = 0x200;
> > + msk = MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_RESERVED;
> > + ret |= regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL
> > 1,
> > + msk, reg);
> > + sprd_phy->is_host = true;
> > + } else {
> > + reg = msk = MASK_AON_APB_USB2_PHY_IDDIG;
> > + ret |= regmap_update_bits(sprd_phy->hsphy_glb,
> > + REG_AON_APB_OTG_PHY_CTRL, msk, reg);
> > +
> > + msk =
> > MASK_ANLG_PHY_G2_DBG_SEL_ANALOG_USB20_USB20_DMPULLDOWN |
> > + MASK_ANLG_PHY_G2_DBG_SEL_ANALOG_USB20_USB20
> > _DPPULLDOWN;
> > + ret |= regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_REG_SEL_CFG_0,
> > + msk, msk);
> > +
> > + msk =
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DMPULLDOWN |
> > + MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DPPULLD
> > OWN;
> > + ret |= regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL
> > 2,
> > + msk, 0);
> > +
> > + msk = MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_RESERVED;
> > + ret |= regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL
> > 1,
> > + msk, 0);
> > + sprd_phy->is_host = false;
> > + }
> > + return ret;
> > +}
> > +
> > +static int sprd_hsphy_init(struct usb_phy *u_phy)
> > +{
> > + struct sprd_hsphy *sprd_phy = container_of(u_phy, struct
> > sprd_hsphy, phy);
> > + u32 reg, msk;
> > + int ret;
> > +
> > + if (atomic_read(&sprd_phy->inited)) {
> > + dev_dbg(u_phy->dev, "%s is already inited!\n",
> > __func__);
> > + return 0;
> > + }
> > +
> > + /* Turn On VDD */
> > + regulator_set_voltage(sprd_phy->vdd, sprd_phy->vdd_vol,
> > sprd_phy->vdd_vol);
> > + if (!regulator_is_enabled(sprd_phy->vdd)) {
> > + ret = regulator_enable(sprd_phy->vdd);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + sprd_hsphy_enable(sprd_phy);
> > + regmap_update_bits(sprd_phy->ana_g2,
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_ISO_SW,
> > + MASK_ANLG_PHY_G2_ANALOG_USB20_USB20
> > _ISO_SW_EN, 0);
> > +
> > + /* usb phy power */
> > + msk = (MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_PS_PD_L |
> > + MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_PS_PD_S);
> > + regmap_update_bits(sprd_phy->ana_g2,
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_BATTER_PLL,
> > + msk, 0);
> > +
> > + /* usb vbus valid */
> > + reg = msk = MASK_AON_APB_OTG_VBUS_VALID_PHYREG;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_OTG_PHY_TEST, msk, reg);
> > +
> > + reg = msk = MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_VBUSVLDEXT;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL1,
> > + msk, reg);
> > +
> > + /* for SPRD phy utmi_width sel */
> > + reg = msk = MASK_AON_APB_UTMI_WIDTH_SEL;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_OTG_PHY_CTRL, msk, reg);
> > +
> > + reg = msk =
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DATABUS16_8;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL1,
> > + msk, reg);
> > +
> > + reg = TUNEHSAMP_2_6MA;
> > + msk = MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_TUNEHSAMP;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_TRIMMING,
> > + msk, reg);
> > +
> > + reg = TFREGRES_TUNE_VALUE;
> > + msk = MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_TFREGRES;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_TRIMMING,
> > + msk, reg);
> > +
> > + if (!atomic_read(&sprd_phy->reset)) {
> > + sprd_hsphy_reset_core(sprd_phy);
> > + atomic_set(&sprd_phy->reset, 1);
> > + }
> > +
> > + atomic_set(&sprd_phy->inited, 1);
> > +
> > + return 0;
> > +}
> > +
> > +static void sprd_hsphy_shutdown(struct usb_phy *u_phy)
> > +{
> > + struct sprd_hsphy *sprd_phy = container_of(u_phy, struct
> > sprd_hsphy, phy);
> > + u32 reg, msk;
> > +
> > + if (!atomic_read(&sprd_phy->inited)) {
> > + dev_dbg(sprd_phy->dev, "%s is already shut down\n",
> > __func__);
> > + return;
> > + }
> > +
> > + /* usb vbus */
> > + msk = MASK_AON_APB_OTG_VBUS_VALID_PHYREG;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_OTG_PHY_TEST, msk, 0);
> > + msk = MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_VBUSVLDEXT;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL1, msk,
> > 0);
> > +
> > + sprd_hsphy_power_down(sprd_phy);
> > + regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_ISO_SW,
> > + msk, reg);
> > +
> > + /* usb cgm ref */
> > + msk = MASK_AON_APB_CGM_OTG_REF_EN |
> > + MASK_AON_APB_CGM_DPHY_REF_EN;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_CGM_REG1, msk, 0);
> > +
> > + if (regulator_is_enabled(sprd_phy->vdd))
> > + regulator_disable(sprd_phy->vdd);
> > +
> > + atomic_set(&sprd_phy->inited, 0);
> > + atomic_set(&sprd_phy->reset, 0);
> > +}
> > +
> > +static ssize_t vdd_voltage_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct sprd_hsphy *sprd_phy = dev_get_drvdata(dev);
> > +
> > + if (!sprd_phy)
> > + return -EINVAL;
> > +
> > + return sprintf(buf, "%d\n", sprd_phy->vdd_vol);
> > +}
> > +
> > +static ssize_t vdd_voltage_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t size)
> > +{
> > + struct sprd_hsphy *sprd_phy = dev_get_drvdata(dev);
> > + u32 vol;
> > +
> > + if (!sprd_phy)
> > + return -EINVAL;
> > +
> > + if (kstrtouint(buf, 16, &vol) < 0)
> > + return -EINVAL;
> > +
> > + if (vol < 1200000 || vol > 3750000) {
> > + dev_err(dev, "Invalid voltage value %d\n", vol);
> > + return -EINVAL;
> > + }
> > + sprd_phy->vdd_vol = vol;
> > +
> > + return size;
> > +}
> > +static DEVICE_ATTR_RW(vdd_voltage);
>
> Why add voltage setting interface in a usb phy driver? should move to
> regulator driver?
>
> > +
> > +static struct attribute *usb_hsphy_attrs[] = {
> > + &dev_attr_vdd_voltage.attr,
> > + NULL
> > +};
> > +ATTRIBUTE_GROUPS(usb_hsphy);
> > +
> > +static int sprd_hsphy_vbus_notify(struct notifier_block *nb,
> > + unsigned long event, void *data)
> > +{
> > + struct usb_phy *u_phy = container_of(nb, struct usb_phy,
> > vbus_nb);
> > + struct sprd_hsphy *sprd_phy = container_of(u_phy, struct
> > sprd_hsphy, phy);
> > + u32 reg, msk;
> > +
> > + if (sprd_phy->is_host || u_phy->last_event == USB_EVENT_ID)
> > {
> > + dev_info(sprd_phy->dev, "USB PHY is host mode\n");
> > + return 0;
> > + }
> > +
> > + if (event) {
> > + /* usb vbus valid */
> > + reg = msk = MASK_AON_APB_OTG_VBUS_VALID_PHYREG;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > + REG_AON_APB_OTG_PHY_TEST, msk, reg);
> > +
> > + reg = msk =
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_VBUSVLDEXT;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL
> > 1, msk, reg);
> > + usb_phy_set_charger_state(u_phy,
> > USB_CHARGER_PRESENT);
> > + } else {
> > + /* usb vbus invalid */
> > + msk = MASK_AON_APB_OTG_VBUS_VALID_PHYREG;
> > + regmap_update_bits(sprd_phy->hsphy_glb,
> > REG_AON_APB_OTG_PHY_TEST,
> > + msk, 0);
> > + msk =
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_VBUSVLDEXT;
> > + regmap_update_bits(sprd_phy->ana_g2,
> > + REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL
> > 1, msk, 0);
> > + usb_phy_set_charger_state(u_phy,
> > USB_CHARGER_ABSENT);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static enum usb_charger_type sprd_hsphy_charger_detect(struct
> > usb_phy *u_phy)
> > +{
> > + struct sprd_hsphy *sprd_phy = container_of(u_phy, struct
> > sprd_hsphy, phy);
> > +
> > + return sc27xx_charger_detect(sprd_phy->pmic);
> > +}
> > +
> > +static int sprd_hsphy_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *regmap_np;
> > + struct platform_device *regmap_pdev;
> > + struct sprd_hsphy *sprd_phy;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > + struct usb_otg *otg;
> > +
> > + sprd_phy = devm_kzalloc(dev, sizeof(*sprd_phy),
> > GFP_KERNEL);
> > + if (!sprd_phy)
> > + return -ENOMEM;
> > +
> > + regmap_np = of_find_compatible_node(NULL, NULL,
> > "sprd,sc27xx-syscon");
> > + if (!regmap_np) {
> > + dev_err(dev, "unable to get syscon node\n");
> > + return -ENODEV;
> > + }
> > +
> > + regmap_pdev = of_find_device_by_node(regmap_np);
> > + if (!regmap_pdev) {
> > + dev_err(dev, "unable to get syscon platform
> > device\n");
> > + ret = -ENODEV;
> > + goto device_node_err;
> > + }
> > +
> > + sprd_phy->pmic = dev_get_regmap(regmap_pdev->dev.parent,
> > NULL);
> > + if (!sprd_phy->pmic) {
> > + dev_err(dev, "unable to get pmic regmap device\n");
> > + ret = -ENODEV;
> > + goto platform_device_err;
> > + }
> > +
> > + ret = of_property_read_u32(dev->of_node, "sprd,vdd-
> > voltage",
> > + &sprd_phy->vdd_vol);
>
> Where is the DT binding secription?
>
> > + if (ret < 0) {
> > + dev_err(dev, "unable to read ssphy vdd voltage\n");
> > + goto platform_device_err;
> > + }
> > +
> > + sprd_phy->vdd = devm_regulator_get(dev, "vdd");
>
> ditto.
> Please add the DT binding firstly.
next patchset I wiil add the bindings file.
>
> > + if (IS_ERR(sprd_phy->vdd)) {
> > + dev_err(dev, "unable to get ssphy vdd supply\n");
> > + ret = PTR_ERR(sprd_phy->vdd);
> > + goto platform_device_err;
> > + }
> > +
> > + ret = regulator_set_voltage(sprd_phy->vdd, sprd_phy-
> > >vdd_vol, sprd_phy->vdd_vol);
> > + if (ret < 0) {
> > + dev_err(dev, "fail to set ssphy vdd voltage at
> > %dmV\n", sprd_phy->vdd_vol);
> > + goto platform_device_err;
> > + }
> > +
> > + otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);
> > + if (!otg) {
> > + ret = -ENOMEM;
> > + goto platform_device_err;
> > + }
> > +
> > + sprd_phy->ana_g2 = syscon_regmap_lookup_by_phandle(dev-
> > >of_node,
> > + "sprd,syscon-anag2");
> > + if (IS_ERR(sprd_phy->ana_g2)) {
> > + dev_err(&pdev->dev, "ap USB anag2 syscon
> > failed!\n");
> > + ret = PTR_ERR(sprd_phy->ana_g2);
> > + goto platform_device_err;
> > + }
> > +
> > + sprd_phy->hsphy_glb = syscon_regmap_lookup_by_phandle(dev-
> > >of_node,
> > + "sprd,syscon-enable");
> > + if (IS_ERR(sprd_phy->hsphy_glb)) {
> > + dev_err(&pdev->dev, "ap USB aon apb syscon
> > failed!\n");
> > + ret = PTR_ERR(sprd_phy->hsphy_glb);
> > + goto platform_device_err;
> > + }
> > +
> > + sprd_hsphy_enable(sprd_phy);
> > +
> > + sprd_hsphy_power_down(sprd_phy);
>
> Why doing this? please add comments to explain these strange hardware
> operation.
the two actions seems redundant, it should in sprd_hsphy_init. I need
to test on the Soc. if not effect, I wiil remove it.
>
> > +
> > + sprd_phy->phy.dev = dev;
> > + sprd_phy->phy.label = "sprd-hsphy";
> > + sprd_phy->phy.otg = otg;
> > + sprd_phy->phy.init = sprd_hsphy_init;
> > + sprd_phy->phy.shutdown = sprd_hsphy_shutdown;
> > + sprd_phy->phy.set_vbus = sprd_hostphy_set;
> > + sprd_phy->phy.type = USB_PHY_TYPE_USB2;
> > + sprd_phy->phy.vbus_nb.notifier_call =
> > sprd_hsphy_vbus_notify;
> > + sprd_phy->phy.charger_detect = sprd_hsphy_charger_detect;
> > + otg->usb_phy = &sprd_phy->phy;
> > +
> > + platform_set_drvdata(pdev, sprd_phy);
> > +
> > + ret = usb_add_phy_dev(&sprd_phy->phy);
> > + if (ret) {
> > + dev_err(dev, "fail to add phy\n");
> > + goto platform_device_err;
> > + }
> > +
> > + ret = sysfs_create_groups(&dev->kobj, usb_hsphy_groups);
> > + if (ret)
> > + dev_warn(dev, "failed to create usb hsphy
> > attributes\n");
> > +
> > + if (extcon_get_state(sprd_phy->phy.edev, EXTCON_USB) > 0)
> > + usb_phy_set_charger_state(&sprd_phy->phy,
> > USB_CHARGER_PRESENT);
> > +
> > + dev_info(dev, "sprd usb phy probe ok !\n");
>
> Please remove these useless printing.
>
> > +
> > +platform_device_err:
> > +device_node_err:
>
> 2 same level label?
>
> > + of_node_put(regmap_np);
> > +
> > + return ret;
> > +}
> > +
> > +static int sprd_hsphy_remove(struct platform_device *pdev)
> > +{
> > + struct sprd_hsphy *sprd_phy = platform_get_drvdata(pdev);
> > +
> > + sysfs_remove_groups(&pdev->dev.kobj, usb_hsphy_groups);
> > + usb_remove_phy(&sprd_phy->phy);
> > + if (regulator_is_enabled(sprd_phy->vdd))
> > + regulator_disable(sprd_phy->vdd);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sprd_hsphy_match[] = {
> > + { .compatible = "sprd,ums512-phy"},
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, sprd_hsphy_match);
> > +
> > +static struct platform_driver sprd_hsphy_driver = {
> > + .probe = sprd_hsphy_probe,
> > + .remove = sprd_hsphy_remove,
> > + .driver = {
> > + .name = "sprd-hsphy",
> > + .of_match_table = sprd_hsphy_match,
> > + },
> > +};
> > +
> > +static int __init sprd_hsphy_driver_init(void)
> > +{
> > + return platform_driver_register(&sprd_hsphy_driver);
> > +}
> > +
> > +static void __exit sprd_hsphy_driver_exit(void)
> > +{
> > + platform_driver_unregister(&sprd_hsphy_driver);
> > +}
> > +
> > +late_initcall(sprd_hsphy_driver_init);
> > +module_exit(sprd_hsphy_driver_exit);
> > +
> > +MODULE_DESCRIPTION("UNISOC USB PHY driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/usb/phy/phy-sprd-ums512.h
> > b/drivers/usb/phy/phy-sprd-ums512.h
> > new file mode 100644
> > index 000000000000..903da0573eae
> > --- /dev/null
> > +++ b/drivers/usb/phy/phy-sprd-ums512.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ OR MIT */
> > +/*
> > + * Spreadtrum UMS512 SOC USB registers file
> > + *
> > + * Copyright C 2022, Spreadtrum Communications Inc.
> > + */
> > +
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DATABUS16_8
> > 0x10000000
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DMPULLDOWN 0x8
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_DPPULLDOWN 0x10
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_ISO_SW_EN 0x1
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_PS_PD_L 0x8
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_PS_PD_S 0x10
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_RESERVED 0xff
> > ff
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_TFREGRES 0x1f
> > 80000
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_TUNEHSAMP 0x60
> > 00000
> > +#define
> > MASK_ANLG_PHY_G2_ANALOG_USB20_USB20_VBUSVLDEXT 0x10
> > 000
> > +#define
> > MASK_ANLG_PHY_G2_DBG_SEL_ANALOG_USB20_USB20_DMPULLDOWN 0x2
> > +#define
> > MASK_ANLG_PHY_G2_DBG_SEL_ANALOG_USB20_USB20_DPPULLDOWN 0x4
> > +#define
> > MASK_AON_APB_ANA_EB 0x10
> > 00
> > +#define
> > MASK_AON_APB_CGM_DPHY_REF_EN 0x40
> > 0
> > +#define
> > MASK_AON_APB_CGM_OTG_REF_EN 0x10
> > 00
> > +#define
> > MASK_AON_APB_OTG_PHY_SOFT_RST 0x20
> > 0
> > +#define
> > MASK_AON_APB_OTG_UTMI_EB 0x10
> > 0
> > +#define
> > MASK_AON_APB_OTG_UTMI_SOFT_RST 0x10
> > 0
> > +#define
> > MASK_AON_APB_OTG_VBUS_VALID_PHYREG 0x10
> > 00000
> > +#define
> > MASK_AON_APB_USB2_PHY_IDDIG 0x8
> > +#define
> > MASK_AON_APB_UTMI_WIDTH_SEL 0x40
> > 000000
> > +#define
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_BATTER_PLL 0x00
> > 5c
> > +#define
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_ISO_SW 0x00
> > 70
> > +#define
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_TRIMMING 0x00
> > 64
> > +#define
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL1 0x00
> > 58
> > +#define
> > REG_ANLG_PHY_G2_ANALOG_USB20_USB20_UTMI_CTL2 0x00
> > 60
> > +#define
> > REG_ANLG_PHY_G2_ANALOG_USB20_REG_SEL_CFG_0 0x00
> > 74
> > +#define
> > REG_AON_APB_APB_EB1 0x00
> > 04
> > +#define
> > REG_AON_APB_APB_RST1 0x00
> > 10
> > +#define
> > REG_AON_APB_CGM_REG1 0x01
> > 38
> > +#define
> > REG_AON_APB_OTG_PHY_CTRL 0x02
> > 08
> > +#define
> > REG_AON_APB_OTG_PHY_TEST 0x02
> > 04
>
> Move them to the driver file and please rename the ugly macro names,
> too
> long :(
>
> And why not move the usb phy driver to be a generic phy driver? I
> mean
> move it to the drivers/phy.
Do you mean all the usb-phy need move to driver/phy? or just this
driver?
next prev parent reply other threads:[~2023-03-18 15:37 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-12 17:14 [RFC PATCH v1] usb/phy add sprd ums512 usbphy Cixi Geng
2023-03-13 6:31 ` Greg KH
2023-03-18 15:15 ` Cixi Geng
2023-03-13 9:11 ` Baolin Wang
2023-03-18 15:37 ` Cixi Geng [this message]
2023-03-21 2:03 ` Baolin Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=be5ddb68b247b8d3b7305ef46d703d0ba2b3753a.camel@linux.dev \
--to=cixi.geng@linux.dev \
--cc=arnd@arndb.de \
--cc=baolin.wang@linux.alibaba.com \
--cc=cixi.geng1@unisoc.com \
--cc=felipe.balbi@linux.intel.com \
--cc=gengcixi@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=orsonzhai@gmail.com \
--cc=paul@crapouillou.net \
--cc=tony@atomide.com \
--cc=zhang.lyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.