+ 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? > 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. 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. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
