| Message ID | 1241640919-4650-9-git-send-email-wd@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Grant Likely |
| Headers | show |
On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: > From: Piotr Ziecik <kosmo@semihalf.com> > > - Enabled I2C interrupts on MPC5121. > - Updated Kconfig for i2c-mpc driver. I think this workaround belongs in the driver itself. g. > > Signed-off-by: Piotr Ziecik <kosmo@semihalf.com> > Signed-off-by: Wolfgang Denk <wd@denx.de> > Cc: <linux-i2c@vger.kernel.org> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: John Rigby <jcrigby@gmail.com> > --- > arch/powerpc/platforms/512x/mpc5121_ads.c | 2 ++ > arch/powerpc/platforms/512x/mpc512x.h | 1 + > arch/powerpc/platforms/512x/mpc512x_shared.c | 24 ++++++++++++++++++++++++ > drivers/i2c/busses/Kconfig | 9 +++++---- > 4 files changed, 32 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c > index 441abc4..a8976b4 100644 > --- a/arch/powerpc/platforms/512x/mpc5121_ads.c > +++ b/arch/powerpc/platforms/512x/mpc5121_ads.c > @@ -42,6 +42,8 @@ static void __init mpc5121_ads_setup_arch(void) > for_each_compatible_node(np, "pci", "fsl,mpc5121-pci") > mpc83xx_add_bridge(np); > #endif > + > + mpc512x_init_i2c(); > } > > static void __init mpc5121_ads_init_IRQ(void) > diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platforms/512x/mpc512x.h > index 9c03693..f4db8a7 100644 > --- a/arch/powerpc/platforms/512x/mpc512x.h > +++ b/arch/powerpc/platforms/512x/mpc512x.h > @@ -13,5 +13,6 @@ > #define __MPC512X_H__ > extern unsigned long mpc512x_find_ips_freq(struct device_node *node); > extern void __init mpc512x_init_IRQ(void); > +extern void __init mpc512x_init_i2c(void); > void __init mpc512x_declare_of_platform_devices(void); > #endif /* __MPC512X_H__ */ > diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c > index 7135d89..b776e45 100644 > --- a/arch/powerpc/platforms/512x/mpc512x_shared.c > +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c > @@ -65,6 +65,30 @@ void __init mpc512x_init_IRQ(void) > ipic_set_default_priority(); > } > > +void __init mpc512x_init_i2c(void) > +{ > + struct device_node *np; > + void __iomem *i2cctl; > + > + /* Enable I2C interrupts */ > + np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-i2c-ctrl"); > + if (np) { > + i2cctl = of_iomap(np, 0); > + if (i2cctl) { > + /* > + * Set interrupt enable bits: > + * - I2C-0: bit 24, > + * - I2C-1: bit 26, > + * - I2C-2: bit 28. > + */ > + out_be32(i2cctl, 0x15000000); > + iounmap(i2cctl); > + } > + > + of_node_put(np); > + } > +} > + > /* > * Nodes to do bus probe on, soc and localbus > */ > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index a48c8ae..57ed637 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -391,13 +391,14 @@ config I2C_IXP2000 > instead. > > config I2C_MPC > - tristate "MPC107/824x/85xx/52xx/86xx" > + tristate "MPC107/824x/85xx/512x/52xx/86xx" > depends on PPC32 > help > If you say yes to this option, support will be included for the > - built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245 and > - MPC85xx/MPC8641 family processors. The driver may also work on 52xx > - family processors, though interrupts are known not to work. > + built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245, > + MPC85xx/MPC8641 and MPC512x family processors. The driver may > + also work on 52xx family processors, though interrupts are known > + not to work. > > This driver can also be built as a module. If so, the module > will be called i2c-mpc. > -- > 1.6.0.6 > >
Dear Grant Likely, In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com> you wrote: > On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: > > From: Piotr Ziecik <kosmo@semihalf.com> > > > > - Enabled I2C interrupts on MPC5121. > > - Updated Kconfig for i2c-mpc driver. > > I think this workaround belongs in the driver itself. Sorry, I don't get it. Which workaround? What exactly should I change? Best regards, Wolfgang Denk
On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Grant Likely, > > In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com> you wrote: >> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: >> > From: Piotr Ziecik <kosmo@semihalf.com> >> > >> > - Enabled I2C interrupts on MPC5121. >> > - Updated Kconfig for i2c-mpc driver. >> >> I think this workaround belongs in the driver itself. > > Sorry, I don't get it. Which workaround? What exactly should I change? Sorry, I misread the patch. Never mind. g.
On Wed, May 6, 2009 at 4:51 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd@denx.de> wrote: >> Dear Grant Likely, >> >> In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com> you wrote: >>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: >>> > From: Piotr Ziecik <kosmo@semihalf.com> >>> > >>> > - Enabled I2C interrupts on MPC5121. >>> > - Updated Kconfig for i2c-mpc driver. >>> >>> I think this workaround belongs in the driver itself. >> >> Sorry, I don't get it. Which workaround? What exactly should I change? > > Sorry, I misread the patch. Never mind. Actually, on 3rd reading, I think my first impression was correct (even if I was wrong about it being a workaround). This code in mpc512x_init_i2c() is only relevant for i2c busses (it isn't shared with any other drivers). Therefore, it belongs with the i2c bus itself. It does not belong in platform code. Cheers, g.
Grant Likely wrote: > On Wed, May 6, 2009 at 4:51 PM, Grant Likely <grant.likely@secretlab.ca> wrote: >> On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd@denx.de> wrote: >>> Dear Grant Likely, >>> >>> In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com> you wrote: >>>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: >>>>> From: Piotr Ziecik <kosmo@semihalf.com> >>>>> >>>>> - Enabled I2C interrupts on MPC5121. >>>>> - Updated Kconfig for i2c-mpc driver. >>>> I think this workaround belongs in the driver itself. >>> Sorry, I don't get it. Which workaround? What exactly should I change? >> Sorry, I misread the patch. Never mind. > > Actually, on 3rd reading, I think my first impression was correct > (even if I was wrong about it being a workaround). This code in > mpc512x_init_i2c() is only relevant for i2c busses (it isn't shared > with any other drivers). Therefore, it belongs with the i2c bus > itself. It does not belong in platform code. Right. Furthermore, the i2c-mpc.c should be extened to support bus speed setting for the MPC512x, which has been merged recently (see commit id f2bd5efe). Wolfgang.
Ok, the interrupt enabling should happen in the driver. Should it key off compatible or should a new property be added like the existing 5200 clocking property? On Wed, May 6, 2009 at 8:41 PM, Grant Likely <grant.likely@secretlab.ca>wrote: > On Wed, May 6, 2009 at 4:51 PM, Grant Likely <grant.likely@secretlab.ca> > wrote: > > On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd@denx.de> wrote: > >> Dear Grant Likely, > >> > >> In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com> > you wrote: > >>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: > >>> > From: Piotr Ziecik <kosmo@semihalf.com> > >>> > > >>> > - Enabled I2C interrupts on MPC5121. > >>> > - Updated Kconfig for i2c-mpc driver. > >>> > >>> I think this workaround belongs in the driver itself. > >> > >> Sorry, I don't get it. Which workaround? What exactly should I change? > > > > Sorry, I misread the patch. Never mind. > > Actually, on 3rd reading, I think my first impression was correct > (even if I was wrong about it being a workaround). This code in > mpc512x_init_i2c() is only relevant for i2c busses (it isn't shared > with any other drivers). Therefore, it belongs with the i2c bus > itself. It does not belong in platform code. > > Cheers, > g. > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. >
On Thu, May 7, 2009 at 8:12 PM, John Rigby <jcrigby@gmail.com> wrote: > Ok, the interrupt enabling should happen in the driver. Should it key off > compatible or should a new property be added like the existing 5200 clocking > property? key off compatible. And the 5200 clocking property has been depreciated. g.
> Right. Furthermore, the i2c-mpc.c should be extened to support bus speed > setting for the MPC512x, which has been merged recently (see commit id > f2bd5efe). I have simple question about bus speed setting support. Existing implementation uses default safe speed if there is no 'clock-frequency' property in i2c node. Comments in code suggest that this behaviour is left for backward compatibility only. Should I make the 'clock-frequency' property mandatory for a new type of I2C controller (MPC5121) ?
2009/5/18 Piotr Zięcik <kosmo@semihalf.com>: >> Right. Furthermore, the i2c-mpc.c should be extened to support bus speed >> setting for the MPC512x, which has been merged recently (see commit id >> f2bd5efe). > > I have simple question about bus speed setting support. Existing > implementation uses default safe speed if there is no 'clock-frequency' > property in i2c node. Comments in code suggest that this behaviour is left > for backward compatibility only. Should I make the 'clock-frequency' > property mandatory for a new type of I2C controller (MPC5121) ? yes. At least in documentation, but I wouldn't do extra work to disable that behaviour for 5121 boards. g.
Piotr Zięcik wrote: >> Right. Furthermore, the i2c-mpc.c should be extened to support bus speed >> setting for the MPC512x, which has been merged recently (see commit id >> f2bd5efe). > > I have simple question about bus speed setting support. Existing > implementation uses default safe speed if there is no 'clock-frequency' > property in i2c node. Comments in code suggest that this behaviour is left > for backward compatibility only. Should I make the 'clock-frequency' > property mandatory for a new type of I2C controller (MPC5121) ? I don't think so, for the same backward compatibility reason as for the other boards. But the DTS file might be changed to use clock-frequency. BTW, when I wrote the code I already had the MPC512x in mind. IIRC, the FDR table and the function scanning it for the MPC52xx should work for the MPC512x as well. It's mainly a matter of using the proper function to get the source clock frequency and fiddling with multiple-arch support. Wolfgang.
Monday 18 May 2009 16:29:04 Wolfgang Grandegger napisał(a): > > I have simple question about bus speed setting support. Existing > > implementation uses default safe speed if there is no 'clock-frequency' > > property in i2c node. Comments in code suggest that this behaviour is > > left for backward compatibility only. Should I make the 'clock-frequency' > > property mandatory for a new type of I2C controller (MPC5121) ? > > I don't think so, for the same backward compatibility reason as for the > other boards. But the DTS file might be changed to use clock-frequency. In my opinion implementing "backward compatilility" for MPC5121 is not good idea. MPC5121 I2C support is completly new thing in mainline. Simply, there is no DTS with which I have to be compatible. Adding backward compatibility with nothing may be confusing.
Piotr Zięcik wrote: > Monday 18 May 2009 16:29:04 Wolfgang Grandegger napisał(a): >>> I have simple question about bus speed setting support. Existing >>> implementation uses default safe speed if there is no 'clock-frequency' >>> property in i2c node. Comments in code suggest that this behaviour is >>> left for backward compatibility only. Should I make the 'clock-frequency' >>> property mandatory for a new type of I2C controller (MPC5121) ? >> I don't think so, for the same backward compatibility reason as for the >> other boards. But the DTS file might be changed to use clock-frequency. > > In my opinion implementing "backward compatilility" for MPC5121 is not good > idea. MPC5121 I2C support is completly new thing in mainline. Simply, there is > no DTS with which I have to be compatible. Adding backward compatibility > with nothing may be confusing. There is a port for the MPC5121 in mainline since 2.6.25 including DTS file: http://lxr.linux.no/linux+v2.6.25/arch/powerpc/boot/dts/mpc5121ads.dts If it was really usable or even used is another question. But it's fine for me be more restrictive, e.g. print a warning when save values are selected. Wolfgang.
diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c index 441abc4..a8976b4 100644 --- a/arch/powerpc/platforms/512x/mpc5121_ads.c +++ b/arch/powerpc/platforms/512x/mpc5121_ads.c @@ -42,6 +42,8 @@ static void __init mpc5121_ads_setup_arch(void) for_each_compatible_node(np, "pci", "fsl,mpc5121-pci") mpc83xx_add_bridge(np); #endif + + mpc512x_init_i2c(); } static void __init mpc5121_ads_init_IRQ(void) diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platforms/512x/mpc512x.h index 9c03693..f4db8a7 100644 --- a/arch/powerpc/platforms/512x/mpc512x.h +++ b/arch/powerpc/platforms/512x/mpc512x.h @@ -13,5 +13,6 @@ #define __MPC512X_H__ extern unsigned long mpc512x_find_ips_freq(struct device_node *node); extern void __init mpc512x_init_IRQ(void); +extern void __init mpc512x_init_i2c(void); void __init mpc512x_declare_of_platform_devices(void); #endif /* __MPC512X_H__ */ diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c index 7135d89..b776e45 100644 --- a/arch/powerpc/platforms/512x/mpc512x_shared.c +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c @@ -65,6 +65,30 @@ void __init mpc512x_init_IRQ(void) ipic_set_default_priority(); } +void __init mpc512x_init_i2c(void) +{ + struct device_node *np; + void __iomem *i2cctl; + + /* Enable I2C interrupts */ + np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-i2c-ctrl"); + if (np) { + i2cctl = of_iomap(np, 0); + if (i2cctl) { + /* + * Set interrupt enable bits: + * - I2C-0: bit 24, + * - I2C-1: bit 26, + * - I2C-2: bit 28. + */ + out_be32(i2cctl, 0x15000000); + iounmap(i2cctl); + } + + of_node_put(np); + } +} + /* * Nodes to do bus probe on, soc and localbus */ diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index a48c8ae..57ed637 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -391,13 +391,14 @@ config I2C_IXP2000 instead. config I2C_MPC - tristate "MPC107/824x/85xx/52xx/86xx" + tristate "MPC107/824x/85xx/512x/52xx/86xx" depends on PPC32 help If you say yes to this option, support will be included for the - built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245 and - MPC85xx/MPC8641 family processors. The driver may also work on 52xx - family processors, though interrupts are known not to work. + built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245, + MPC85xx/MPC8641 and MPC512x family processors. The driver may + also work on 52xx family processors, though interrupts are known + not to work. This driver can also be built as a module. If so, the module will be called i2c-mpc.