On 6/4/24 6:33 PM, Sebastian Reichel wrote:
[...]
+static int fusb302_i2c_block_write(struct udevice *dev, u8 address,
+ u8 length, const u8 *data)
+{
+ int ret = 0;
This initialization of ret = 0 is probably unnecessary, see right below.
+ if (length <= 0)
+ return ret;
Should this return -EINVAL if length < 0 ?
+ ret = dm_i2c_write(dev, address, data, length);
+ if (ret)
+ dev_err(dev, "cannot block write 0x%02x, len=%d, ret=%d\n",
+ address, length, ret);
+
+ return ret;
+}
+
+static int fusb302_i2c_read(struct udevice *dev, u8 address, u8 *data)
+{
+ int ret = 0, retries;
Plain 'int ret;' is enough here , ret is always assigned in the loop below.
+ for (retries = 0; retries < 3; retries++) {
+ ret = dm_i2c_read(dev, address, data, 1);
+ if (ret == 0)
+ return ret;
+ dev_err(dev, "cannot read %02x, ret=%d\n", address, ret);
+ }
+
+ return ret;
+}
+
+static int fusb302_i2c_block_read(struct udevice *dev, u8 address,
+ u8 length, u8 *data)
+{
+ int ret = 0;
int ret;
+ if (length <= 0)
+ return ret;
return -EINVAL;
+ ret = dm_i2c_read(dev, address, data, length);
+ if (ret)
+ dev_err(dev, "cannot block read 0x%02x, len=%d, ret=%d\n",
+ address, length, ret);
+ return ret;
+}
+
+static int fusb302_i2c_mask_write(struct udevice *dev, u8 address,
+ u8 mask, u8 value)
+{
+ int ret = 0;
int ret;
+ u8 data;
+
+ ret = fusb302_i2c_read(dev, address, &data);
+ if (ret)
+ return ret;
+ data &= ~mask;
+ data |= value;
+ ret = fusb302_i2c_write(dev, address, data);
+ if (ret)
+ return ret;
+
+ return ret;
+}
+
+static int fusb302_i2c_set_bits(struct udevice *dev, u8 address, u8 set_bits)
+{
+ return fusb302_i2c_mask_write(dev, address, 0x00, set_bits);
+}
+
+static int fusb302_i2c_clear_bits(struct udevice *dev, u8 address, u8
clear_bits)
+{
+ return fusb302_i2c_mask_write(dev, address, clear_bits, 0x00);
+}
+
+static int fusb302_sw_reset(struct udevice *dev)
+{
+ int ret = fusb302_i2c_write(dev, FUSB_REG_RESET,
FUSB_REG_RESET_SW_RESET);
+
+ if (ret)
+ dev_err(dev, "cannot sw reset the fusb302: %d\n", ret);
+
+ return ret;
+}
+
+static int fusb302_enable_tx_auto_retries(struct udevice *dev, u8 retry_count)
+{
+ int ret = 0;
int ret; , please fix globally
+ ret = fusb302_i2c_set_bits(dev, FUSB_REG_CONTROL3, retry_count |
+ FUSB_REG_CONTROL3_AUTO_RETRY);
+
+ return ret;
+}
[...]
+static int fusb302_set_cc(struct udevice *dev, enum typec_cc_status cc)
+{
+ struct fusb302_chip *chip = dev_get_priv(dev);
+ u8 switches0_mask = FUSB_REG_SWITCHES0_CC1_PU_EN |
+ FUSB_REG_SWITCHES0_CC2_PU_EN |
+ FUSB_REG_SWITCHES0_CC1_PD_EN |
+ FUSB_REG_SWITCHES0_CC2_PD_EN;
+ u8 rd_mda, switches0_data = 0x00;
The masks can be const u8 .
+ int ret = 0;
[...]
+static int fusb302_set_polarity(struct udevice *dev,
+ enum typec_cc_polarity polarity)
+{
+ return 0;
The uclass should check if callback is available and not call it if it
is not.
+}
[...]
+static int fusb302_pd_send_message(struct udevice *dev,
+ const struct pd_message *msg)
+{
+ int ret = 0;
+ u8 buf[40];
+ u8 pos = 0;
+ int len;
+
+ /* SOP tokens */
+ buf[pos++] = FUSB302_TKN_SYNC1;
+ buf[pos++] = FUSB302_TKN_SYNC1;
+ buf[pos++] = FUSB302_TKN_SYNC1;
+ buf[pos++] = FUSB302_TKN_SYNC2;
Why not do this ?
u8 buf[40] = { FUSB302_TKN_SYNC1, FUSB302_TKN_SYNC1, FUSB302_TKN_SYNC1,
FUSB302_TKN_SYNC2 };
u8 pos = 4;
+ len = pd_header_cnt_le(msg->header) * 4;
+ /* plug 2 for header */
+ len += 2;
+ if (len > 0x1F) {
Magic value should be defined in some macro.
+ dev_err(dev, "PD message too long %d (incl. header)", len);
+ return -EINVAL;
+ }
[...]
