linux-kernel - Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2panrf3dgpwkwywf4vv676tjlbdqpzjb75vpfhiohabhrxc6h2@tmouy7prgikm>
Date: Wed, 10 Apr 2024 13:32:26 +0200
From: Ondřej Jirman <megi@....cz>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Pavel Machek <pavel@....cz>, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 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
Subject: Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C
 HDMI bridge

On Tue, Apr 09, 2024 at 06:35:50PM GMT, Dmitry Baryshkov wrote:
> On Tue, Apr 09, 2024 at 01:04:12PM +0200, Pavel Machek wrote:
> > 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.
> > 
> 
> [skipped]
> 
> > 
> > > > +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)?
> 
> From Documentation/devicetree/bindings/connector/usb-connector.yaml:
> 
>             altmodes {
>                 displayport {
>                     svid = /bits/ 16 <0xff01>;
>                     vdo = <0x00001c46>;
>                 };
>             };
> 
> BTW, I don't see the VDO for the DP altmode in your code. Maybe I missed
> it at a quick glance.

VDO is set via TYPE_DP_SNK_CFG message to the firmware. There may be some
default in the firmware which matches Pinephone receptacle configuration.

I guess the driver can send the VDO value from DT after firmware is
initialized. Other values can be set in DT too, but extreme care needs to
ne take, because firmware has some bugs, which cause it to request
high voltage from PD PSU when it's in fact not part of PDO, potentially
destroying the device. So I'd rather not expose at least PDOs in DT to random
unsuspecting DT users who just copy paste and edit DT without reading the driver
code or some obscure notes somewhere.

kind regards,
	o.

> > 
> > > > +	u32 caps[8];
> > > > +
> 
> 
> -- 
> With best wishes
> Dmitry