Hi, On Thu, Sep 20, 2018 at 11:03 AM Stephen Boyd <swb...@chromium.org> wrote: > > We never really look at the 'ret' local variable in these functions, so > let's remove it to make way for shorter and simpler code. Furthermore, > we can shorten some lines by adding two local variables for the SE and > the message length so that everything fits in 80 columns. And kernel > style is to leave the return statement by itself, detached from the rest > of the function. > > Cc: Karthikeyan Ramasubramanian <krama...@codeaurora.org> > Cc: Sagar Dharia <sdha...@codeaurora.org> > Cc: Girish Mahadevan <giri...@codeaurora.org> > Signed-off-by: Stephen Boyd <swb...@chromium.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 54 ++++++++++++++---------------- > 1 file changed, 25 insertions(+), 29 deletions(-)
I didn't hate the old code, but you're right that this looks a little better. Reviewed-by: Douglas Anderson <diand...@chromium.org> NOTE: now that you're cleaning it up, I'd personally go even further and totally get rid of the "mode" local variable. Basically, just: dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); if (dma_buf) geni_se_select_mode(se, GENI_SE_DMA); else geni_se_select_mode(se, GENI_SE_FIFO); ...elsewhere just continue to use "dma_buf" being NULL to mean FIFO if you have to fall back to the FIFO case you set dma_buf to NULL after calling i2c_put_dma_safe_msg_buf() to indicate to later code that you're in FIFO mode. I prototyped this up and the net-net is one fewer line of code and also the nice consistency that you never hold dma_buf in a pointer after you've freed it. -Doug
