i2c-mpc.c driver issues

i2c-mpc.c driver issues

[Tjernlund]

While browsing the i2c-mpc.c driver I noticed some things that look odd
to me so I figured I report them. Could not find a maintainer in the MAINTANERS file
so I sent here, cc:ed linuxppc-dev as well.

1) There are a lot of return -1 error code that is propagated back to
   userspace. Should be changed to proper -Exxx codes.

2) mpc_read(), according to the comment below it sends a STOP condition here but
   this function does not known if this is the last read or not. mpc_xfer is
   the one that knows when the transaction is over and should send the stop, which it already
   does.

 /* Generate stop on last byte */
  if (i == length - 1)
       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);

 Jocke

Re: i2c-mpc.c driver issues

[Jon Smirl]

On 10/24/07, Tjernlund <tjernlund@tjernlund.se> wrote:
> While browsing the i2c-mpc.c driver I noticed some things that look odd
> to me so I figured I report them. Could not find a maintainer in the MAINTANERS file
> so I sent here, cc:ed linuxppc-dev as well.

There appear to be more issues with this driver. It is still
registering as platform driver instead of a of_platform driver.

On the mpc5200 the probe function for platform drivers is not getting
called, so fsl_i2c_probe never gets called.  It's not clear to me that
this driver is functioning on the mpc5200.

> 1) There are a lot of return -1 error code that is propagated back to
>    userspace. Should be changed to proper -Exxx codes.
>
> 2) mpc_read(), according to the comment below it sends a STOP condition here but
>    this function does not known if this is the last read or not. mpc_xfer is
>    the one that knows when the transaction is over and should send the stop, which it already
>    does.
>
>  /* Generate stop on last byte */
>   if (i == length - 1)
>        writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
>
>  Jocke
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] i2c-mpc.c driver issues

[Jean Delvare]

Hi Jocke,

On Wed, 24 Oct 2007 23:06:13 +0200, Tjernlund wrote:
> While browsing the i2c-mpc.c driver I noticed some things that look odd
> to me so I figured I report them. Could not find a maintainer in the MAINTANERS file
> so I sent here, cc:ed linuxppc-dev as well.
> 
> 1) There are a lot of return -1 error code that is propagated back to
>    userspace. Should be changed to proper -Exxx codes.

This is true of many Linux i2c bus drivers, unfortunately. While nothing
actually prevents drivers from returning -1 to userspace on error,
meaningful error codes would of course be preferred.

> 2) mpc_read(), according to the comment below it sends a STOP condition here but
>    this function does not known if this is the last read or not. mpc_xfer is
>    the one that knows when the transaction is over and should send the stop, which it already
>    does.
> 
>  /* Generate stop on last byte */
>   if (i == length - 1)
>        writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);

Probably correct, although I am not familiar with this specific
hardware. I guess that the same is true of mpc_write as well, which is
even worse because write + read combined transactions are very common
(while read + write are not.)

I'm not completely sure that mpc_xfer sends the stop. mpc_i2c_stop
doesn't seem to do much.

Now that you've identified these bugs, what about sending patches
to fix them?

-- 
Jean Delvare

Re: [i2c] i2c-mpc.c driver issues

[Joakim Tjernlund]

On Fri, 2007-10-26 at 11:53 +0200, Jean Delvare wrote:
> Hi Jocke,
> 
> On Wed, 24 Oct 2007 23:06:13 +0200, Tjernlund wrote:
> > While browsing the i2c-mpc.c driver I noticed some things that look odd
> > to me so I figured I report them. Could not find a maintainer in the MAINTANERS file
> > so I sent here, cc:ed linuxppc-dev as well.
> > 
> > 1) There are a lot of return -1 error code that is propagated back to
> >    userspace. Should be changed to proper -Exxx codes.
> 
> This is true of many Linux i2c bus drivers, unfortunately. While nothing
> actually prevents drivers from returning -1 to userspace on error,
> meaningful error codes would of course be preferred.
> 
> > 2) mpc_read(), according to the comment below it sends a STOP condition here but
> >    this function does not known if this is the last read or not. mpc_xfer is
> >    the one that knows when the transaction is over and should send the stop, which it already
> >    does.
> > 
> >  /* Generate stop on last byte */
> >   if (i == length - 1)
> >        writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
> 
> Probably correct, although I am not familiar with this specific
> hardware. I guess that the same is true of mpc_write as well, which is
> even worse because write + read combined transactions are very common
> (while read + write are not.)

Don't think write is a problem, only read. I would have to look at the
HW spec to make sure though.

> 
> I'm not completely sure that mpc_xfer sends the stop. mpc_i2c_stop
> doesn't seem to do much.
> 
> Now that you've identified these bugs, what about sending patches
> to fix them?

Normally I would do that, but I am too busy with other things. Thats
why I only reported this as I know I won't have time to fix it for some
time.

 Jocke

RE: [i2c] i2c-mpc.c driver issues

[Joakim Tjernlund]

> -----Original Message-----
> From: 
> linuxppc-dev-bounces+joakim.tjernlund=transmode.se@ozlabs.org 
> [mailto:linuxppc-dev-bounces+joakim.tjernlund=transmode.se@ozl
abs.org] On Behalf Of Jean Delvare
> Sent: den 26 oktober 2007 11:53
> To: Tjernlund
> Cc: linuxppc-dev@ozlabs.org; i2c@lm-sensors.org
> Subject: Re: [i2c] i2c-mpc.c driver issues
> 
> Hi Jocke,
> 
> On Wed, 24 Oct 2007 23:06:13 +0200, Tjernlund wrote:
> > While browsing the i2c-mpc.c driver I noticed some things 
> that look odd
> > to me so I figured I report them. Could not find a 
> maintainer in the MAINTANERS file
> > so I sent here, cc:ed linuxppc-dev as well.
> > 
> > 1) There are a lot of return -1 error code that is 
> propagated back to
> >    userspace. Should be changed to proper -Exxx codes.
> 
> This is true of many Linux i2c bus drivers, unfortunately. 
> While nothing
> actually prevents drivers from returning -1 to userspace on error,
> meaningful error codes would of course be preferred.
> 
> > 2) mpc_read(), according to the comment below it sends a 
> STOP condition here but
> >    this function does not known if this is the last read or 
> not. mpc_xfer is
> >    the one that knows when the transaction is over and 
> should send the stop, which it already
> >    does.
> > 
> >  /* Generate stop on last byte */
> >   if (i == length - 1)
> >        writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
> 
> Probably correct, although I am not familiar with this specific
> hardware. I guess that the same is true of mpc_write as well, which is
> even worse because write + read combined transactions are very common
> (while read + write are not.)
> 
> I'm not completely sure that mpc_xfer sends the stop. mpc_i2c_stop
> doesn't seem to do much.

Don't have the manual handy, but there is something bothering me with
the read function. After reading the last char there is nothing that
wait for the STOP to complete, instead one just exits and call
mpc_i2c_stop(). It might be so that the i2c_wait() won't complete until
the STOP has been sent, but I would not bet on it.

 Jocke