RTEMS | Add the support for i2c driver in the aarch64/raspberrypi bsp (!363)

[Gedare Bloom (@gedare)]

Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363 was reviewed by Gedare Bloom

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125130

 > +  i2c_bus                 base;
 > +  uintptr_t               input_clock;
 > +  rtems_id                task_id;

This field is unused? remove unused fields from private structs

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/include/bsp/raspberrypi-i2c.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125131

 > +#include <dev/i2c/i2c.h>
 > +
 > +typedef enum {

please document this (public) data type

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125132

 > +static void i2c_polling_read( raspberrypi_i2c_bus *bus )
 > +{
 > +  while ( !( S_REG( bus ) & S_DONE ) && ( bus->remaining_bytes > 0 ) ) {

Can multiple callers attempt this in parallel? Do you need a lock in the `raspberrypi_i2c_bus` to protect its shared data?

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125133

 > +      bus->current_buffer++;
 > +      bus->remaining_bytes--;
 > +    }

Do you need to check for errors here?

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125134

 > +    ++divider;
 > +    clock_rate = BSC_CORE_CLK_HZ / divider;
 > +  }

I'm confused by this loop. Because of integer division, you should have that `clock_rate == clock` from the preceding divisions?

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125135

 > +  BCM2835_REG( bus->base_address + BCM2711_I2C_DIV ) = divider;
 > +
 > +  bus->input_clock = clock_rate;

This gets set, but where does it get used?

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125136

 > +                             );
 > +    BCM2835_REG( bus->base_address + BCM2711_I2C_DLEN ) = bus->remaining_bytes;
 > +    S_REG( bus ) = S_ERR | S_CLKT | S_DONE;

what's this doing here?

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125137

 > +
 > +
 > +    if ( msgs[ i ].flags & I2C_M_TEN ) // 10-bit slave address

don't use `//`

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125138

 > +    if ( msgs[ i ].flags & I2C_M_TEN ) // 10-bit slave address
 > +    {
 > +      /* Add the 8 lsbs of the 10-bit slave address to the fifo register*/

space before *

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125139

 > +}
 > +
 > +static int rpi_i2c_setup_transfer( raspberrypi_i2c_bus *bus )

this function seems misnamed, maybe `rpi_i2c_setup_and_transfer`?

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125140

 > +}
 > +
 > +static rtems_status_code i2c_gpio_init(

name it with `rpi_` like the other private functions in this file

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/i2c/raspberrypi-i2c.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125141

 > +  switch ( device ) {
 > +    case raspberrypi_bscm0:
 > +      bus_path = "/dev/i2c-0";

this could instead be done in `rpi_i2c_gpio_init`

--
  
Gedare Bloom started a new discussion on bsps/aarch64/raspberrypi/include/bsp/raspberrypi-i2c.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363#note_125142

 > +#define BSC_CORE_CLK_HZ 150000000
 > +
 > +#define C_REG( bus ) BCM2835_REG( ( bus )->base_address + BCM2711_I2C_CONTROL )

should not define this kind of macro in a public header file. you can move this ad the `S_REG` macros to the .c file.


-- 
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/363
You're receiving this email because of your account on gitlab.rtems.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20250624/cc5fe876/attachment-0001.htm>