Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

[Dan Carpenter <dan.carpenter@oracle.com>]

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@mail.google.com/ 
> 
> 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.

regards,
dan carpenter