Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Wolfram Sang <wsa@the-dreams.de>]

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

On Wed, Jun 10, 2020 at 09:33:11PM +0200, Ulrich Hecht wrote:
> Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and
> similar boards.
> 
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Thanks, Uli! Works fine here. Really nice to finally being able to
reboot now without WARNings.

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Some review comments:


> @@ -366,7 +369,7 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
>  
>  static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
>  {
> -	unsigned char data;
> +	unsigned char data = 0;

Please rebase against i2c/for-next. 'data' is gone since recently.

> -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
> -			      struct i2c_msg *msgs,
> -			      int num)
> +static int xfer(struct sh_mobile_i2c_data *pd, struct i2c_msg *msgs, int num)

'xfer' is too generic IMO. '__sh_mobile_i2c_xfer' maybe?

> -	pm_runtime_get_sync(pd->dev);
> +	if (!pd->atomic_xfer)
> +		pm_runtime_get_sync(pd->dev);

This was a small surprise to me. I assume RPM is disabled that late?
But can we be sure the clock is on, then?

> +		if (pd->atomic_xfer) {
> +			unsigned long j = jiffies + pd->adap.timeout;
> +
> +			timeout = 1;
> +			while (!time_after(jiffies, j) &&

To avoid the negation, maybe 'time_before_eq(...)'?

> +			       !(pd->sr & (ICSR_TACK | SW_DONE))) {
> +				unsigned char sr = iic_rd(pd, ICSR);
> +
> +				if (sr & (ICSR_AL   | ICSR_TACK |
> +					  ICSR_WAIT | ICSR_DTE)) {
> +					sh_mobile_i2c_isr(0, pd);
> +					udelay(150);
> +				} else
> +					cpu_relax();

Braces for else block.

> +			}
> +
> +			if (time_after(jiffies, j))
> +				timeout = 0;

Uhh, 'timeout' should have been named 'time_left' back then. Then, this
all would be more readable and we could do here:

	time_left = time_before_eq(...);

and avoid both 'timeout' assignments above.

> +static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
> +			      struct i2c_msg *msgs,
> +			      int num)
> +{
> +	struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter);
> +
> +	pd->atomic_xfer = false;


Maybe move this above to the xfer function ...

> +	return xfer(pd, msgs, num);


... and have here only:

	return __sh_mobile_i2c_xfer(adapter, msgs, num, false);

But yeah, this is bike-shedding. I don't mind much.

>  static const struct i2c_algorithm sh_mobile_i2c_algorithm = {
> -	.functionality	= sh_mobile_i2c_func,
> -	.master_xfer	= sh_mobile_i2c_xfer,
> +	.functionality		= sh_mobile_i2c_func,
> +	.master_xfer		= sh_mobile_i2c_xfer,
> +	.master_xfer_atomic	= sh_mobile_i2c_xfer_atomic,

Just convert to a single space before the '='.

All the best,

   Wolfram


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