On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote: > On Fri, Jan 07, 2022 at 10:33:25AM +1300, Paulo Miguel Almeida wrote: > > As a convention for the pi433 driver, all routines that deals with the > > rf69 chip are defined in the rf69.c file. > > That's some EnterpriseQuality[tm] style guidelines. It's an over fussy > rule that just makes the code harder to read for no reason. EnterpriseQuality[tm] was witty :) > > > > /* Ping the chip by reading the version register */ > > This comment doesn't make sense now. you are right, I will change this. > > - retval = spi_w8r8(spi, 0x10); > > - if (retval < 0) > > - return retval; > > + retval = rf69_get_version(spi); > > Just say: > > retval = rf69_read_reg(spi, REG_VERSION); atm rf69_read_reg is a static function in rf69.c. 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 are you strongly set on the rf69_read_reg approach or would you be open to keep the original approach? (rf69_get_version) > 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. thanks, Paulo A.
