Re: [CORRECTED] I2C driver supporting Moorestown and Medfield platform

[Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>]

On Mon, Aug 09, 2010 at 01:23:45PM +0100, Alan Cox wrote:
> > I would much prefer this to be called i2c-moorsetown, we have modern
> > systems which can handle >8 character names.
> 
> Moorestown. Everything else in the kernel uses 'mrst' so this would
> make the driver differ from the rest of the tree. I don't care too much
> what its called. Given its now for Medfield etc as well it probably
> should be i2c_intel_mid to match the rest of the kernel

Ok.
 
> > Hmm, if this is a synopsys block, then is it a standard one and if so
> > can we get some standard support?
> 
> See other reply on this one.

Annoying, I'd much rather have a core driver with some bus support
added as needed.

> > > +	if ((p1->flags ^ p2->flags) & I2C_M_TEN)
> > > +		return 1;
> > > +	return 0;
> > > +}
> > 
> > would have been better to return bool.
> 
> Can do but most of the kernel never uses bool so again it would be a
> quirky style. If its preferred i2c style no problem.

I thought it was introduced for this sort of thing... I've been using
it in all my new driver work.
 
> 
> > > +	for (i = 0; i < length; i++)
> > > +		mrst_i2c_write(i2c->base + IC_DATA_CMD,
> > > +			       (uint16_t)(*(buf + i)));
> > 
> > you say length in bytes, but write u16?
> > also, would be neater to have a u16 *buf?
> 
> Ermm no - that would be most peculiar as you'd then only send every
> alternate byte of data. Then again the cast ought to be implied by the
> types of mrst_i2c_write - will review

This code is rather easy to mis-understand, how about making it
easier to read, ie:

       (uint16_t)buf[i]

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.