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