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_121497

 >  #include <dev/i2c/i2c.h>
 >  #include <bsp/irq.h>
 > +#include <stdio.h>

Why do you add this header?

--
  
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_121498

 > -{
 > -    while (bus->remaining_bytes >= 1)
 > +static int rpi_i2c_bus_transfer(raspberrypi_i2c_bus *bus) {

why do you change the format of existing lines?

--
  
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_121499

 > -            BCM2835_REG(bus->base_address + BCM2711_I2C_FIFO) = *(bus->current_buffer);
 > +        while (!(BCM2835_REG(bus->base_address + BCM2711_I2C_STATUS) & S_DONE )) {
 > +            while ((BCM2835_REG(bus->base_address + BCM2711_I2C_STATUS) & S_RXD ) && (bus->remaining_bytes > 0)) {

This line looks too long. The logic here is aa bit complicated. I suggest adding a helper function/macro for this repeated expression `BCM2835_REG(bus->base_address + BCM2711_I2C_STATUS)`

--
  
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_121500

 > +            
 > +            // If we've read all expected bytes, we can exit
 > +            if (bus->remaining_bytes == 0) {

I think this is already handled by the loop conditional?

--
  
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_121501

 > +static int rpi_i2c_bus_transfer(raspberrypi_i2c_bus *bus) {
 > +    if (bus->read_transfer)
 >      {

consider adding helper functions for separating the logic for read/write 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_121502

 > +                bus->current_buffer++;
 > +                bus->remaining_bytes--;
 > +                if(bus->remaining_bytes == 0){

handled by the loop?

--
  
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_121503

 > +                }
 > +            }
 > +            if(bus->remaining_bytes == 0){

I think this is superfluous.

--
  
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_121504

 > -    while (bus->remaining_transfers > 0)
 > -    {
 > +    while(bus->remaining_transfers > 0){

avoid making random formatting changes to existing code.

--
  
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_121505

 >          rv = rpi_i2c_bus_transfer(bus);
 > -
 > +        

ditto

--
  
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_121506

 >          {
 > -            BCM2835_REG(bus->base_address + BCM2711_I2C_CONTROL) &= ~(1 << 0);
 > +            BCM2835_REG(bus->base_address + BCM2711_I2C_CONTROL) |= 0 | C_ST; // Write packet transfer

 `0 |` is superfluous

--
  
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_121507

 >          raspberrypi_gpio_set_function(1, GPIO_AF0);
 > -        bus->base_address = 0x00205000;
 > +        bus->base_address = RPI_PERIPHERAL_BASE + 0x00205000;

should there be a macro for this (and the below) offset value?


-- 
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/20250320/c8ded834/attachment-0001.htm>