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@xxxxxxxx>
Thanks, Uli! Works fine here. Really nice to finally being able to
reboot now without WARNings.
Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
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:
signature.asc
Description: PGP signature
