On 9/27/24 9:01 AM, Venkatesh Yadav Abbarapu wrote:
[...]
struct onboard_hub { struct udevice *vdd; struct gpio_desc *reset_gpio; @@ -21,8 +27,86 @@ struct onboard_hub { struct onboard_hub_data { unsigned long reset_us; unsigned long power_on_delay_us;
- int (*i2c_init)(struct udevice *dev);
Why not call it simply "init" ?
};
+static int usb5744_i2c_init(struct udevice *dev) +{
- /*
* Prevent the MCU from the putting the HUB in suspend mode through register write.* The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2 register at address* 0x411D controls this aspect of the hub.* Format to write to hub registers via SMBus- 2D 00 00 05 00 01 41 1D 08* Byte 0: Address of slave 2D* Byte 1: Memory address 00* Byte 2: Memory address 00* Byte 3: Number of bytes to write to memory* Byte 4: Write configuration register (00)* Byte 5: Write the number of data bytes (01- 1 data byte)* Byte 6: LSB of register address 0x41* Byte 7: MSB of register address 0x1D* Byte 8: value to be written to the register*/- u8 data_buf[8] = {0x0, 0x5, 0x0, 0x1, 0x41, 0x1D, 0x08};
- int ret, slave_addr;
- u8 buf = USB5744_COMMAND_ATTACH;
- u8 config_reg_access_buf = USB5744_CONFIG_REG_ACCESS;
- struct dm_i2c_chip *i2c_chip;
- struct ofnode_phandle_args phandle;
- struct udevice *i2c_bus = NULL, *i2c_dev;
Make sure this list of variables is ordered as reverse xmas tree if possible.
- if (dev_read_phandle_with_args(dev, "i2c-bus", NULL, 0, 0, &phandle)) {
Return the error code:
ret = dev_read_phandle_with_args() if (ret) { ... return ret; }
dev_err(dev, "i2c-bus not specified\n");return -ENOENT;- }
- ret = device_get_global_by_ofnode(ofnode_get_parent(phandle.node), &i2c_bus);
- if (ret) {
dev_err(dev, "Failed to get i2c node, err: %d\n", ret);return ret;- }
- ret = ofnode_read_u32(phandle.node, "reg", &slave_addr);
- if (ret)
return ret;- ret = i2c_get_chip(i2c_bus, slave_addr, 1, &i2c_dev);
- if (ret) {
dev_dbg(dev, "%s: can't find i2c chip device for addr %x\n", __func__,slave_addr);return ret;- }
- i2c_chip = dev_get_parent_plat(i2c_dev);
Can this fail ? If so, is return 0 really the correct thing to do ?
- if (!i2c_chip)
return 0;- i2c_chip->flags &= ~DM_I2C_CHIP_WR_ADDRESS;
- /* SMBus write command */
- ret = dm_i2c_write(i2c_dev, 0, (uint8_t *)&data_buf, 8);
Is the uint8_t * cast necessary ?
- if (ret) {
dev_err(dev, "data_buf i2c_write failed, err:%d\n", ret);return ret;- }
- /* Configuration register access command */
- ret = dm_i2c_write(i2c_dev, USB5744_CONFIG_REG_ACCESS_LSB,
(uint8_t *)&config_reg_access_buf, 2);
Is the uint8_t * cast necessary ?
- if (ret) {
dev_err(dev, "config_reg_access i2c_write failed, err: %d\n", ret);return ret;- }
- /* USB Attach with SMBus */
- ret = dm_i2c_write(i2c_dev, USB5744_COMMAND_ATTACH_LSB, (uint8_t *)&buf, 2);
Is the uint8_t * cast necessary ?
- if (ret) {
dev_err(dev, "usb_attach i2c_write failed, err: %d\n", ret);return ret;- }
- return 0;
+}
- static int usb_onboard_hub_probe(struct udevice *dev) { struct onboard_hub_data *data =
@@ -61,7 +145,21 @@ static int usb_onboard_hub_probe(struct udevice *dev) udelay(data->power_on_delay_us); }
[...]