| Message ID | 20230921144400.62380-6-dlechner@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: resolver: move ad2s1210 out of staging | expand |
On Thu, 21 Sep 2023 09:43:46 -0500 David Lechner <dlechner@baylibre.com> wrote: > This fixes a number of issues in the ad2s1210_probe() function: > - Move call to ad2s1210_setup_gpios() after `st->sdev = spi;`. This > fixes use of this pointer before it is initialized. > - Check return value on ad2s1210_initial(). Should mention moving it as well. > - Move devm_iio_device_register() to the end to avoid race of > registering before fully initialized. > - Remove call to spi_setup(). Note: MODE_3 was incorrect, it should be > MODE_1 but we can let the device tree select this. > - Change default value for fclkin. This is an external oscillator, not > the SPI bus clock. (Will use device tree to get the correct value > in a future patch. For now, using the eval board value.) > - Remove spi_set_drvdata(). Hmm. This is a lot of different things. I'd prefer it more split up as a few of these are not completely trivial. > > Signed-off-by: David Lechner <dlechner@baylibre.com> I think the patch split up broke on this one... > --- > drivers/staging/iio/resolver/ad2s1210.c | 30 ++++++++++++------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 0bdd5a30d45d..9c7f76114360 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -3,6 +3,7 @@ > * ad2s1210.c support for the ADI Resolver to Digital Converters: AD2S1210 > * > * Copyright (c) 2010-2010 Analog Devices Inc. > + * Copyright (C) 2023 BayLibre, SAS Bit early to justify that, but I'm fine with it anyway as it will soon be justified! > */ > #include <linux/types.h> > #include <linux/mutex.h> > @@ -657,12 +658,8 @@ static int ad2s1210_probe(struct spi_device *spi) > indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > if (!indio_dev) > return -ENOMEM; > - st = iio_priv(indio_dev); > - ret = ad2s1210_setup_gpios(st); > - if (ret < 0) > - return ret; > > - spi_set_drvdata(spi, indio_dev); Looks fine, but unconnected to the rest of this patch. > + st = iio_priv(indio_dev); > > mutex_init(&st->lock); > st->sdev = spi; > @@ -671,22 +668,25 @@ static int ad2s1210_probe(struct spi_device *spi) > st->resolution = 12; > st->fexcit = AD2S1210_DEF_EXCIT; > > + ret = ad2s1210_setup_clocks(st); doesn't exist yet. > + if (ret < 0) > + return ret; > + > + ret = ad2s1210_setup_gpios(st); > + if (ret < 0) > + return ret; > + > + ret = ad2s1210_initial(st); > + if (ret < 0) > + return ret; > + > indio_dev->info = &ad2s1210_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = ad2s1210_channels; > indio_dev->num_channels = ARRAY_SIZE(ad2s1210_channels); > indio_dev->name = spi_get_device_id(spi)->name; > > - ret = devm_iio_device_register(&spi->dev, indio_dev); > - if (ret) > - return ret; > - > - st->fclkin = spi->max_speed_hz; > - spi->mode = SPI_MODE_3; > - spi_setup(spi); > - ad2s1210_initial(st); > - > - return 0; > + return devm_iio_device_register(&spi->dev, indio_dev); > } > > static const struct of_device_id ad2s1210_of_match[] = {
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index 0bdd5a30d45d..9c7f76114360 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -3,6 +3,7 @@ * ad2s1210.c support for the ADI Resolver to Digital Converters: AD2S1210 * * Copyright (c) 2010-2010 Analog Devices Inc. + * Copyright (C) 2023 BayLibre, SAS */ #include <linux/types.h> #include <linux/mutex.h> @@ -657,12 +658,8 @@ static int ad2s1210_probe(struct spi_device *spi) indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); if (!indio_dev) return -ENOMEM; - st = iio_priv(indio_dev); - ret = ad2s1210_setup_gpios(st); - if (ret < 0) - return ret; - spi_set_drvdata(spi, indio_dev); + st = iio_priv(indio_dev); mutex_init(&st->lock); st->sdev = spi; @@ -671,22 +668,25 @@ static int ad2s1210_probe(struct spi_device *spi) st->resolution = 12; st->fexcit = AD2S1210_DEF_EXCIT; + ret = ad2s1210_setup_clocks(st); + if (ret < 0) + return ret; + + ret = ad2s1210_setup_gpios(st); + if (ret < 0) + return ret; + + ret = ad2s1210_initial(st); + if (ret < 0) + return ret; + indio_dev->info = &ad2s1210_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = ad2s1210_channels; indio_dev->num_channels = ARRAY_SIZE(ad2s1210_channels); indio_dev->name = spi_get_device_id(spi)->name; - ret = devm_iio_device_register(&spi->dev, indio_dev); - if (ret) - return ret; - - st->fclkin = spi->max_speed_hz; - spi->mode = SPI_MODE_3; - spi_setup(spi); - ad2s1210_initial(st); - - return 0; + return devm_iio_device_register(&spi->dev, indio_dev); } static const struct of_device_id ad2s1210_of_match[] = {
This fixes a number of issues in the ad2s1210_probe() function: - Move call to ad2s1210_setup_gpios() after `st->sdev = spi;`. This fixes use of this pointer before it is initialized. - Check return value on ad2s1210_initial(). - Move devm_iio_device_register() to the end to avoid race of registering before fully initialized. - Remove call to spi_setup(). Note: MODE_3 was incorrect, it should be MODE_1 but we can let the device tree select this. - Change default value for fclkin. This is an external oscillator, not the SPI bus clock. (Will use device tree to get the correct value in a future patch. For now, using the eval board value.) - Remove spi_set_drvdata(). Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/staging/iio/resolver/ad2s1210.c | 30 ++++++++++++------------- 1 file changed, 15 insertions(+), 15 deletions(-)