Message-ID: <ZhUgrNwRYoaV1AIJ@duo.ucw.cz>
Date: Tue, 9 Apr 2024 13:04:12 +0200
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: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C
 HDMI bridge

Hi!

> > This is driver for ANX7688 USB-C HDMI, with flashing and debugging
> > features removed. ANX7688 is rather criticial piece on PinePhone,
> > there's no display and no battery charging without it.
> > 
> > There's likely more work to be done here, but having basic support
> > in mainline is needed to be able to work on the other stuff
> > (networking, cameras, power management).
> > 
> > Signed-off-by: Ondrej Jirman <megi@....cz>
> > Co-developed-by: Martijn Braam <martijn@...xit.nl>
> > Co-developed-by: Samuel Holland <samuel@...lland.org>
> > Signed-off-by: Pavel Machek <pavel@....cz>
> 
> Just couple of quick comments below - I did not have time to go over
> this very thoroughly, but I think you need to make a new version in
> any case because of comments in 1/2.

Yes, there will be new version.

There is ton of sleep primitives, and existing ones are okay. I can
change everything to fdelay or whatever primitive-of-the-day is, but
I'd rather not do pointless changes.

You can ask for major changes, or complain about extra newlines, but
doing both in the same email does not make sense.

> Btw, Co-developed-by comes before Signed-off-by I think.

I believe this order is fine, too.

> > +++ b/drivers/usb/typec/anx7688.c
> > @@ -0,0 +1,1830 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ANX7688 USB-C HDMI bridge/PD driver
> > + *
> > + * Warning, this driver is somewhat PinePhone specific.
> 
> So why not split this into core part and PinePhone specific glue
> part?

Potentially a lot of work I can't really test and would rather not do.

> > +	struct delayed_work work;
> > +	struct timer_list work_timer;
> > +
> > +	struct mutex lock;
> 
> Undocumented lock.

This is simple driver. How do you expect me to document it? Protects
this data structure, not exactly uncommon.

> > +
> > +	/* wait for power to stabilize and release reset */
> > +	msleep(10);
> > +	gpiod_set_value(anx7688->gpio_reset, 0);
> > +	usleep_range(2, 4);
> 
> Why not just use usleep_range() in both cases.

It does not really make code any better. Can do if you insist.

> > +static int anx7688_connect(struct anx7688 *anx7688)
> > +{
> > +	struct typec_partner_desc desc = {};
> > +	int ret, i;
> > +	u8 fw[2];
> > +	const u8 dp_snk_identity[16] = {
> > +		0x00, 0x00, 0x00, 0xec,	/* id header */
> > +		0x00, 0x00, 0x00, 0x00,	/* cert stat */
> > +		0x00, 0x00, 0x00, 0x00,	/* product type */
> > +		0x39, 0x00, 0x00, 0x51	/* alt mode adapter */
> > +	};
> > +	const u8 svid[4] = {
> > +		0x00, 0x00, 0x01, 0xff,
> > +	};
> 
> Why not get those from DT?

Are you sure it belongs to the DT (and that DT people will agree)?

> > +	u32 caps[8];
> > +
> > +	dev_dbg(anx7688->dev, "cable inserted\n");
> > +
> > +	anx7688->last_status = -1;
> > +	anx7688->last_cc_status = -1;
> > +	anx7688->last_dp_state = -1;
> > +
> > +	msleep(10);
> 
> Please make a comment here why you have to wait, and use
> usleep_range().

/* Dunno because working code does that and waiting for hardware to
settle down after cable insertion kind of looks like a good thing */

I did not write the driver, and there's no good documentation for this
chip. I can try to invent something, but...

> > +	i = 0;
> > +	while (1) {
> > +		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_dbg(anx7688->dev, "fw loaded after %d ms\n", i * 10);
> > +			break;
> > +		}
> > +
> > +		if (i > 99) {
> > +			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;
> > +		}
> > +		msleep(5);
> > +		i++;
> > +	}
> 
> You need to use something like time_is_after_jiffies() here instead of
> a counter.

Well, this works as well, but yes, I agree here.

> > +	ret = i2c_smbus_read_i2c_block_data(anx7688->client,
> > +					    ANX7688_REG_FW_VERSION1, 2, fw);
> > +	if (ret < 0) {
> > +		dev_err(anx7688->dev, "failed to read firmware version\n");
> > +		goto err_vconoff;
> > +	}
> > +
> > +	dev_dbg(anx7688->dev, "OCM firmware loaded (version 0x%04x)\n",
> > +		 fw[1] | fw[0] << 8);
> > +
> > +	/* Unmask interrupts */
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0);
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT_MASK, ~ANX7688_SOFT_INT_MASK);
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, 0xff);
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_MASK2, (u8)~ANX7688_IRQ2_SOFT_INT);
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	/* time to turn off vbus after cc disconnect (unit is 4 ms) */
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_VBUS_OFF_DELAY_TIME, 100 / 4);
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	/* 300ms (unit is 2 ms) */
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_TRY_UFP_TIMER, 300 / 2);
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	/* maximum voltage in 100 mV units */
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_VOLTAGE, 50); /* 5 V */
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	/* min/max power in 500 mW units */
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_POWER, 15 * 2); /* 15 W */
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_MIN_POWER, 1);  /* 0.5 W */
> > +	if (ret)
> > +		goto err_vconoff;
> > +
> > +	/* auto_pd, try.src, try.sink, goto safe 5V */
> > +	ret = anx7688_reg_write(anx7688, ANX7688_REG_FEATURE_CTRL, 0x1e & ~BIT(2)); // disable try_src
> 
> Those two comments are obscure.

I hoped they make sense to someone familiar with the area. Can't do
much better than remove them.

> This function seems to have lot of hard coded information above.
> Shouldn't much of that come from DT?

You tell me, I suppose you seen some similar drivers.

> Please note that if you have that PinePhone specific glue driver, you
> can get much of the hard coded information from there, if getting the
> information from DT is not an option for what ever reason.

Thanks for review.

Could you trim the parts of email you are not replying to?

Do you see any other major problems?

Do you have any idea if this chip is used elsewhere? I do not have any
other hardware with anx7688, so I won't be able to test it elsewhere,
and if there are no other users, having specific driver should not be
a problem for anyone. If there's other user, well, there's chance they
have docs and can help.

How would you envision the split? Do you have any other driver that
could be used as an example? Is someone else putting them in the
device tree?

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