On Mon, Nov 10, 2008 at 12:30:07PM +0100, Guennadi Liakhovetski wrote:
> Hi Sascha,
>
> Thanks for the comments,
>
> On Mon, 10 Nov 2008, Sascha Hauer wrote:
>
> > > +static struct resource mxci2c1_resources[] = {
> > > + [0] = {
> > > + .start = I2C_BASE_ADDR,
> > > + .end = I2C_BASE_ADDR + SZ_4K - 1,
> > > + .flags = IORESOURCE_MEM,
> > > + }, [1] = {
> > > + .start = MXC_INT_I2C,
> > > + .end = MXC_INT_I2C,
> > > + .flags = IORESOURCE_IRQ,
> > > + },
> > > +};
> >
> > Can we drop the [0] and [1]? gcc knows how to count.
>
> I certainly hope so too:-) I normally only use indices in array
> initialisations when they are important, however, here this is not the
> case. Will drop.
>
> > > +{
> > > + switch (i2c_num) {
> > > + case 0:
> > > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_CLK, IOMUX_CONFIG_FUNC));
> > > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_I2C_DAT, IOMUX_CONFIG_FUNC));
> > > + break;
> > > + case 1:
> > > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MOSI,
> > > IOMUX_CONFIG_ALT1));
> > > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_MISO,
> > > IOMUX_CONFIG_ALT1));
> > > + break;
> > > + case 2:
> > > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SS2,
> > > IOMUX_CONFIG_ALT1));
> > > + mxc_iomux_mode(IOMUX_MODE(MX31_PIN_CSPI2_SCLK,
> > > IOMUX_CONFIG_ALT1));
> > > + }
> > > +}
> >
> > This should be moved to the platform part. You make the driver MX31
> > specific with this.
>
> First, even though the driver is called (also in Freescale sources) "mxc,"
> I am not sure which other *mx* SoCs have the same i2c controller, do you
> know that? So, it might anyway _be_ i.MX3* specific.
It works at least on MX2 (where I use a variant of this driver) and
should work on MX1 which has the same register layout.
> Next, I could just
> configure pins in i2c platform device registration code, but I don't know
> if anyone ever comes up with an idea to dynamically switch between, say,
> I2C #2 and SPI #2...
Sounds unlikely.
> Shall I make this a platform hook?
I asked myself whether it was worth the effort to implement a platform
hook for each and every device instead of just creating a long list of
pins and just configure them during board initialization. But this might
get interesting when it comes to powermanagement. So a platform hook
seems good, boards are still free to ignore the platform hooks and use a
list instead.
>
> > Also, care to update the macros in iomux-mx3.h
> > instead of constructing the pin definitions directly using IOMUX_MODE?
> > (btw how is the general feeling about these macros? I really like it
> > using these macros since I don't have to read the datasheet to lookup up
> > a specific pin, but I may be the only one)
>
> You mean these:
>
> /*
> * Convenience values for use with mxc_iomux_mode()
> *
> * Format here is MX31_PIN_(pin name)__(function)
> */
> #define MX31_PIN_CSPI3_MOSI__RXD3 IOMUX_MODE(MX31_PIN_CSPI3_MOSI,
> IOMUX_CONFIG_ALT1)
> ...
>
> ? Well, I usually only define macros if they are used more than once. If a
> value is used only once I usually don't bother... But if you like, I can
> define them, sure.
Well, they are used once for every board implementing I2C.
>
> > > +/**
> > > + * Function enables the I2C module and initializes the registers.
> > > + *
> > > + * @param dev the mxc i2c structure used to get to the right i2c
> > > device
> > > + * @param trans_flag transfer flag
> > > + */
> > > +static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
> > > +{
> > > + clk_enable(dev->clk);
> > > + /* Set the frequency divider */
> > > + writew(dev->clkdiv, dev->membase + MXC_IFDR);
> >
> > Can't we do this only once during probe()?
>
> Hm, would have to test...
>
> I'll address your other comments in the next version too.
Thank you
Sascha
--
Pengutronix - Linux Solutions for Science and Industry
Handelsregister: Amtsgericht Hildesheim, HRA 2686
Hannoversche Str. 2, 31134 Hildesheim, Germany
Phone: +49-5121-206917-0 | Fax: +49-5121-206917-9
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
