Re: [PATCH] i2c: SuperH Mobile I2C Bus Controller V4

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Ben, Magnus, Paul,

This is Ben's first review as the new i2c subsystem co-maintainer. I
will just make a couple of comments on top of that to clarify a few
things. Thanks _a lot_ Ben for stepping in and doing that work :)

On Sat, 29 Mar 2008 17:57:32 +0000, Ben Dooks wrote:
> On Fri, Mar 28, 2008 at 06:35:30PM +0900, Magnus Damm wrote:
> > This is V4 of the SuperH Mobile I2C Controller Driver. A simple Master
> > only driver for the I2C block included in processors such as sh7343,
> > sh7722 and sh7723. Tested on a sh7722 MigoR using a rs5c732b rtc.
> > 
> > Changes since V3:
> >  - Added SUPERH Kconfig dependency
> >  - Use spin_lock_irqsave() and spin_unlock_irqrestore()
> >  - Use IRQF_DISABLED
> >  - Checkpatch fixes
> > Changes since V2:
> >  - dev_xxx() use
> >  - Kill off superfluous ioarea resource
> >  - Add SMBus emulation
> > Changes since V1:
> >  - Use clk_get()/clk_put()/clk_get_rate() to get peripheral clock rate.
> >  - Use pdev->dev.bus_id instead of dev->name
> 
> Do we really need these in the change message, we tend to assume that
> the changes before submission aren't really relevant. I would advise
> on removing these.

In fact the right place for these (useful) temporary changelog entries
is before the diffstat, right below, so that the reviewers can see them,
but they don't go in git.

> > Verified with the rtc-rs5c372 SMBus conversion patches currently in -mm.
> > 
> > Signed-off-by: Magnus Damm <damm-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
> > Signed-off-by: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
> > ---
> > 
> >  drivers/i2c/busses/Kconfig         |   10 
> >  drivers/i2c/busses/Makefile        |    1 
> >  drivers/i2c/busses/i2c-sh_mobile.c |  498 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 509 insertions(+)
> > 
> > --- 0001/drivers/i2c/busses/Kconfig
> > +++ work/drivers/i2c/busses/Kconfig	2008-03-28 16:45:42.000000000 +0900
> > (...)

> (...)
> IIRC, the error for not receiving an ACK is -ECONNREFUSED when
> transfering data and -EREMOTEIO when sending the device address,
> and you should also check the message flags appropriately for
> I2C_M_IGNORE_ACK.

Actually not. I2C_M_IGNORE_ACK's implementation is purely optional, and
as this is a violation of the I2C standard, the less implemented it
gets, the happier I am.

> > (...)
> > +	/* Calculate the value for iccl. From the data sheet:
> > +	 * iccl = (p clock ÷ transfer rate) × (L ÷ (L + H))
> > +	 * where L and H are the SCL low/high ratio (5/4 in this case).
> > +	 * We also round off the result.
> > +	 */
> 
> there are some weird encoding characters in this message, making it
> difficult to work out what is going on here.

I totally second this. Please stick to ASCII as much as possible, it's
way easier for everyone.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c