[PATCH v2] bsps/beagle: register i2c device at initialization

[Christian Mauderer]

On 19/09/2019 19:48, Vijay Kumar Banerjee wrote:
> 
> 
> 
> On Thu, Sep 19, 2019 at 10:05 PM Christian Mauderer <list at c-mauderer.de
> <mailto:list at c-mauderer.de>> wrote:
> 
>     Hello Vijay,
> 
>     sorry for being picky: What happens if something goes wrong during that
>     initialization? For example: If the system has a configuration where
>     /dev/i2c0 can't be created. In that case it will just silently fail to
>     create the i2c-0 which maybe leads to odd errors later.
> 
> This is a very good point. 
> 
>     There's already one precedent case how that could be handled: The
>     powerpc/mvme3100 BSP. That BSP also initializes an I2C driver with a
>     RTEMS_SYSINIT_ITEM. The return value is ignored too. But the
>     BSP_i2c_initialize does print error messages.
> 
>     I would suggest that here too: Check the return value of
>     am335x_i2c_bus_register in your new bbb_i2c_0_initialize and at least
>     print an error message that i2c0 couldn't be initialized. Most likely
>     you should use printk here because it's not sure whether printf would
>     work already.
> 
> Will it be sufficient to do this:
> ```
> static void bbb_i2c_0_initialize(void)
> {
>   int err;
> 
>   err = am335x_i2c_bus_register(BBB_I2C_0_BUS_PATH,
>                                 AM335X_I2C0_BASE,
>                                 I2C_BUS_CLOCK_DEFAULT,
>                                 BBB_I2C0_IRQ);
>   if (err != 0){
>     printk("i2c-0 device could not be registered");
>   }
> }
> ``` 

You might could add the value of `err` to the output (just at the end in
brackets or similar). Sometimes it is helpful to find the reason.
Otherwise I'm OK with it.

> 
>     Best regards
> 
>     Christian
> 
>     On 18/09/2019 21:18, Vijay Kumar Banerjee wrote:
>     > ---
>     >  bsps/arm/beagle/i2c/bbb-i2c.c     |  6 +++---
>     >  bsps/arm/beagle/include/bsp/i2c.h | 10 ----------
>     >  bsps/arm/beagle/start/bspstart.c  | 16 ++++++++++++++++
>     >  3 files changed, 19 insertions(+), 13 deletions(-)
>     >
>     > diff --git a/bsps/arm/beagle/i2c/bbb-i2c.c
>     b/bsps/arm/beagle/i2c/bbb-i2c.c
>     > index 37b88864b9..f705078085 100644
>     > --- a/bsps/arm/beagle/i2c/bbb-i2c.c
>     > +++ b/bsps/arm/beagle/i2c/bbb-i2c.c
>     > @@ -186,16 +186,16 @@ static int am335x_i2c_reset( bbb_i2c_bus *bus )
>     > 
>     >    bus->con_reg = 0;
>     >    regs->BBB_I2C_CON = bus->con_reg;
>     > -  udelay( 50000 );
>     > +  rtems_counter_delay_nanoseconds(50000000);
>     > 
>     >    regs->BBB_I2C_SYSC = AM335X_I2C_SYSC_SRST;
>     > -  udelay( 1000 );
>     > +  rtems_counter_delay_nanoseconds(1000000);
>     >    regs->BBB_I2C_CON = AM335X_I2C_CON_I2C_EN;
>     > 
>     >    while ( !( regs->BBB_I2C_SYSS & AM335X_I2C_SYSS_RDONE )
>     >            && timeout >= 0 ) {
>     >      --timeout;
>     > -    udelay( 100 );
>     > +  rtems_counter_delay_nanoseconds(100000);
>     >    }
>     > 
>     >    if ( timeout <= 0 ) {
>     > diff --git a/bsps/arm/beagle/include/bsp/i2c.h
>     b/bsps/arm/beagle/include/bsp/i2c.h
>     > index 9d253406bf..60f71194bf 100644
>     > --- a/bsps/arm/beagle/include/bsp/i2c.h
>     > +++ b/bsps/arm/beagle/include/bsp/i2c.h
>     > @@ -94,16 +94,6 @@ int am335x_i2c_bus_register(
>     >    rtems_vector_number irq
>     >  );
>     > 
>     > -static inline int bbb_register_i2c_0( void )
>     > -{
>     > -  return am335x_i2c_bus_register(
>     > -    BBB_I2C_0_BUS_PATH,
>     > -    AM335X_I2C0_BASE,
>     > -    I2C_BUS_CLOCK_DEFAULT,
>     > -    BBB_I2C0_IRQ
>     > -  );
>     > -}
>     > -
>     >  static inline int bbb_register_i2c_1( void )
>     >  {
>     >    return am335x_i2c_bus_register(
>     > diff --git a/bsps/arm/beagle/start/bspstart.c
>     b/bsps/arm/beagle/start/bspstart.c
>     > index 224f9ecf3b..8112e0867d 100644
>     > --- a/bsps/arm/beagle/start/bspstart.c
>     > +++ b/bsps/arm/beagle/start/bspstart.c
>     > @@ -17,6 +17,8 @@
>     >  #include <bsp/irq-generic.h>
>     >  #include <bsp/fdt.h>
>     >  #include <bsp/linker-symbols.h>
>     > +#include <bsp/i2c.h>
>     > +#include <rtems/sysinit.h>
>     > 
>     >  #include "bspdebug.h"
>     > 
>     > @@ -41,3 +43,17 @@ uint32_t bsp_fdt_map_intr(const uint32_t *intr,
>     size_t icells)
>     >  {
>     >    return intr[0];
>     >  }
>     > +
>     > +static void bbb_i2c_0_initialize(void)
>     > +{
>     > +  am335x_i2c_bus_register(BBB_I2C_0_BUS_PATH,
>     > +                          AM335X_I2C0_BASE,
>     > +                          I2C_BUS_CLOCK_DEFAULT,
>     > +                          BBB_I2C0_IRQ);
>     > +}
>     > +
>     > +RTEMS_SYSINIT_ITEM(
>     > +  bbb_i2c_0_initialize,
>     > +  RTEMS_SYSINIT_LAST,
>     > +  RTEMS_SYSINIT_ORDER_LAST
>     > +);
>     >
>