On Mon, Dec 16, 2019 at 10:49:46PM +0000, Ajay Gupta wrote:
> Hi Heikki,
>
> > -----Original Message-----
> > From: linux-usb-owner@xxxxxxxxxxxxxxx <linux-usb-owner@xxxxxxxxxxxxxxx>
> > On Behalf Of Heikki Krogerus
> > Sent: Friday, December 13, 2019 4:38 AM
> > To: Ajay Gupta <ajayg@xxxxxxxxxx>
> > Cc: Ajay Gupta <ajaykuee@xxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v6] usb: typec: ucsi: add support for separate DP altmode
> > devices
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, Dec 12, 2019 at 05:42:28PM +0000, Ajay Gupta wrote:
> > > Hi Heikki,
> > >
> > > > -----Original Message-----
> > > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > Sent: Thursday, December 12, 2019 5:44 AM
> > > > To: Ajay Gupta <ajaykuee@xxxxxxxxx>
> > > > Cc: linux-usb@xxxxxxxxxxxxxxx; Ajay Gupta <ajayg@xxxxxxxxxx>
> > > > Subject: Re: [PATCH v6] usb: typec: ucsi: add support for separate
> > > > DP altmode devices
> > > >
> > > >
> > > > Hi Ajay,
> > > >
> > > > On Fri, Nov 22, 2019 at 04:43:47PM -0800, Ajay Gupta wrote:
> > > > > From: Ajay Gupta <ajayg@xxxxxxxxxx>
> > > > >
> > > > > CCGx controller used on NVIDIA GPU card has two separate display
> > > > > altmode for two DP pin assignments. UCSI specification doesn't
> > > > > prohibits using separate display altmode.
> > > > >
> > > > > Current UCSI Type-C framework expects only one display altmode for
> > > > > all DP pin assignment. This patch squashes two separate display
> > > > > altmode into single altmode to support controllers with separate
> > > > > display altmode. We first read all the alternate modes of
> > > > > connector and then run through it to know if there are separate
> > > > > display altmodes. If so, it prepares a new port altmode set after
> > > > > squashing two or more separate altmodes into one.
> > > >
> > > > I didn't see any major issues with this. There were still few extra
> > > > spaces etc., but I can clean those. Maybe it would have been good to
> > > > mention in the commit message that the reason why we need those two
> > > > separate alt modes, for what is in reality two separate pin
> > > > configurations, is limitations in UCSI specification, but never mind.
> > > >
> > > > I still don't like the approach, but since I'm unable to explain my
> > > > idea, or have time to write something for this myself, I don't want
> > > > block this any longer. It does not add that much code, so once I
> > > > have time, I can always try to improve it myself, right?
> > > >
> > > > Otherwise it's OK by me. I'll queue it up.
> > > Thanks for reviewing. Please feel free to improve the patch or commit
> > message.
> >
> > One thing that I do want to do is I want to isolate the changes done to ucsi.c.
> > Can you test the attached diff? It applies on top of this patch.
> Tested the diff and it looks good. Please add additional change at [A] on top of
> your diff..
Done.
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -368,11 +368,8 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
> if (!len)
> break;
>
> - if (!alt.svid) {
> - /* break out of outer loop and register */
> - i = max_altmodes;
> + if (!alt.svid)
> break;
> - }
>
> orig[k].mid = alt.mid;
> orig[k].svid = alt.svid;
> @@ -382,7 +379,7 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
> * Update the original altmode table as some ppms may report
> * multiple DP altmodes.
> */
> - if (recipient == UCSI_RECIPIENT_CON && ucsi->ops->update_altmodes)
> + if (recipient == UCSI_RECIPIENT_CON)
> multi_dp = ucsi->ops->update_altmodes(ucsi, orig, updated);
>
> /* now register altmodes */
thanks,
--
heikki
