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.
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
