Message-ID: <ZfywPHEgYEhFSkGS@duo.ucw.cz>
Date: Thu, 21 Mar 2024 23:10:04 +0100
From: Pavel Machek <pavel@....cz>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: phone-devel@...r.kernel.org, kernel list <linux-kernel@...r.kernel.org>,
	fiona.klute@....de, martijn@...xit.nl, samuel@...lland.org,
	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org, megi@....cz
Subject: Re: [PATCHv2 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C
 HDMI bridge

Hi!

> I'm sorry to keep you waiting.

Thanks for comments.

> > +	struct gpio_desc *gpio_reset;
> > +	struct gpio_desc *gpio_cabledet;
> > +
> > +	uint32_t src_caps[8];
> 
> Use u32 instead of uint32_t.

Will replace globally.

> > +static int anx7688_reg_read(struct anx7688 *anx7688, u8 reg_addr)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(anx7688->client, reg_addr);
> > +	if (ret < 0)
> > +		dev_err(anx7688->dev, "i2c read failed at 0x%x (%d)\n",
> > +			reg_addr, ret);
> 
> dev_dbg instead.

I'd prefer not to. i2c functions should not really fail, and if they
do, we want the error log. This is not debugging, this is i2c failing.

> > +static void anx7688_power_enable(struct anx7688 *anx7688)
> > +{
> > +	gpiod_set_value(anx7688->gpio_reset, 1);
> > +	gpiod_set_value(anx7688->gpio_enable, 1);
> > +
> > +	/* wait for power to stabilize and release reset */
> > +	msleep(10);
> 
> So is it okay that the sleep may take up to 20ms?

I don't see how that would be a problem.

> > +	gpiod_set_value(anx7688->gpio_reset, 0);
> > +	udelay(2);
> 
> Use usleep_range() instead.

Can do, but it makes no difference.

> > +	gpiod_set_value(anx7688->gpio_reset, 1);
> > +	msleep(5);
> 
> The same question here, is it a problem if the sleep ends up taking
> 20ms?

Again, I expect that to be ok.

> > +	ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND);
> > +	if (ret) {
> > +		dev_err(anx7688->dev,
> > +			"failed to send pd packet (tx buffer full)\n");
> 
> One line should be enought for that one.

That makes it go over 80 columns, but yes, can be one line.

> > +	// wait until the message is processed (30ms max)
> > +	for (i = 0; i < 300; i++) {
> > +		ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND);
> > +		if (ret <= 0)
> > +			return ret;
> > +
> > +		udelay(100);
> > +	}
> > +
> > +	dev_err(anx7688->dev, "timeout waiting for the message queue flush\n");
> 
> Maybe dev_dbg for this too.

Let's not hide these. If they happen, we can downgrade them, but they
should not.

> > +	/* wait till the firmware is loaded (typically ~30ms) */
> > +	for (i = 0; i < 100; i++) {
> > +		ret = anx7688_reg_read(anx7688, ANX7688_REG_EEPROM_LOAD_STATUS0);
> > +
> > +		if (ret >= 0 && (ret & ANX7688_EEPROM_FW_LOADED) == ANX7688_EEPROM_FW_LOADED) {
> > +			dev_dbg(anx7688->dev, "eeprom0 = 0x%02x\n", ret);
> > +			dev_info(anx7688->dev, "fw loaded after %d ms\n", i * 10);
> 
> Debugging information. Use dev_dbg.

Ok.

> > +	set_bit(ANX7688_F_FW_FAILED, anx7688->flags);
> > +	dev_err(anx7688->dev, "boot firmware load failed (you may need to flash FW to anx7688 first)\n");
> > +	ret = -ETIMEDOUT;
> > +	goto err_vconoff;
> > +
> > +fw_loaded:
> 
> This label looks a bit messy to me. You could also move that firmware
> loading wait to its own function.

Ok, let me try to refactor this.

> > +static int anx7688_handle_pd_message_response(struct anx7688 *anx7688,
> > +					      u8 to_cmd, u8 resp)
> > +{
...
> > +	return 0;
> > +}
> 
> Noise. Drop this whole function. If you need this kind of information,
> then please consider trace points, or just use some debugfs trick like
> what we have in drivers/usb/typec/tcpm/tcpm.c and few other drivers.

Ok.

> > +	switch (cmd) {
> > +	case ANX7688_OCM_MSG_PWR_SRC_CAP:
> > +		dev_info(anx7688->dev, "received SRC_CAP\n");
> 
> Noise.

Ok, let me convert these to dev_dbg.

> > +
> > +		if (len % 4 != 0) {
> > +			dev_warn(anx7688->dev, "received invalid sized PDO array\n");
> > +			break;
> > +		}
> > +
> > +		/* the partner is PD capable */
> > +		anx7688->pd_capable = true;
> > +
> > +		for (i = 0; i < len / 4; i++) {
> > +			pdo = le32_to_cpu(pdos[i]);
> > +
> > +			if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> > +				unsigned int voltage = pdo_fixed_voltage(pdo);
> > +				unsigned int max_curr = pdo_max_current(pdo);
> > +
> > +				dev_info(anx7688->dev, "SRC_CAP PDO_FIXED (%umV %umA)\n", voltage, max_curr);
> 
> Noise.
> 
> > +			} else if (pdo_type(pdo) == PDO_TYPE_BATT) {
> > +				unsigned int min_volt = pdo_min_voltage(pdo);
> > +				unsigned int max_volt = pdo_max_voltage(pdo);
> > +				unsigned int max_pow = pdo_max_power(pdo);
> > +
> > +				dev_info(anx7688->dev, "SRC_CAP PDO_BATT (%umV-%umV %umW)\n", min_volt, max_volt, max_pow);
> 
> Noise. That line also really should be split in two.
> 
> I'm stopping my review here. This driver is too noisy. All dev_info
> calls need to be dropped. If the driver is working correctly then it
> needs to quiet.
> 
> Most of those prints are useful for debugging only, so I think similar
> debugfs log like the one tcpm.c uses could be a good idea for them
> since you already use debugfs in this driver in any case.

Ok, let me convert the non-error ones to dev_dbg() and split the long
lines. Debug needs to be enabled, so it should not bother anyone, and
it is easier than refactoring driver to use debugfs.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)

Powered by blists - more mailing lists