RE: [PATCH 16/24] omap4: Add i2c support on omap4 platform
Thanks Paul, I am just summarizing all my responses in this email. > -Original Message- > From: Paul Walmsley [mailto:p...@pwsan.com] > Sent: Wednesday, February 17, 2010 4:51 AM > To: Shilimkar, Santosh > Cc: t...@atomide.com; linux-omap@vger.kernel.org; b...@fluff.org.uk; > sa...@linux.intel.com; > l...@slimlogic.co.uk; Nayak, Rajendra; Syed, Rafiuddin; Ben Dooks > Subject: Re: [PATCH 16/24] omap4: Add i2c support on omap4 platform > > Hello Santosh, > > one general comment on this patch, before the specific comments. Rather > than adding more cpu_is_omap*() all over, please instead convert this code > to use flags passed in via platform_data. The I2C hwmod conversion > patches are one attempt to do that via the hwmod mechanism; you can use > that as an example of one way to do that with this driver. 1. As you suggested most of the driver clock related modifications can be avoided with dummy_nodes. Will do that. 2. Some drivers like gptimer, the clock node names chosen are different between omap4 and rest of the omaps. Since we do clk_get based on clk names, such cases, we need cpu check. Another way is to keep same clock nodes which means need to change database scripts. Which one you prefer ? 3. I shall the possibility of passing flags via board-files instead of cpu_is_() checks in drivers. This is ofcourse much cleaner approach if it's doable. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 16/24] omap4: Add i2c support on omap4 platform
> -Original Message-
> From: Tony Lindgren [mailto:t...@atomide.com]
> Sent: Wednesday, February 17, 2010 12:21 AM
> To: Shilimkar, Santosh
> Cc: linux-omap@vger.kernel.org; b...@fluff.org.uk; sa...@linux.intel.com;
> l...@slimlogic.co.uk;
> p...@pwsan.com; Syed, Rafiuddin; Ben Dooks
> Subject: Re: [PATCH 16/24] omap4: Add i2c support on omap4 platform
>
> * Santosh Shilimkar [100216 07:55]:
> > This patch is rebased version of earlier post to add I2C
> > driver support to OMAP4 platform. On OMAP4, all
> > I2C register address offsets are changed from OMAP1/2/3 I2C.
> > In order to not have #ifdef's at various places in code,
> > as well as to support multi-OMAP build, an array is created
> > to hold the register addresses with it's offset.
> >
> > This patch was submitted, reviewed and acked on mailing list
> > already. For more details refer below link
> > http://www.mail-archive.com/linux-...@vger.kernel.org/msg02281.html
> >
> > This version updated on top of 16 bit support added as part of
> > the commit "8bb209278e555a626f9637e844fe4c1b90e849f6"
> >
> > Signed-off-by: Syed Rafiuddin
> > Signed-off-by: Santosh Shilimkar
> > Acked-by: Kevin Hilman
> > CC: Tony Lindgren
> > CC: Ben Dooks
> > ---
> > arch/arm/plat-omap/i2c.c | 17 -
> > drivers/i2c/busses/i2c-omap.c | 148
> > -
> > 2 files changed, 131 insertions(+), 34 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> > index 96d2781..f18d757 100644
> > --- a/arch/arm/plat-omap/i2c.c
> > +++ b/arch/arm/plat-omap/i2c.c
> > @@ -53,9 +53,15 @@ static struct resource i2c_resources[][2] = {
> > #ifdefined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> > { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE2, INT_24XX_I2C2_IRQ) },
> > #endif
> > +#ifdefined(CONFIG_ARCH_OMAP4)
> > + { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE2, INT_44XX_I2C2_IRQ) },
> > +#endif
> > #ifdefined(CONFIG_ARCH_OMAP3)
> > { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE3, INT_34XX_I2C3_IRQ) },
> > #endif
> > +#ifdefined(CONFIG_ARCH_OMAP4)
> > + { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE3, INT_44XX_I2C3_IRQ) },
> > +#endif
> > };
>
> This is OK, but..
>
> > #define I2C_DEV_BUILDER(bus_id, res, data) \
> > @@ -72,10 +78,11 @@ static struct resource i2c_resources[][2] = {
> > static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> > static struct platform_device omap_i2c_devices[] = {
> > I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> > -#ifdefined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> > +#ifdefined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3) || \
> > + defined(CONFIG_ARCH_OMAP4)
> > I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> > #endif
> > -#ifdefined(CONFIG_ARCH_OMAP3)
> > +#ifdefined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
> > I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> > #endif
> > };
>
> .. the ifdefs above can be simplified to ifdef CONFIG_ARCH_OMAP2PLUS.
> That is, with the patches already queued into omap for-next.
YEP
>
> > @@ -370,7 +434,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
> > internal_clk = 9600;
> > else
> > internal_clk = 4000;
> > - fclk_rate = clk_get_rate(dev->fclk) / 1000;
> > + /* FIXME: Remove this once clock framework is available*/
> > + if (dev->rev >= OMAP_I2C_REV_ON_4430)
> > + fclk_rate = 96000;
> > + else
> > + fclk_rate = clk_get_rate(dev->fclk) / 1000;
> >
> > /* Compute prescaler divisor */
> > psc = fclk_rate / internal_clk;
>
> Sounds like this part should no longer be needed?
>
Yes. I missed this
> > *dev->buf++ = w;
> > dev->buf_len--;
> > - /* Data reg from 2430 is 8 bit wide */
> > + /* Data reg in 2430, omap3 and
> > +* omap4 is 8 bit wide
> > +*/
> > if (!cpu_is_omap2430() &&
> > - !cpu_is_omap34xx()) {
> > +
Re: [PATCH 16/24] omap4: Add i2c support on omap4 platform
Hello Santosh,
one general comment on this patch, before the specific comments. Rather
than adding more cpu_is_omap*() all over, please instead convert this code
to use flags passed in via platform_data. The I2C hwmod conversion
patches are one attempt to do that via the hwmod mechanism; you can use
that as an example of one way to do that with this driver.
On Tue, 16 Feb 2010, Santosh Shilimkar wrote:
> @@ -746,9 +814,12 @@ complete:
> if (dev->buf_len) {
> *dev->buf++ = w;
> dev->buf_len--;
> - /* Data reg from 2430 is 8 bit wide */
> + /* Data reg in 2430, omap3 and
> + * omap4 is 8 bit wide
> + */
The above comment needs to be fixed per CodingStyle.
> if (!cpu_is_omap2430() &&
> - !cpu_is_omap34xx()) {
> + !cpu_is_omap34xx() &&
> + !cpu_is_omap44xx()) {
> if (dev->buf_len) {
> *dev->buf++ = w >> 8;
> dev->buf_len--;
> @@ -786,9 +857,12 @@ complete:
> if (dev->buf_len) {
> w = *dev->buf++;
> dev->buf_len--;
> - /* Data reg from 2430 is 8 bit wide */
> + /* Data reg in 2430, omap3 and
> + * omap4 is 8 bit wide
> + */
The above comment needs to be fixed per CodingStyle.
> if (!cpu_is_omap2430() &&
> - !cpu_is_omap34xx()) {
> + !cpu_is_omap34xx() &&
> + !cpu_is_omap44xx()) {
> if (dev->buf_len) {
> w |= *dev->buf++ << 8;
> dev->buf_len--;
> @@ -897,6 +971,8 @@ omap_i2c_probe(struct platform_device *pdev)
>
> if (cpu_is_omap7xx())
> dev->reg_shift = 1;
> + else if (cpu_is_omap44xx())
> + dev->reg_shift = 0;
> else
> dev->reg_shift = 2;
>
> @@ -911,11 +987,16 @@ omap_i2c_probe(struct platform_device *pdev)
> if ((r = omap_i2c_get_clocks(dev)) != 0)
> goto err_iounmap;
>
> + if (cpu_is_omap44xx())
> + dev->regs = (u8 *) omap4_reg_map;
> + else
> + dev->regs = (u8 *) reg_map;
> +
> omap_i2c_unidle(dev);
>
> dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
>
> - if (cpu_is_omap2430() || cpu_is_omap34xx()) {
> + if (cpu_is_omap2430() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
> u16 s;
>
> /* Set up the fifo size - Get total size */
> @@ -927,8 +1008,13 @@ omap_i2c_probe(struct platform_device *pdev)
>* size. This is to ensure that we can handle the status on int
>* call back latencies.
>*/
> - dev->fifo_size = (dev->fifo_size / 2);
> - dev->b_hw = 1; /* Enable hardware fixes */
> + if (dev->rev >= OMAP_I2C_REV_ON_4430) {
> + dev->fifo_size = 0;
> + dev->b_hw = 0; /* Enable hardware fixes */
> + } else {
> + dev->fifo_size = (dev->fifo_size / 2);
> + dev->b_hw = 1; /* Disable hardware fixes */
You've got the comments backwards here. b_hw = 1 means "enable hardware
fixes" as noted above.
> + }
> }
>
> /* reset ASAP, clearing any IRQs */
> --
> 1.6.0.4
>
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/24] omap4: Add i2c support on omap4 platform
* Santosh Shilimkar [100216 07:55]:
> This patch is rebased version of earlier post to add I2C
> driver support to OMAP4 platform. On OMAP4, all
> I2C register address offsets are changed from OMAP1/2/3 I2C.
> In order to not have #ifdef's at various places in code,
> as well as to support multi-OMAP build, an array is created
> to hold the register addresses with it's offset.
>
> This patch was submitted, reviewed and acked on mailing list
> already. For more details refer below link
> http://www.mail-archive.com/linux-...@vger.kernel.org/msg02281.html
>
> This version updated on top of 16 bit support added as part of
> the commit "8bb209278e555a626f9637e844fe4c1b90e849f6"
>
> Signed-off-by: Syed Rafiuddin
> Signed-off-by: Santosh Shilimkar
> Acked-by: Kevin Hilman
> CC: Tony Lindgren
> CC: Ben Dooks
> ---
> arch/arm/plat-omap/i2c.c | 17 -
> drivers/i2c/busses/i2c-omap.c | 148
> -
> 2 files changed, 131 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index 96d2781..f18d757 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -53,9 +53,15 @@ static struct resource i2c_resources[][2] = {
> #if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE2, INT_24XX_I2C2_IRQ) },
> #endif
> +#if defined(CONFIG_ARCH_OMAP4)
> + { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE2, INT_44XX_I2C2_IRQ) },
> +#endif
> #if defined(CONFIG_ARCH_OMAP3)
> { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE3, INT_34XX_I2C3_IRQ) },
> #endif
> +#if defined(CONFIG_ARCH_OMAP4)
> + { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE3, INT_44XX_I2C3_IRQ) },
> +#endif
> };
This is OK, but..
> #define I2C_DEV_BUILDER(bus_id, res, data) \
> @@ -72,10 +78,11 @@ static struct resource i2c_resources[][2] = {
> static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> static struct platform_device omap_i2c_devices[] = {
> I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> +#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3) || \
> + defined(CONFIG_ARCH_OMAP4)
> I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> #endif
> -#if defined(CONFIG_ARCH_OMAP3)
> +#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
> I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> #endif
> };
.. the ifdefs above can be simplified to ifdef CONFIG_ARCH_OMAP2PLUS.
That is, with the patches already queued into omap for-next.
> @@ -370,7 +434,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
> internal_clk = 9600;
> else
> internal_clk = 4000;
> - fclk_rate = clk_get_rate(dev->fclk) / 1000;
> + /* FIXME: Remove this once clock framework is available*/
> + if (dev->rev >= OMAP_I2C_REV_ON_4430)
> + fclk_rate = 96000;
> + else
> + fclk_rate = clk_get_rate(dev->fclk) / 1000;
>
> /* Compute prescaler divisor */
> psc = fclk_rate / internal_clk;
Sounds like this part should no longer be needed?
> *dev->buf++ = w;
> dev->buf_len--;
> - /* Data reg from 2430 is 8 bit wide */
> + /* Data reg in 2430, omap3 and
> + * omap4 is 8 bit wide
> + */
> if (!cpu_is_omap2430() &&
> - !cpu_is_omap34xx()) {
> + !cpu_is_omap34xx() &&
> + !cpu_is_omap44xx()) {
> if (dev->buf_len) {
> *dev->buf++ = w >> 8;
> dev->buf_len--;
How about change this to if (cpu_class_is_omap1() || cpu_is_omap2420())?
That way it does not need to be patched for new omaps.
> @@ -786,9 +857,12 @@ complete:
> if (dev->buf_len) {
> w = *dev->buf++;
> dev->buf_len--;
> - /* Data reg from 2430 is 8 bit wide */
> + /* Data reg in 2430, omap3 and
> + * omap4 is 8 bit wide
> + */
> if (!cpu_is_omap2430() &&
> - !cpu_is_omap34xx()) {
> + !cpu_is_omap34xx() &&
> +
