Hi, On 08.01.2025 15:16, David Lechner wrote: > On 1/8/25 11:29 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxx> > > --- > > 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) >
