From: Mike Looijmans <mike.looijmans@topic.nl>
To: Jonathan Cameron <jic23@kernel.org>
Cc: marius.cristea@microchip.com, lars@metafoo.de,
robh+dt@kernel.org, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC
Date: Tue, 20 Jun 2023 16:34:43 +0200 [thread overview]
Message-ID: <b2c432e2-ea5f-79bc-0429-e89e6f948268@topic.nl> (raw)
In-Reply-To: <20230528202024.0ade4595@jic23-huawei>
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@topic.nl
W: www.topic.nl
prev parent reply other threads:[~2023-06-20 14:35 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-19 16:01 [PATCH 0/2] Adding support for Microchip MCP3564 ADC family marius.cristea
2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea
2023-05-19 18:29 ` Conor Dooley
2023-05-20 15:17 ` Jonathan Cameron
2023-06-08 19:34 ` Rob Herring
2023-06-09 17:41 ` Jonathan Cameron
2023-05-20 13:20 ` Jonathan Cameron
2023-05-24 10:37 ` Mike Looijmans
2023-05-28 19:16 ` Jonathan Cameron
2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea
2023-05-20 15:15 ` Jonathan Cameron
2023-06-22 11:32 ` Marius.Cristea
2023-07-02 10:28 ` Jonathan Cameron
2023-05-24 11:05 ` Mike Looijmans
2023-05-28 19:20 ` Jonathan Cameron
2023-06-20 14:34 ` Mike Looijmans [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b2c432e2-ea5f-79bc-0429-e89e6f948268@topic.nl \
--to=mike.looijmans@topic.nl \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marius.cristea@microchip.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).