Re: [PATCH lora-next v2 3/8] net: lora: sx1301: convert to passing priv data throughout

[Ben Whitten <benwhitten@gmail.com>]

On Thu, 9 Aug 2018 at 21:43, Andreas Färber <afaerber@suse.de> wrote:
>
> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> > Instead of passing around the spi device we instead pass around our
> > driver data directly.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> > ---
> >  drivers/net/lora/sx1301.c | 305 +++++++++++++++++++++++-----------------------
> >  1 file changed, 155 insertions(+), 150 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> > index 3c09f5a..7324001 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> > @@ -73,24 +73,26 @@ struct spi_sx1301 {
> >  };
> >
> >  struct sx1301_priv {
> > +     struct device           *dev;
> > +     struct spi_device       *spi;
>
> Obviously this is not a long-term solution, but as interim step it'll
> have to do.
>
> >       struct lora_priv lora;
> >       struct gpio_desc *rst_gpio;
> >       u8 cur_page;
> >       struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> >  };
> >
> > -static int sx1301_read_burst(struct spi_device *spi, u8 reg, u8 *val, size_t len)
> > +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(spi, &addr, 1, val, len);
> > +     return spi_write_then_read(priv->spi, &addr, 1, val, len);
> >  }
> >
> > -static int sx1301_read(struct spi_device *spi, u8 reg, u8 *val)
> > +static int sx1301_read(struct sx1301_priv *priv, u8 reg, u8 *val)
> >  {
> > -     return sx1301_read_burst(spi, reg, val, 1);
> > +     return sx1301_read_burst(priv, reg, val, 1);
> >  }
> >
> > -static int sx1301_write_burst(struct spi_device *spi, u8 reg, const u8 *val, size_t len)
> > +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] = {
>
> This hunk did not apply for some reason, I've manually re-applied it.
>
> [...]
> > @@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
> >       priv->rst_gpio = rst;
> >       priv->cur_page = 0xff;
> >
> > -     spi_set_drvdata(spi, netdev);
> > +     spi_set_drvdata(spi, priv);
>
> This change seems unnecessary and counter-productive for unregistration.
>
> Otherwise applying.

This is actually pretty critical, as it stands with the two spi masters we use
spi_get_drvdata on the parent device of the controller to recover the priv
struct for regmap.

We may have to include the netdev in the priv data, or do a container_of
dance to recover netdev in unregistration.
That said if we wrap things in devm then really our remove function could
be empty, as we have done with the allocation.

Regards,
Ben