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

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: <20190806205124.GG911@ninjato>
Date:   Tue, 6 Aug 2019 22:51:24 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     "Adamski, Krzysztof (Nokia - PL/Wroclaw)" 
        <krzysztof.adamski@...ia.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@...ia.com>
Subject: Re: [PATCH] i2c-axxia: support slave mode

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


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)