[PATCH v2 2/5] usb: tcpm: fusb302: add driver

[Marek Vasut]

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;
> +	}

[...]