Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
Am 09.08.2018 um 15:23 schrieb Ben Whitten: >> 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? It uses an MCU implementing a USB CDC interface that implements UART commands corresponding to the individual SPI read and write modes like single and burst. That reference design is for SX1308, but some vendors appear to be using it for SX1301, too. https://github.com/Lora-net/picoGW_mcu So I believe I'll need to implement a regmap_bus in a new serdev driver to support it, once I gain access to such an implementation. But there's other general serdev problems with USB not limited to picoGW that I'll need to start a thread about on linux-serial/linux-usb... Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
> To: Ben Whitten ; Ben > Whitten > Cc: starni...@g.ncu.edu.tw; hasnain.v...@arm.com; > netdev@vger.kernel.org; Xue Liu > ; Sebastian Heß > > 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
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? > 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)
RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
> 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. 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? 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. Regards, Ben Whitten
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? Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add > register, bit-fields, and helpers for regmap > > Am 07.08.2018 um 19:32 schrieb Ben Whitten: > > From: Ben Whitten > > > > The register and bit-field definitions are taken from the > SX1301 > > datasheet version 2.01 dated June 2014 with the revision > information > > 'First released version'. > > > > The reset state and RW capability of each field is not > reflected in this > > patch however from the datasheet: > > "Bits and registers that are not documented are reserved. > They may > > include calibration values. It is important not to modify > these bits and > > registers. If specific bits must be changed in a register with > reserved > > bits, the register must be read first, specific bits modified > while > > masking reserved bits and then the register can be > written." > > > > Then goes on to state: > > "Reserved bits should be written with their reset state, > they may be > > read different states." > > > > Caching is currently disabled. > > > > The version is read back using regmap_read to verify > regmap operation, > > in doing so needs to be moved after priv and regmap > allocation. > > > > Signed-off-by: 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. Thanks, Ben
RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
> register, bit-fields, and helpers for regmap
>
> Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> > diff --git a/drivers/net/lora/sx1301.c
> b/drivers/net/lora/sx1301.c
> > index 5342b61..49958f0 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> [...]
> > @@ -76,8 +287,29 @@ struct sx1301_priv {
> > struct gpio_desc *rst_gpio;
> > u8 cur_page;
> > struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> > + struct regmap *regmap;
> > + struct regmap_field
> *regmap_fields[ARRAY_SIZE(sx1301_reg_fields)];
> > };
> >
> > +static int sx1301_field_read(struct sx1301_priv *priv,
> > + enum sx1301_fields field_id)
> > +{
> > + int ret;
> > + int val;
> > +
> > + ret = regmap_field_read(priv-
> >regmap_fields[field_id], &val);
> > + if (ret)
> > + return ret;
> > +
> > + return val;
>
> This strikes me as a bad idea. Please keep returning the
> value by
> pointer, so that we can clearly distinguish from error values.
Good point, will change it.
> > +}
> > +
> > +static int sx1301_field_write(struct sx1301_priv *priv,
> > + enum sx1301_fields field_id, u8 val)
> > +{
> > + return regmap_field_write(priv-
> >regmap_fields[field_id], val);
> > +}
> > +
> > static int sx1301_read_burst(struct spi_device *spi, u8
> reg, u8 *val, size_t len)
> > {
> > u8 addr = reg & 0x7f;
> [snip]
>
> It looks to me as if both of those static functions are unused
> in this
> patch? Please keep things bisectable.
Sure, it was a prep patch bit I can include these functions in the
next one along where they are actually used.
Thanks,
Ben
Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
Am 07.08.2018 um 19:32 schrieb Ben Whitten:
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 5342b61..49958f0 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
[...]
> @@ -76,8 +287,29 @@ struct sx1301_priv {
> struct gpio_desc *rst_gpio;
> u8 cur_page;
> struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> + struct regmap *regmap;
> + struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_reg_fields)];
> };
>
> +static int sx1301_field_read(struct sx1301_priv *priv,
> + enum sx1301_fields field_id)
> +{
> + int ret;
> + int val;
> +
> + ret = regmap_field_read(priv->regmap_fields[field_id], &val);
> + if (ret)
> + return ret;
> +
> + return val;
This strikes me as a bad idea. Please keep returning the value by
pointer, so that we can clearly distinguish from error values.
> +}
> +
> +static int sx1301_field_write(struct sx1301_priv *priv,
> + enum sx1301_fields field_id, u8 val)
> +{
> + return regmap_field_write(priv->regmap_fields[field_id], val);
> +}
> +
> static int sx1301_read_burst(struct spi_device *spi, u8 reg, u8 *val, size_t
> len)
> {
> u8 addr = reg & 0x7f;
[snip]
It looks to me as if both of those static functions are unused in this
patch? Please keep things bisectable.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Re: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap
Am 07.08.2018 um 19:32 schrieb Ben Whitten: > From: Ben Whitten > > The register and bit-field definitions are taken from the SX1301 > datasheet version 2.01 dated June 2014 with the revision information > 'First released version'. > > The reset state and RW capability of each field is not reflected in this > patch however from the datasheet: > "Bits and registers that are not documented are reserved. They may > include calibration values. It is important not to modify these bits and > registers. If specific bits must be changed in a register with reserved > bits, the register must be read first, specific bits modified while > masking reserved bits and then the register can be written." > > Then goes on to state: > "Reserved bits should be written with their reset state, they may be > read different states." > > Caching is currently disabled. > > The version is read back using regmap_read to verify regmap operation, > in doing so needs to be moved after priv and regmap allocation. > > Signed-off-by: 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? Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
