Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 23 Aug 2020 19:33:45 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Nagarjuna Kristam <nkristam@nvidia.com>
Cc:     kishon@ti.com, thierry.reding@gmail.com,
        devicetree@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V5 3/7] phy: tegra: xusb: Add USB2 pad power control
 support for Tegra210
Message-ID: <20200823140345.GP2639@vkoul-mobl>
References: <1595238948-20531-1-git-send-email-nkristam@nvidia.com>
 <1595238948-20531-4-git-send-email-nkristam@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1595238948-20531-4-git-send-email-nkristam@nvidia.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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