Re: [PATCH 5/5] i2c-i801: Remove redundant code and event-drive

[Corey Minyard <minyard@acm.org>]

On 05/25/2016 07:52 AM, Jean Delvare wrote:
> Hi Corey,
>
> On Tue, 19 Jan 2016 11:09:37 -0600, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> There was a lot of redundant code in the driver, basically one copy
>> for the polled interface and one for the interrupt-driven interface.
>> Only use the interrupt-driven interface and use a timer to check
>> the interface periodically and time things out.
> This patch is simply too large and messy for me to consider reviewing
> it. It needs to be split into smaller pieces. But honestly I don't see
> that much redundancy in the driver, and as a matter of fact your patch
> adds more lines than it removes.

Umm:

  drivers/i2c/busses/i2c-i801.c | 582 +++++++++++++++++++++---------------------
  1 file changed, 284 insertions(+), 298 deletions(-)

It's not a lot of lines, but it is a little smaller.

>
> This is unfortunate because some of the changes would still be good to
> have. In particular having a single call to i801_check_post() would be
> a nice clean-up.

Yes, looking over this some more, several of the issues have been
addressed that these patches originally targeted.

This was originally part of a set of patches that allowed access to
I2C devices when the system couldn't schedule, primarily for
storing panic information and handling watchdogs at panic and
in kgdb.  For that you have to event-drive it so you can poll.
I've abandoned those changes, but this small set looked
like it has some useful stuff.

Thanks for the review, I'll look at pulling out some of the good things
(and getting rid of hwpec from i801_block_transaction_byte_by_byte
since it's not used there any more).

-corey

>> This also adds timeouts when using interrupt, so a failed operation
>> won't hang forever.
> As far as I can see, the current code already has these timeouts,
> although maybe it wasn't the case when you first wrote this patch? This
> was added by b3b8df9772 in November 2014.
>