Messages in this thread | | | Date | Sat, 8 Jan 2022 16:36:21 +0000 | From | Sidong Yang <> | Subject | Re: [PATCH v2] staging: pi433: move get version func to where all other functions are |
| |
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@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.
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 >
| |