Re: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

Re: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Andreas Färber]

Hi Ben and Mark,

Am 09.10.18 um 14:52 schrieb Ben Whitten:
> As we have caching disabled we can access the regmap using raw for our
> firmware reading and writing bursts.
> We also remove the now defunct spi element from the structure as this
> completes the move to regmap.
> 
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
>  drivers/net/lora/sx1301.c | 26 +++++++++++++++++---------
>  drivers/net/lora/sx1301.h |  2 --
>  2 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index fd29258..5ab0e2d 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -76,19 +76,28 @@ static struct regmap_config sx1301_regmap_config = {
>  
>  static int sx1301_read_burst(struct sx1301_priv *priv, u8 reg, u8 *val, size_t len)
>  {
> -	u8 addr = reg & 0x7f;
> -	return spi_write_then_read(priv->spi, &addr, 1, val, len);
> +	size_t max;
> +
> +	max = regmap_get_raw_read_max(priv->regmap);
> +	if (max && max < len) {
> +		dev_err(priv->dev, "Burst greater then max raw read\n");
> +		return -EINVAL;
> +	}
> +
> +	return regmap_raw_read(priv->regmap, reg, val, len);

I believe the way we are using it with a single register needs
regmap_noinc_read() instead, in case we ever want to enable caching? ...

>  }
>  
>  static int sx1301_write_burst(struct sx1301_priv *priv, u8 reg, const u8 *val, size_t len)
>  {
> -	u8 addr = reg | BIT(7);
> -	struct spi_transfer xfr[2] = {
> -		{ .tx_buf = &addr, .len = 1 },
> -		{ .tx_buf = val, .len = len },
> -	};
> +	size_t max;
> +
> +	max = regmap_get_raw_write_max(priv->regmap);
> +	if (max && max < len) {
> +		dev_err(priv->dev, "Burst greater then max raw write\n");
> +		return -EINVAL;
> +	}
>  
> -	return spi_sync_transfer(priv->spi, xfr, 2);
> +	return regmap_raw_write(priv->regmap, reg, val, len);

... Which would mean we are lacking a noinc API for write here!

@Mark/Stefan, any reason noinc was not implemented symmetrically?

For sx1276 I have local regmap conversion patches, but I kept getting
-EINVAL for bursts and haven't figured out why yet. Need to compare to
your code - I assume you successfully tested this, Ben.

Regards,
Andreas

>  }
>  
>  static int sx1301_soft_reset(struct sx1301_priv *priv)
> @@ -566,7 +575,6 @@ static int sx1301_probe(struct spi_device *spi)
>  
>  	spi_set_drvdata(spi, netdev);
>  	priv->dev = &spi->dev;
> -	priv->spi = spi;
>  
>  	priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
>  	if (IS_ERR(priv->regmap)) {
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> index e939c02..e6400f8 100644
> --- a/drivers/net/lora/sx1301.h
> +++ b/drivers/net/lora/sx1301.h
> @@ -12,7 +12,6 @@
>  #include <linux/regmap.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/lora/dev.h>
> -#include <linux/spi/spi.h>
>  
>  #define SX1301_CHIP_VERSION 103
>  
> @@ -64,7 +63,6 @@
>  struct sx1301_priv {
>  	struct lora_dev_priv lora;
>  	struct device		*dev;
> -	struct spi_device	*spi;
>  	struct gpio_desc *rst_gpio;
>  	struct regmap		*regmap;
>  };
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Ben Whitten]

> Subject: Re: [PATCH v2 lora-next 1/4] net: lora: sx1301:
> convert burst spi functions to regmap raw
> 
> Hi Ben and Mark,
> 
> Am 09.10.18 um 14:52 schrieb Ben Whitten:
> > As we have caching disabled we can access the regmap
> using raw for our
> > firmware reading and writing bursts.
> > We also remove the now defunct spi element from the
> structure as this
> > completes the move to regmap.
> >
> > Signed-off-by: Ben Whitten
> <ben.whitten@lairdtech.com>
> > ---
> >  drivers/net/lora/sx1301.c | 26 +++++++++++++++++------
> ---
> >  drivers/net/lora/sx1301.h |  2 --
> >  2 files changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c
> b/drivers/net/lora/sx1301.c
> > index fd29258..5ab0e2d 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> > @@ -76,19 +76,28 @@ static struct regmap_config
> sx1301_regmap_config = {
> >
> >  static int sx1301_read_burst(struct sx1301_priv *priv, u8
> reg, u8 *val, size_t len)
> >  {
> > -	u8 addr = reg & 0x7f;
> > -	return spi_write_then_read(priv->spi, &addr, 1, val,
> len);
> > +	size_t max;
> > +
> > +	max = regmap_get_raw_read_max(priv->regmap);
> > +	if (max && max < len) {
> > +		dev_err(priv->dev, "Burst greater then max
> raw read\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return regmap_raw_read(priv->regmap, reg, val,
> len);
> 
> I believe the way we are using it with a single register needs
> regmap_noinc_read() instead, in case we ever want to
> enable caching? ...

Certainly, the noinc was introduced in 4.19 lets rebase lora-next
and use this API instead.

> 
> >  }
> >
> >  static int sx1301_write_burst(struct sx1301_priv *priv, u8
> reg, const u8 *val, size_t len)
> >  {
> > -	u8 addr = reg | BIT(7);
> > -	struct spi_transfer xfr[2] = {
> > -		{ .tx_buf = &addr, .len = 1 },
> > -		{ .tx_buf = val, .len = len },
> > -	};
> > +	size_t max;
> > +
> > +	max = regmap_get_raw_write_max(priv->regmap);
> > +	if (max && max < len) {
> > +		dev_err(priv->dev, "Burst greater then max
> raw write\n");
> > +		return -EINVAL;
> > +	}
> >
> > -	return spi_sync_transfer(priv->spi, xfr, 2);
> > +	return regmap_raw_write(priv->regmap, reg, val,
> len);
> 
> ... Which would mean we are lacking a noinc API for write
> here!
> 
> @Mark/Stefan, any reason noinc was not implemented
> symmetrically?
> 
> For sx1276 I have local regmap conversion patches, but I
> kept getting
> -EINVAL for bursts and haven't figured out why yet. Need to
> compare to
> your code - I assume you successfully tested this, Ben.

Yes I was able to load and verify/run the firmware using this
patch on my Sentrius RG1xx board.

Thanks,
Ben

Re: [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Wed, Oct 10, 2018 at 09:59:29AM +0200, Andreas Färber wrote:
> Am 09.10.18 um 14:52 schrieb Ben Whitten:

> > -	return spi_sync_transfer(priv->spi, xfr, 2);
> > +	return regmap_raw_write(priv->regmap, reg, val, len);

> ... Which would mean we are lacking a noinc API for write here!

> @Mark/Stefan, any reason noinc was not implemented symmetrically?

Nobody wanted it enough to do the work of implementing it, there's
nothing stopping someone doing that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]