> To: Ben Whitten <ben.whit...@lairdtech.com>; Ben > Whitten <ben.whit...@gmail.com> > Cc: starni...@g.ncu.edu.tw; hasnain.v...@arm.com; > netdev@vger.kernel.org; Xue Liu > <liuxuenetm...@gmail.com>; Sebastian Heß > <sh...@hessware.de> > Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add > register, bit-fields, and helpers for regmap > > + Xue Liu + Sebastian > > Am 08.08.2018 um 17:52 schrieb Ben Whitten: > >> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: > add > >> register, bit-fields, and helpers for regmap > >> > >> Am 08.08.2018 um 14:32 schrieb Ben Whitten: > >>>>> drivers/net/lora/Kconfig | 1 + > >>>>> drivers/net/lora/sx1301.c | 282 > >>>> > +++++++++++++++++++++++++++++++++++++++++++- > >> -- > >>>>> drivers/net/lora/sx1301.h | 169 > >>>> +++++++++++++++++++++++++++ > >>>>> 3 files changed, 439 insertions(+), 13 deletions(-) > >>>>> create mode 100644 drivers/net/lora/sx1301.h > >>>> > >>>> My main concern about this patch is its sheer size. > >> Normally > >>>> for > >>>> #defines the rule is not to add unused ones. Here I > see > >> for > >>>> example FSK > >>>> RSSI fields that we're surely not using yet. Any chance > to > >>>> strip this > >>>> down some more? > >>> > >>> Sure, I'll strip the reg_fields down to those only > required > >> for > >>> loading firmware and setting clocks that will be used in > the > >>> immediate term. This does clutter the driver a bit > >>> unnecessarily at the moment. > >>> I would like to keep the full register dump in the header > >> file > >>> though as its self-contained and gives a full picture of > the > >> chip. > >> > >> Could you do that in more steps though? I'm thinking, > >> convert only the > >> registers in use to regmap (that'll make it easier to > review), > >> then add > >> more registers, then convert to regmap fields? > > > > I split the conversion function by function but we can > probably go > > further if you think it helpful. > > That split feels wrong... > > > Is it more the addition of the replacement register defines > that you > > would like to correlate with what's being removed, so you > can see > > in one patch that this register has the same page and the > same > > address in the new system? > > I am looking for patches that are trivial to review. One > aspect only. > So I would much rather get a large patch with a consistent > series of > > -my_read > +your_read > > -my_write > +your_write > > that's quick to review than lots of different refactorings > mixed into > one patch, grouped by their file location. > > So I am suggesting that if, for example, you want to pass fw > to helpers, > which looks like a great idea, that should be a small patch of > its own, > at the very beginning of your series. (git rebase -i is your > friend.) > > Converting to regmap_read/write I imagine to be a trivial > search-and-replace type refactoring, leaving my |= and &= > operations in > place, to minimize the diff and avoid it mis-detecting the > patch context. > Without caching I'd expect regmap to work up to here - if it > doesn't, > then we can't apply it to my tree and need to prioritize other > cleanups > while we review/debug further. > > Once all registers use regmap successfully, we can optimize > that code by > introducing regmap fields. This could be split by location, if > desired. > > Finally in the end you can introduce more registers and > fields for > future use. > > Does that make sense now?
Yep no problem, hopefully my V2 is suitable. I stopped short of including all registers and doing any regmap_field work in this series so that we can progress. I have the sx125x split and conversion to regmap_bus for the concentrator ready to go in the wings. > > The problem I face is that these conversions are almost > blind as > > when I run on my hardware I either oops with the > sx1301_read > > or the cal firmware doesn't verify so I can't finish probe. I > only > > get a full sx1301 module probe pass on physical hardware > when > > I get right to the end of the series where it's all replaced > with > > regmap. > > If patches don't build or don't work then I can't apply them. > Otherwise > the 0-day bots will spam us with error reports, as you've > seen before. Understood, each patch in v2 has been tested and I was able to probe and remove modules. > BTW we'll need this regmap conversion for the picoGW_hal, > so once we > have a working SPI regmap driver, we'll need to split out the > SPI bits, > similar to sx125x. I am unfamiliar with the picoGW_hal, do they expose the sx1301 as a device on a regmap_bus then? Regards, Ben Whitten
