| Message ID | 20250108-wip-bl-ad3552r-axi-v0-iio-testing-carlos-v2-6-2dac02f04638@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: ad3552r-hs: add support for ad3541/42r | expand |
On 1/8/25 11:29 AM, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Use "instruction" mode over initial configuration and all other > non-streaming operations. > > DAC boots in streaming mode as default, and the driver is not > changing this mode. > > Instruction r/w is still working becouse instruction is processed s/becouse/because/ > from the DAC after chip select is deasserted, this works until > loop mode is 0 or greater than the instruction size. > > All initial operations should be more safely done in instruction > mode, a mode provided for this. I'm not sure it is really "safer". The way I read the datasheet, this just enables bulk reads of multiple registers. So unless we need to do bulk reads it seems like this is just adding extra complexity to the driver without a tangible benefit. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > drivers/iio/dac/ad3552r-hs.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > index 27949f207d42..991b11702273 100644 > --- a/drivers/iio/dac/ad3552r-hs.c > +++ b/drivers/iio/dac/ad3552r-hs.c > @@ -132,6 +132,13 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > return -EINVAL; > } > > + /* Primary region access, set streaming mode (now in SPI + SDR). */ > + ret = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_B, > + AD3552R_MASK_SINGLE_INST, 0, 1); Missing undoing this operation in the error path if a later operation in this function fails? > + if (ret) > + return ret; > + > ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > loop_len, 1); > if (ret)
Hi, On 08.01.2025 15:16, David Lechner wrote: > On 1/8/25 11:29 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Use "instruction" mode over initial configuration and all other > > non-streaming operations. > > > > DAC boots in streaming mode as default, and the driver is not > > changing this mode. > > > > Instruction r/w is still working becouse instruction is processed > > s/becouse/because/ > > > from the DAC after chip select is deasserted, this works until > > loop mode is 0 or greater than the instruction size. > > > > All initial operations should be more safely done in instruction > > mode, a mode provided for this. > > I'm not sure it is really "safer". The way I read the datasheet, this just > enables bulk reads of multiple registers. So unless we need to do bulk reads > it seems like this is just adding extra complexity to the driver without a > tangible benefit. > this change was initially requested from the HDL team to stay aligned with the HDL side streaming/non streaming mode, and later, applied since seems a safer way to operate. I see that for the driver and DAC point of view, this is "functionally" not needed, but streaming mode for configuration or raw read/write works until STREAM_MODE register is set to 0 or to a value >= to the register size to be accessed, since the _CS deasserting completes the transaction. I.e., a transaction with a STREAM_MODE set to 1 would fail in case of a 16bit r/w access. Staying into streaming mode seems to me a bit tricky, and setting to instruction mode may be safer since in this mode the STREAM_MODE value is not important anymore. For me is the same, i can remove related code. As you prefer. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > --- > > drivers/iio/dac/ad3552r-hs.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > > index 27949f207d42..991b11702273 100644 > > --- a/drivers/iio/dac/ad3552r-hs.c > > +++ b/drivers/iio/dac/ad3552r-hs.c > > @@ -132,6 +132,13 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > > return -EINVAL; > > } > > > > + /* Primary region access, set streaming mode (now in SPI + SDR). */ > > + ret = ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_B, > > + AD3552R_MASK_SINGLE_INST, 0, 1); > > Missing undoing this operation in the error path if a later operation in this > function fails? > > > + if (ret) > > + return ret; > > + > > ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > > loop_len, 1); > > if (ret) >
On 1/9/25 11:25 AM, Angelo Dureghello wrote: > Hi, > > On 08.01.2025 15:16, David Lechner wrote: >> On 1/8/25 11:29 AM, Angelo Dureghello wrote: >>> From: Angelo Dureghello <adureghello@baylibre.com> >>> >>> Use "instruction" mode over initial configuration and all other >>> non-streaming operations. >>> >>> DAC boots in streaming mode as default, and the driver is not >>> changing this mode. >>> >>> Instruction r/w is still working becouse instruction is processed >> >> s/becouse/because/ >> >>> from the DAC after chip select is deasserted, this works until >>> loop mode is 0 or greater than the instruction size. >>> >>> All initial operations should be more safely done in instruction >>> mode, a mode provided for this. >> >> I'm not sure it is really "safer". The way I read the datasheet, this just >> enables bulk reads of multiple registers. So unless we need to do bulk reads >> it seems like this is just adding extra complexity to the driver without a >> tangible benefit. >> > > this change was initially requested from the HDL team to stay aligned with > the HDL side streaming/non streaming mode, and later, applied since seems > a safer way to operate. > > I see that for the driver and DAC point of view, this is "functionally" > not needed, but streaming mode for configuration or raw read/write works > until STREAM_MODE register is set to 0 or to a value >= to the register > size to be accessed, since the _CS deasserting completes the transaction. > > I.e., a transaction with a STREAM_MODE set to 1 would fail in case of > a 16bit r/w access. Staying into streaming mode seems to me a bit tricky, > and setting to instruction mode may be safer since in this mode the > STREAM_MODE value is not important anymore. > > For me is the same, i can remove related code. As you prefer. > If it really does make a difference, I'm OK to keep it.
diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c index 27949f207d42..991b11702273 100644 --- a/drivers/iio/dac/ad3552r-hs.c +++ b/drivers/iio/dac/ad3552r-hs.c @@ -132,6 +132,13 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) return -EINVAL; } + /* Primary region access, set streaming mode (now in SPI + SDR). */ + ret = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_B, + AD3552R_MASK_SINGLE_INST, 0, 1); + if (ret) + return ret; + ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, loop_len, 1); if (ret) @@ -198,6 +205,14 @@ static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) if (ret) return ret; + /* Back to single instruction mode, disabling loop. */ + ret = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_B, + AD3552R_MASK_SINGLE_INST, + AD3552R_MASK_SINGLE_INST, 1); + if (ret) + return ret; + return 0; } @@ -308,6 +323,13 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) if (ret) return ret; + ret = st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_B, + AD3552R_MASK_SINGLE_INST | + AD3552R_MASK_SHORT_INSTRUCTION, 1); + if (ret) + return ret; + ret = ad3552r_hs_scratch_pad_test(st); if (ret) return ret;