Received-SPF: pass (google.com: domain of linux-kernel+bounces-137243-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 9 Apr 2024 18:35:50 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>, 
	phone-devel@vger.kernel.org, kernel list <linux-kernel@vger.kernel.org>, fiona.klute@gmx.de, 
	martijn@brixit.nl, samuel@sholland.org, gregkh@linuxfoundation.org, 
	linux-usb@vger.kernel.org, megi@xff.cz
Subject: Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C
 HDMI bridge
Message-ID: <4q7o5vb26ibkbvqal5nn4kdnc32rrajhtszrf4fnuisnlfcsg5@6322saeu7qoe>
References: <ZhPMHdt6r/4D99Zg@duo.ucw.cz>
 <ZhPM4XU8ttsFftBd@duo.ucw.cz>
 <ZhUQ6kzV5ARlkfPC@kuha.fi.intel.com>
 <ZhUgrNwRYoaV1AIJ@duo.ucw.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZhUgrNwRYoaV1AIJ@duo.ucw.cz>

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@xff.cz>
> > > Co-developed-by: Martijn Braam <martijn@brixit.nl>
> > > Co-developed-by: Samuel Holland <samuel@sholland.org>
> > > Signed-off-by: Pavel Machek <pavel@ucw.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.

> 
> > > +	u32 caps[8];
> > > +


-- 
With best wishes
Dmitry