Re: [PATCH V5 3/7] phy: tegra: xusb: Add USB2 pad power control support for Tegra210

[Vinod Koul <vkoul@kernel.org>]

On 20-07-20, 15:25, Nagarjuna Kristam wrote:
> Add USB2 pad power on and off API's for TEgra210 and provide its control
> via soc ops. It can be used by operations like charger detect to power on
> and off USB2 pad if needed.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> V5:
>  - Made tegra210_usb2_pad_power_on() and tegra210_usb2_pad_power_down() static.
> ---
> V4:
>  - No changes
> ---
> V3:
>  - Added Acked-by updates to commit message.
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 190 ++++++++++++++++++++++++++------------
>  1 file changed, 133 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 66bd461..2e5f71c 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -994,6 +994,128 @@ static int tegra210_xusb_padctl_id_override(struct tegra_xusb_padctl *padctl,
>  	return 0;
>  }
>  
> +static void tegra210_usb2_bias_pad_power_on(struct tegra_xusb_usb2_pad *pad)
> +{
> +	struct tegra_xusb_padctl *padctl = pad->base.padctl;
> +	u32 value;
> +
> +	if (pad->enable++ > 0)
> +		return;
> +
> +	dev_dbg(padctl->dev, "power on BIAS PAD & USB2 tracking\n");
> +
> +	if (clk_prepare_enable(pad->clk))
> +		dev_warn(padctl->dev, "failed to enable BIAS PAD & USB2 tracking\n");

do you want to proceed ahead even if clock is not enabled..?

> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
> +	value &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_MASK <<
> +		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
> +		   (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_MASK <<
> +		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT));
> +	value |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_VAL <<
> +		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
> +		 (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_VAL <<
> +		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT);

I recently found <linux/bitfield.h> I think this would look lot neater
if we use FIELD_PREP or u32_encode_bits() and you can drop shift defines

Since this is not new code but moved here, I will leave it upto you to
change this, either ways is fine by me.

-- 
~Vinod