On Sat, Jan 08, 2022 at 02:19:10PM +0300, Dan Carpenter wrote: Hi, Paul. Thanks for applying my opinion. And there is one thing to metion. > On Sat, Jan 08, 2022 at 08:24:38AM +1300, Paulo Miguel Almeida wrote: > > On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote: > > > Just say: > > > > > > retval = rf69_read_reg(spi, REG_VERSION); > > > > atm rf69_read_reg is a static function in rf69.c. > > > > I would remove be the static. > > > I do agree that this is technically possible to write the code > > exactly as you suggested but on the other hand, that would be the only > > exception to the rule when considering all other higher-level functions > > provided in the rf69.c > > There may be other functions which will benefit from this later on. So > instead of "only" a better word is "first". Some of those high level > functions make sense because they are slightly complicated and have > multiple callers. But in this case open coding it seems easier to read. > > > > > are you strongly set on the rf69_read_reg approach or would you > > be open to keep the original approach? (rf69_get_version) > > I think my approach is best but I don't care. > > > > > > if (retval < 0) > > > return retval; > > > > > > Deleting the error handling was a bad style choice. Also preserve the > > > error code. > > > > > > > I just want to double-check if this suggestion is taking into > > consideration what was mentioned here: > > https://lore.kernel.org/lkml/20220106210134.GB3416@xxxxxxxxxxxxxxx/ > > > > If it is, I'm happy to add it back but I just wanted to confirm it > > first. > > Yes. Keep the error handling. Your way makes the code more complicated > to read. I totally really agree with it. Because the switch clause under this patch catches error with 'default:' but it returns '-ENODEV'. I worried that it lost retval from reading register if it has error. > > regards, > dan carpenter >
