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. > While at it, the Version Register hardcoded value was replaced with a > pre-existing constant in the driver. This is good, though. > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c > index 68c09fa016ed..1372361d56e1 100644 > --- a/drivers/staging/pi433/pi433_if.c > +++ b/drivers/staging/pi433/pi433_if.c > @@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi) > spi->mode, spi->bits_per_word, spi->max_speed_hz); > > /* Ping the chip by reading the version register */ This comment doesn't make sense now. > - retval = spi_w8r8(spi, 0x10); > - if (retval < 0) > - return retval; > + retval = rf69_get_version(spi); Just say: retval = rf69_read_reg(spi, REG_VERSION); if (retval < 0) return retval; Deleting the error handling was a bad style choice. Also preserve the error code. regards, dan carpenter
