linux-kernel - RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: 
 <SJ0PR03MB62246270CC24E70732D0288F91DA2@SJ0PR03MB6224.namprd03.prod.outlook.com>
Date: Mon, 8 Jul 2024 05:17:55 +0000
From: "Tinaco, Mariel" <Mariel.Tinaco@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lars-Peter
 Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Liam Girdwood
	<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
        "Hennerich, Michael"
	<Michael.Hennerich@...log.com>,
        Marcelo Schmitt <marcelo.schmitt1@...il.com>,
        Dimitri Fedrau <dima.fedrau@...il.com>,
        Guenter Roeck <linux@...ck-us.net>
Subject: RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC



> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Saturday, June 29, 2024 2:46 AM
> To: Tinaco, Mariel <Mariel.Tinaco@...log.com>
> Cc: linux-iio@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; Lars-Peter Clausen <lars@...afoo.de>; Rob Herring
> <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; Liam Girdwood <lgirdwood@...il.com>; Mark Brown
> <broonie@...nel.org>; Hennerich, Michael <Michael.Hennerich@...log.com>;
> Marcelo Schmitt <marcelo.schmitt1@...il.com>; Dimitri Fedrau
> <dima.fedrau@...il.com>; Guenter Roeck <linux@...ck-us.net>
> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> [External]
> 
> > > > +};
> > > > +
> > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > +				     const struct iio_chan_spec *chan) {
> > > > +	return 0;
> > >
> > > Why have the stubs in here?
> >
> > Should I move the stubs to a different place in the code or remove
> > them altogether since there is only a single powerdown mode available
> Ah. I'd not really understood what was going on here.  This is fine as is.
> 
> > > AD8460_HVDAC_DATA_WORD_HIGH(index),
> > > > +			    ((val >> 8) & 0xFF));
> > >
> > > bulk write? or do these need to be ordered?
> >
> > For this I used bulk read/write this way.
> >
> > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > 				 int index,
> > 				 int val)
> > {
> > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];
> regmap bulk accesses (when spi anyway) should be provided with DMA safe
> buffers.
> Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
> end of the ad8460_state structure.  Possibly you'll need a lock to protect it - I
> haven't checked.
> >
> > 	regvals[0] = val & 0xFF;
> > 	regvals[1] = (val >> 8) & 0xFF;
> 
> That is an endian conversion so use appropriate endian function to fill it
> efficiently and document clearly what is going on.
> 
> 
> 	put_unaligned_le16()
> 
> >
> > 	return regmap_bulk_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > 				 regvals,
> AD8460_DATA_BYTE_WORD_LENGTH); }
> >
> >
> > > > +}
> 
> > > > +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > > > +	if (IS_ERR(state->regmap))
> > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > > > +				     "Failed to initialize regmap");
> > > > +
> > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> > > > +"tx",
> > > > +
> > > IIO_BUFFER_DIRECTION_OUT);
> > >
> > > Ah. I take back my binding comment. I assume this is mapping some
> > > non standard interface for the parallel data flow?
> >
> > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > which this driver was tested
> 
> Hmm. I'd like to see this brought inline with the other iio backend drivers if
> possible.

Does this mean that we would need to implement an AXI IP core on the
FPGA side to be able to test this?

> 
> Jonathan
>