Re: [PATCH v3 1/2] i2c: tegra: add separate function for config_load

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2289 bytes --]

On Mon, Apr 18, 2016 at 06:45:54PM +0530, Shardar Shariff Md wrote:
> - Define separate function for configuration load register handling
> to make it use by different functions later.
> - Instead of calculating timeout for the config load during init,
> calculate it when config load register is written. Also use the
> msecs_to_jiffies for timeout calculation instead of macro HZ.

Looking really good now. One minor nit: If you need to resort to lists
in the commit message, it's usually a sign that you can split things up
into further patches. In this particular case I think moving the timeout
computation can be considered implicit in splitting apart the function.
That is, I'd simply skip the second list item (and remove the - from the
first line).

> Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
> 
> Changes since v1:
> - Add separate function for config load handling
> ---

Almost perfect. The changelog should go *below* the --- separator. This
is somewhat tricky to do because the separator will only be added by git
format-patch. You can either manually move it after git format-patch or
add the separator to the commit message. If you do the latter you'll get
two separators, but git send-email/apply/am will do the right things and
skip everything after the first separator and the beginning of the diff.

Finally, one nitpick below, which you don't necessarily have to address.

>  drivers/i2c/busses/i2c-tegra.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index d764d64..6235f16 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -423,12 +423,31 @@ static inline void tegra_i2c_clock_disable(struct tegra_i2c_dev *i2c_dev)
>  		clk_disable(i2c_dev->fast_clk);
>  }
>  
> +static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
> +{
> +	unsigned long timeout;
> +
> +	if (i2c_dev->hw->has_config_load_reg) {

You could move the timeout variable declaration into the conditional
block and initialize it immediately. That saves one line of code and
restricts the scope of the variable to the conditional.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]