Wolfgang Denk <[EMAIL PROTECTED]> wrote:
> Dear Olav Morken,
>
> In message <[EMAIL PROTECTED]> you wrote:
> > This patch adds support for the AT32UC3A0xxx chips.
> >
> > Signed-off-by: Gunnar Rangoy <[EMAIL PROTECTED]>
> > Signed-off-by: Paul Driveklepp <[EMAIL PROTECTED]>
> > Signed-off-by: Olav Morken <[EMAIL PROTECTED]>
>
> Coding style violations: C++ comments, indentation not by TAB, too
> long lines, incorrect multiline comment style.
>
> ...
> > +static inline unsigned long get_hsb_clk_rate(void)
> > +{
> > + //TODO HSB is always the same as cpu-rate
> -------^^
> > + return MAIN_CLK_RATE >> CFG_CLKDIV_CPU;
> > +}
> > +static inline unsigned long get_pba_clk_rate(void)
> > +{
> > + return MAIN_CLK_RATE >> CFG_CLKDIV_PBA;
> > +}
> > +static inline unsigned long get_pbb_clk_rate(void)
> > +{
> > + return MAIN_CLK_RATE >> CFG_CLKDIV_PBB;
> > +}
> > +
> > +/* Accessors for specific devices. More will be added as needed. */
> > +static inline unsigned long get_sdram_clk_rate(void)
> > +{
> > + return get_hsb_clk_rate();
> > +}
> > +#ifdef AT32UC3A0xxx_CHIP_HAS_USART
> > +static inline unsigned long get_usart_clk_rate(unsigned int dev_id)
> > +{
> > + return get_pba_clk_rate();
> > +}
> > +#endif
> > +#ifdef AT32UC3A0xxx_CHIP_HAS_MACB
> > +static inline unsigned long get_macb_pclk_rate(unsigned int dev_id)
> > +{
> > + return get_pbb_clk_rate();
> > +}
> > +static inline unsigned long get_macb_hclk_rate(unsigned int dev_id)
> > +{
> > + return get_hsb_clk_rate();
> > +}
> > +#endif
> > +#ifdef AT32UC3A0xxx_CHIP_HAS_SPI
> > +static inline unsigned long get_spi_clk_rate(unsigned int dev_id)
> > +{
> > + return get_pba_clk_rate();
> > +}
> > +#endif
>
> Would it make sense to provide weak functions the get defined
> accordingly? A function which only calls another function looks
> stupid to me.
This matches other AVR32 chips. I'm not sure how you intend to use weak
functions here...the whole point of these things is to make drivers
unaware of which bus a certain peripheral is connected to.
I suppose the same thing could be done using aliases, but then you
can't do it inline, which in turn means that the constant calculations
won't be constant anymore, and the code gets larger and slower as a
result.
> > +#ifdef AT32UC3A0xxx_CHIP_HAS_USART
> > +static inline void portmux_enable_usart0(unsigned long drive_strength)
> > +{
> > + portmux_select_peripheral(PORTMUX_PORT_A, (1 << 0) | (1 << 1),
> > + PORTMUX_FUNC_A, 0);
> > +}
> > +
> > +static inline void portmux_enable_usart1(unsigned long drive_strength)
> > +{
> > + portmux_select_peripheral(PORTMUX_PORT_A, (1 << 5) | (1 << 6),
> > + PORTMUX_FUNC_A, 0);
> > +}
> > +
> > +static inline void portmux_enable_usart2(unsigned long drive_strength)
> > +{
> > + portmux_select_peripheral(PORTMUX_PORT_B, (1 << 29) | (1 << 30),
> > + PORTMUX_FUNC_A, 0);
> > +}
> > +
> > +static inline void portmux_enable_usart3(unsigned long drive_strength)
> > +{
> > + portmux_select_peripheral(PORTMUX_PORT_B, (1 << 10) | (1 << 11),
> > + PORTMUX_FUNC_B, 0);
> > +}
> > +#endif
>
> I don't like this either.
Why not? I think it's pretty elegant -- the board code only needs to
specify which USARTs to enable, it doesn't need to know which pins and
functions it corresponds to.
That said, it might be a better idea to make a single function with a
switch () block.
Haavard
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
