Skip to content

boards/arm/rp2040: Fix code style and error checking #16869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

boards/arm/rp2040: Fix code style and error checking #16869

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 62 additions & 25 deletions boards/arm/rp2040/common/src/rp2040_common_bringup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,10 @@ int rp2040_common_bringup(void)
struct mtd_dev_s *mtd_dev;
#endif

#ifdef CONFIG_RP2040_I2C
struct i2c_master_s *i2c;
#endif

#ifdef CONFIG_RP2040_I2C_DRIVER
#ifdef CONFIG_RP2040_I2C0
ret = board_i2cdev_initialize(0);
Expand Down Expand Up @@ -478,17 +482,21 @@ int rp2040_common_bringup(void)
{
syslog(LOG_ERR, "Failed to initialize SPI bus 0\n");
}

struct adc_dev_s *mcp3008 = mcp3008_initialize(spi);
if (mcp3008 == NULL)
{
syslog(LOG_ERR, "Failed to initialize MCP3008\n");
}

ret = adc_register("/dev/adc1", mcp3008);
if (ret < 0)
else
{
syslog(LOG_ERR, "Failed to register MCP3008 device driver: %d\n", ret);
struct adc_dev_s *mcp3008 = mcp3008_initialize(spi);
Copy link
Contributor

@linguini1 linguini1 Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd honestly fully remove the MCP3008 stuff

Copy link
Contributor

@cederom cederom Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually may be a good idea to remove external ic specific code from common rp2040 initialization code.. users may want to add it to their application :-)

if (mcp3008 == NULL)
{
syslog(LOG_ERR, "Failed to initialize MCP3008\n");
}
else
{
ret = adc_register("/dev/adc1", mcp3008);
if (ret < 0)
{
syslog(LOG_ERR, "Failed to register MCP3008: %d\n", ret);
}
}
}
#endif

Expand Down Expand Up @@ -546,43 +554,72 @@ int rp2040_common_bringup(void)

/* Try to register SHT4X device on I2C0 */

ret = sht4x_register(rp2040_i2cbus_initialize(0), 0,
CONFIG_SHT4X_I2C_ADDR);
if (ret < 0)
i2c = rp2040_i2cbus_initialize(0);
if (i2c == NULL)
{
syslog(LOG_ERR, "ERROR: couldn't initialize SHT4x: %d\n", ret);
syslog(LOG_ERR, "ERROR: failed to initialize I2C0\n");
}
else
{
ret = sht4x_register(i2c, 0, CONFIG_SHT4X_I2C_ADDR);
if (ret < 0)
{
syslog(LOG_ERR, "ERROR: couldn't initialize SHT4x: %d\n", ret);
}
}
#endif

#ifdef CONFIG_SENSORS_MCP9600
/* Try to register MCP9600 device as /dev/therm0 at I2C0. */

ret = mcp9600_register(rp2040_i2cbus_initialize(0), 0x60, 1, 2, 3);
if (ret < 0)
i2c = rp2040_i2cbus_initialize(0);
Copy link
Contributor

@hartmannathan hartmannathan Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if CONFIG_SENSORS_SHT4X and i2c is already initialized?

if (i2c == NULL)
{
syslog(LOG_ERR, "ERROR: couldn't initialize MCP9600: %d\n", ret);
syslog(LOG_ERR, "ERROR: failed to initialize I2C0\n");
}
else
{
ret = mcp9600_register(i2c, 0x60, 1, 2, 3);
if (ret < 0)
{
syslog(LOG_ERR, "ERROR: couldn't initialize MCP9600: %d\n", ret);
}
}
#endif

#ifdef CONFIG_SENSORS_MS56XX
/* Try to register MS56xx device at I2C0 */

ret = ms56xx_register(rp2040_i2cbus_initialize(0), 0, MS56XX_ADDR0,
MS56XX_MODEL_MS5611);
if (ret < 0)
i2c = rp2040_i2cbus_initialize(0);
Copy link
Contributor

@hartmannathan hartmannathan Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, what happens if CONFIG_SENSORS_SHT4X and/or CONFIG_SENSORS_MCP9600 and i2c is already initialized?

if (i2c == NULL)
{
syslog(LOG_ERR, "ERROR: couldn't register MS5611: %d\n", ret);
syslog(LOG_ERR, "ERROR: failed to initialize I2C0\n");
}
else
{
ret = ms56xx_register(i2c, 0, MS56XX_ADDR0, MS56XX_MODEL_MS5611);
if (ret < 0)
{
syslog(LOG_ERR, "ERROR: couldn't register MS5611: %d\n", ret);
}
}
#endif

#ifdef CONFIG_SENSORS_TMP112
/* Try to register TMP112 device at I2C0 with a common address */

ret = board_tmp112_initialize(rp2040_i2cbus_initialize(0), 0,
TMP112_ADDR_1);
if (ret < 0)
i2c = rp2040_i2cbus_initialize(0);
Copy link
Contributor

@hartmannathan hartmannathan Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, CONFIG_SENSORS_SHT4X and/or CONFIG_SENSORS_MCP9600 and/or CONFIG_SENSORS_MS56XX already initialized i2c?

Copy link
Contributor

@linguini1 linguini1 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all of these, it doesn't matter. The rp2040_i2cbus_initialize() function (and all xxxx_i2cbus_initialize() functions first check if the bus has already been initialized. If it has, it just returns the initialized i2cdev struct without re-performing the init.

if (i2c == NULL)
{
syslog(LOG_ERR, "ERROR: failed to initialize I2C0");
}
else
{
syslog(LOG_ERR, "Failed to initialize TMP112 driver: %d\n", ret);
ret = board_tmp112_initialize(i2c, 0, TMP112_ADDR_1);
if (ret < 0)
{
syslog(LOG_ERR, "Failed to initialize TMP112 driver: %d\n", ret);
}
}
#endif

Expand Down
Loading