Re: [PATCH V5 5/7] phy: tegra: xusb: Add support for charger detect

From: Vinod Koul
Date: Mon Aug 31 2020 - 04:50:38 EST


On 20-07-20, 15:25, Nagarjuna Kristam wrote:

> +#define USB2_BATTERY_CHRG_OTGPADX_CTL0(x) (0x80 + (x) * 0x40)
> +#define PD_CHG BIT(0)
> +#define VDCD_DET_FILTER_EN BIT(4)
> +#define VDAT_DET BIT(5)
> +#define VDAT_DET_FILTER_EN BIT(8)
> +#define OP_SINK_EN BIT(9)
> +#define OP_SRC_EN BIT(10)
> +#define ON_SINK_EN BIT(11)
> +#define ON_SRC_EN BIT(12)
> +#define OP_I_SRC_EN BIT(13)
> +#define ZIP_FILTER_EN BIT(21)
> +#define ZIN_FILTER_EN BIT(25)
> +#define DCD_DETECTED BIT(26)
> +
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x) (0x84 + (x) * 0x40)
> +#define PD_VREG BIT(6)
> +#define VREG_LEV(x) (((x) & 0x3) << 7)
> +#define VREG_DIR(x) (((x) & 0x3) << 11)

GENMASK and FIELD_PREP please

> +static void
> +tegra_xusb_padctl_set_debounce_time(struct tegra_xusb_padctl *padctl,
> + u32 debounce)
> +{
> + u32 value;
> +
> + value = padctl_readl(padctl,
> + XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);

Single line

> +static void
> +tegra_xusb_padctl_charger_detect_on(struct tegra_xusb_padctl *padctl, u32 index)
> +{
> + u32 value;
> +
> + value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> + value &= ~USB2_OTG_PD_ZI;
> + padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +
> + value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> + value |= (USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
> + padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + value &= ~PD_CHG;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));

maybe a padctl_updatel() helper to do read, modify and write

> +static void tegra_xusb_padctl_dcd(struct tegra_xusb_padctl *padctl, u32 index)
> +{
> + u32 value;
> + unsigned int offset;
> + bool ret = false;
> +
> + /* Turn on IDP_SRC */
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + value |= OP_I_SRC_EN;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> + /* Turn on D- pull-down resistor */
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> + value |= USBON_RPD_OVRD_VAL;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> + /* Wait for TDCD_DBNC (DCD debounce), refer to BC1.2 spec Table 5 */
> + usleep_range(10000, 20000);
> +
> + offset = USB2_BATTERY_CHRG_OTGPADX_CTL0(index);
> + ret = readl_poll_timeout(padctl->regs + offset, value,
> + value & DCD_DETECTED, 20000,
> + TDCD_TIMEOUT_MS * 1000);
> + if (!ret)
> + dev_warn(padctl->dev, "%s: DCD timeout.", __func__);

Not dev_err() ? and continue after this?

> +static bool
> +tegra_xusb_padctl_primary_secondary(struct tegra_xusb_padctl *padctl, u32 index,
> + bool is_primary)
> +{
> + u32 value;
> + u32 config = is_primary ? (OP_SRC_EN | ON_SINK_EN) :
> + (ON_SRC_EN | OP_SINK_EN);
> + bool ret = false;
> +
> + if (is_primary)
> + /* data contact detection */
> + tegra_xusb_padctl_dcd(padctl, index);
> +
> + /* Source D- to D+ */
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + value |= config;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> + /*
> + * Wait for TVDPSRC_ON/TVDMSRC_ON(D+/- voltage source on time),
> + * refer to BC1.2 spec Table 5
> + */
> + msleep(40);
> +
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + if (is_primary)
> + ret = !!(value & VDAT_DET);
> + else
> + ret = !(value & VDAT_DET);

How about

ret = !(value & VDAT_DET);
if (is_primary)
ret = !ret;

> +#define VON_DIV2P0_DET BIT(0)
> +#define VON_DIV2P7_DET BIT(1)
> +#define VOP_DIV2P0_DET BIT(2)
> +#define VOP_DIV2P7_DET BIT(3)
> +
> +#define VREG_CUR_LEVEL_0 500
> +#define VREG_CUR_LEVEL_1 900
> +#define VREG_CUR_LEVEL_2 1500
> +#define VREG_CUR_LEVEL_3 2000

tabs or spaces, pick one please, not both

> +
> +#define IS_CUR_IN_RANGE(ma, low, high) \
> + ((ma >= VREG_CUR_LEVEL_##low) && (ma <= (VREG_CUR_LEVEL_##high - 1)))
> +#define VREG_LVL(ma, level) IS_CUR_IN_RANGE(ma, level, level + 1)
> +
> +static void tegra_xusb_padctl_vbus_pad_portection(struct tegra_xusb_port *port)
> +{
> + struct tegra_xusb_padctl *padctl = port->padctl;
> + int level = 0;
> + enum tegra_vbus_dir dir = TEGRA_VBUS_SINK;
> + int max_ua, min_ua;

reverse christmas tree please

> +static enum usb_charger_type tegra_xusb_charger_detect(struct usb_phy *usb_phy)
> +{
> + struct tegra_xusb_port *port = container_of(usb_phy,
> + struct tegra_xusb_port,
> + usb_phy);

how about:

struct tegra_xusb_port *port =
container_of(usb_phy, struct tegra_xusb_port, usb_phy);
--
~Vinod