Re: [PATCH] i2c-axxia: support slave mode

[Wolfram Sang]

Hi Krzysztof,

> This device contains both master and slave controllers which can be
> enabled simultaneously. Both controllers share the same SDA/SCL lines
> and interrupt source but has separate control and status registers.
> Controllers also works in loopback mode - slave device can communicate
> with its own master controller internally. The controller can handle up

Cool, I never got this to work with my hardware. I always had to wire
two controllers together,

> to two addresses, both of which may be 10 bit. Most of the logic
> (sending (N)ACK, handling repeated start or switching between
> write/read) is handled automatically which makes working with this
> controller quite easy.

Yes, looks pretty straightforward. Nice!

> For simplicity, this patch adds basic support, limiting to only one
> slave address. Support for the 2nd device may be added in the future.

Fine with me. Incremental additions are easier to review.

> Note that checkpatch shows warnings about "line over 80 characters" for
> some of those register definitions added but I personally think
> splitting those comments would decrease readability, not increase it. I
> can do that, however, if you think otherwise.

I am fine with that, too.

> +	if (fifo_status & SLV_FIFO_DV1) {
> +		if (fifo_status & SLV_FIFO_STRC) {
> +			dev_dbg(dev, "First data byte sent\n");

I think, however, these debug messages could go. They were surely
helpful during development but assuming things work now, they will not
help backend authors. Can you agree?

Rest looks good from what I can tell without knowing the hardware.

Thanks,

   Wolfram

Attachment: signature.asc
Description: PGP signature