Re: [PATCH v2 6/6] i2c: mpc: Interrupt driven transfer

From: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
To: Wolfram Sang <wsa@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"linux@roeck-us.net" <linux@roeck-us.net>,
	"jdelvare@suse.com" <jdelvare@suse.com>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 6/6] i2c: mpc: Interrupt driven transfer
Date: Sun, 11 Apr 2021 21:31:47 +0000	[thread overview]
Message-ID: <723acaa7-ff57-40c2-a733-a520c5899800@alliedtelesis.co.nz> (raw)
In-Reply-To: <20210410201302.GC2471@kunai>

On 11/04/21 8:13 am, Wolfram Sang wrote:
> On Mon, Mar 29, 2021 at 02:52:06PM +1300, Chris Packham wrote:
>> The fsl-i2c controller will generate an interrupt after every byte
>> transferred. Make use of this interrupt to drive a state machine which
>> allows the next part of a transfer to happen as soon as the interrupt is
>> received. This is particularly helpful with SMBUS devices like the LM81
>> which will timeout if we take too long between bytes in a transfer.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Okay, this change is too large and HW specific for a detailed review.
> But I trust you and hope you will be around to fix regressions if I
> apply it for 5.13?
Yep I plan on being around. I've got access to a couple of designs with 
P2040 and T2081 so hopefully that's sufficient to deal with any 
regressions. One issue is a lack of different i2c devices (the systems 
we have tend to use the same devices) but hopefully any reports of 
regression will be from people with access to such devices.
> That kind of leads to the question if you want to
> step up as the maintainer for this driver?
Sure can do. It'd be nice if it was someone from NXP but I think they've 
lost interest in the PowerPC based SoCs. Should I send a patch for 
MAINTAINERS? If so does that go through the i2c tree?
> Only thing I noticed was a "BUG" and "BUG_ON" and wonder if we really
> need to halt the kernel in that case. Maybe WARN is enough?

Yeah I think they can both be WARN variants. The one in mpc_xfer() can 
happily continue. It's a little less clear what I should do in 
mpc_i2c_do_action() if the WARN is ever hit but in theory it should be 
an unreachable case anyway so the only thing that could get there is 
some kind of memory corruption which would likely cause a crash elsewhere.

Do you want me to send a V3 of just that patch?

> I'll apply the first five patches now, they look good to me.
>