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

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: <20230528202024.0ade4595@jic23-huawei>
Date:   Sun, 28 May 2023 20:20:24 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Mike Looijmans <mike.looijmans@...ic.nl>
Cc:     marius.cristea@...rochip.com, lars@...afoo.de, robh+dt@...nel.org,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC


> 
> > +
> > +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 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.

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

Jonathan