| Message ID | 20220318162722.51215-6-alexandru.tachici@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad_sigma_delta: Add sequencer support | expand |
On Fri, 18 Mar 2022 18:27:19 +0200 <alexandru.tachici@analog.com> wrote: > From: Alexandru Tachici <alexandru.tachici@analog.com> > > Add sequencer support for AD7124. > > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> Hi Alexandru, A few comments inline. Thanks, Jonathan > --- > drivers/iio/adc/ad7124.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > index 428ec3e257d7..782b7cdd8ebe 100644 > --- a/drivers/iio/adc/ad7124.c > +++ b/drivers/iio/adc/ad7124.c > @@ -43,6 +43,8 @@ > #define AD7124_STATUS_POR_FLAG_MSK BIT(4) > > /* AD7124_ADC_CONTROL */ > +#define AD7124_ADC_STATUS_EN_MSK BIT(10) > +#define AD7124_ADC_STATUS_EN(x) FIELD_PREP(AD7124_ADC_STATUS_EN_MSK, x) > #define AD7124_ADC_CTRL_REF_EN_MSK BIT(8) > #define AD7124_ADC_CTRL_REF_EN(x) FIELD_PREP(AD7124_ADC_CTRL_REF_EN_MSK, x) > #define AD7124_ADC_CTRL_PWR_MSK GENMASK(7, 6) > @@ -512,14 +514,27 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel) > return ret; > } > > +static int ad7124_append_status(struct ad_sigma_delta *sd, bool append) > +{ > + struct ad7124_state *st = container_of(sd, struct ad7124_state, sd); > + > + st->adc_control &= ~AD7124_ADC_STATUS_EN_MSK; > + st->adc_control |= AD7124_ADC_STATUS_EN(append); Generally avoid updating cached state until you know the write succeeded. So I would operate on a local variable and if ad_sd_write_reg() succeeds copy that back into st->adc_control. Obviously on error it 'might' have been updated successfully and something failed after the write, but it is probably more likely the write failed. > + > + return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control); > +} > + > static const struct ad_sigma_delta_info ad7124_sigma_delta_info = { > .set_channel = ad7124_set_channel, > + .append_status = ad7124_append_status, > .set_mode = ad7124_set_mode, > .has_registers = true, > .addr_shift = 0, > .read_mask = BIT(6), > + .status_ch_mask = GENMASK(3, 0), > .data_reg = AD7124_DATA, > - .irq_flags = IRQF_TRIGGER_FALLING > + .num_slots = 8, > + .irq_flags = IRQF_TRIGGER_FALLING, > }; > > static int ad7124_read_raw(struct iio_dev *indio_dev, > @@ -679,10 +694,11 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev, > > for (i = 0; i < st->num_channels; i++) { > bit_set = test_bit(i, scan_mask); > - ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), > - AD7124_CHANNEL_EN_MSK, > - AD7124_CHANNEL_EN(bit_set), > - 2); > + if (bit_set) > + ret = ad7124_set_channel(&st->sd, i); This is going to repeatedly take an release the cfg mutex. Perhaps it's worth introducing a __ad7124_set_channel() that doesn't take the lock then take it around this loop instead? > + else > + ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK, > + 0, 2); > if (ret < 0) > return ret; > } > @@ -906,12 +922,14 @@ static int ad7124_probe(struct spi_device *spi) > > st->chip_info = info; > > - ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info); > - > indio_dev->name = st->chip_info->name; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &ad7124_info; > > + ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info); > + if (ret < 0) > + return ret; > + > ret = ad7124_of_parse_channel_config(indio_dev, spi->dev.of_node); > if (ret < 0) > return ret;
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index 428ec3e257d7..782b7cdd8ebe 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -43,6 +43,8 @@ #define AD7124_STATUS_POR_FLAG_MSK BIT(4) /* AD7124_ADC_CONTROL */ +#define AD7124_ADC_STATUS_EN_MSK BIT(10) +#define AD7124_ADC_STATUS_EN(x) FIELD_PREP(AD7124_ADC_STATUS_EN_MSK, x) #define AD7124_ADC_CTRL_REF_EN_MSK BIT(8) #define AD7124_ADC_CTRL_REF_EN(x) FIELD_PREP(AD7124_ADC_CTRL_REF_EN_MSK, x) #define AD7124_ADC_CTRL_PWR_MSK GENMASK(7, 6) @@ -512,14 +514,27 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel) return ret; } +static int ad7124_append_status(struct ad_sigma_delta *sd, bool append) +{ + struct ad7124_state *st = container_of(sd, struct ad7124_state, sd); + + st->adc_control &= ~AD7124_ADC_STATUS_EN_MSK; + st->adc_control |= AD7124_ADC_STATUS_EN(append); + + return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control); +} + static const struct ad_sigma_delta_info ad7124_sigma_delta_info = { .set_channel = ad7124_set_channel, + .append_status = ad7124_append_status, .set_mode = ad7124_set_mode, .has_registers = true, .addr_shift = 0, .read_mask = BIT(6), + .status_ch_mask = GENMASK(3, 0), .data_reg = AD7124_DATA, - .irq_flags = IRQF_TRIGGER_FALLING + .num_slots = 8, + .irq_flags = IRQF_TRIGGER_FALLING, }; static int ad7124_read_raw(struct iio_dev *indio_dev, @@ -679,10 +694,11 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev, for (i = 0; i < st->num_channels; i++) { bit_set = test_bit(i, scan_mask); - ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), - AD7124_CHANNEL_EN_MSK, - AD7124_CHANNEL_EN(bit_set), - 2); + if (bit_set) + ret = ad7124_set_channel(&st->sd, i); + else + ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK, + 0, 2); if (ret < 0) return ret; } @@ -906,12 +922,14 @@ static int ad7124_probe(struct spi_device *spi) st->chip_info = info; - ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info); - indio_dev->name = st->chip_info->name; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &ad7124_info; + ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info); + if (ret < 0) + return ret; + ret = ad7124_of_parse_channel_config(indio_dev, spi->dev.of_node); if (ret < 0) return ret;