Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC

Mike Looijmans <mike.looijmans@xxxxxxxx> · Tue, 20 Jun 2023 16:34:43 +0200

On 28-05-2023 21:20, Jonathan Cameron wrote:
+
+static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd)
+{
+	return ((chip_addr << 6) | (cmd << 2));
+}
+
+static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len)
+{
+	int ret;
+	u8 tmp_reg;
+
+	tmp_reg = mcp356x_reg_read(adc->dev_addr, reg);
+
+	ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len);
I had issues with spi_write_then_read not communicating properly with the
device. This may be due to the SPI controller (Xilinx IP in FPGA) though.
That's a bit worrying!  Shouldn't be a problem unless there is something
fiddly required with the chip select or similar.

I suspect it's the Xilinx SPI controller IP that's misbehaving, we saw 
some strange behavior of the CS line while probing with a scope. Judging 
from the scope images I suspect it tri-states the CS line between the 
transfers, instead of always driving it low or high.

I had to use spi_sync() to get reliable answers. Also has the added benefit of
giving access to the interrupt flags.
+
+#define MCP3564_CHANNELS(depth) {			\
+	MCP356X_V_CHANNEL(0, 0, depth),			\
+	MCP356X_V_CHANNEL(1, 1, depth),			\
+	MCP356X_V_CHANNEL(2, 2, depth),			\
+	MCP356X_V_CHANNEL(3, 3, depth),			\
+	MCP356X_V_CHANNEL(4, 4, depth),			\
+	MCP356X_V_CHANNEL(5, 5, depth),			\
+	MCP356X_V_CHANNEL(6, 6, depth),			\
+	MCP356X_V_CHANNEL(7, 7, depth),			\
+	MCP356X_T_CHAN(depth),				\
+	MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth),	\
+	MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth),	\
+	MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth),	\
+	MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth),	\
+	MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth),	\
+	MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth),	\
+	MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth),	\
+	MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth),	\
+	MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth),	\
+	MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth),	\
+	MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth),	\
+	MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth),	\
+	MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth),	\
+	MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth)	\
+}
Nice that the chip can do full 8x8 mux delivering 56 channels, but I don't
think that will be useful to many people.

Also I'd want to see buffer support added to the device, which only works for
the channels as in the table in the datasheet, so only 4 differential ones.
It'd be annoying to have 56 channels but only be able to contiuously read 4
specific ones.
One common solution to this is to push the channel setup to firmware
as it corresponds to 'what is wired'.  See the channel bindings
in Documentation/devicetree/bindings/iio/adc/adc.yaml
and various examples in tree.

Makes sense. Also would allow you to hide inputs that aren't connected 
anyway.

Thanks for combining efforts btw - should give us the best overall
result but I feel for you when you discovered the duplication :(

What worries me more is that Marius Cristea isn't responding... maybe 
he's on a holiday? I'd be happy to join forces on this...

Jonathan

--
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@xxxxxxxx
W: www.topic.nl