Hello Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Sent: Friday, November 8, 2024 9:08 AM > To: Facklam, Olivér <oliver.facklam@xxxxxxxxxxx> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux- > usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; von Heyl, Benedict > <benedict.vonheyl@xxxxxxxxxxx>; Först, Mathis > <mathis.foerst@xxxxxxxxxxx>; Glettig, Michael > <Michael.Glettig@xxxxxxxxxxx> > Subject: Re: [PATCH 2/4] usb: typec: hd3ss3220: use typec_get_fw_cap() to fill > typec_cap > > On Thu, Nov 07, 2024 at 03:24:44PM +0000, Facklam, Olivér wrote: > > Hello Heikki, > > > > > -----Original Message----- > > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > > Sent: Thursday, November 7, 2024 3:21 PM > > > To: Facklam, Olivér <oliver.facklam@xxxxxxxxxxx> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux- > > > usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; von Heyl, > > > Benedict <benedict.vonheyl@xxxxxxxxxxx>; Först, Mathis > > > <mathis.foerst@xxxxxxxxxxx>; Glettig, Michael > > > <Michael.Glettig@xxxxxxxxxxx> > > > Subject: Re: [PATCH 2/4] usb: typec: hd3ss3220: use > > > typec_get_fw_cap() to fill typec_cap > > > > > > Hi Oliver, > > > > > > On Thu, Nov 07, 2024 at 12:43:28PM +0100, Oliver Facklam via B4 > > > Relay > > > wrote: > > > > From: Oliver Facklam <oliver.facklam@xxxxxxxxxxx> > > > > > > > > The type, data, and prefer_role properties were previously > > > > hard-coded when creating the struct typec_capability. > > > > > > > > Use typec_get_fw_cap() to populate these fields based on the > > > > respective fwnode properties. > > > > > > > > Signed-off-by: Oliver Facklam <oliver.facklam@xxxxxxxxxxx> > > > > --- > > > > drivers/usb/typec/hd3ss3220.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/usb/typec/hd3ss3220.c > > > > b/drivers/usb/typec/hd3ss3220.c index > > > > > > > > 56f74bf70895ca701083bde44a5bbe0b691551e1..e6e4b1871b5d805f8c3671 > > > 31509f > > > > 4e6ec0d2b5f0 100644 > > > > --- a/drivers/usb/typec/hd3ss3220.c > > > > +++ b/drivers/usb/typec/hd3ss3220.c > > > > @@ -259,12 +259,12 @@ static int hd3ss3220_probe(struct i2c_client > > > *client) > > > > goto err_put_fwnode; > > > > } > > > > > > > > - typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE; > > > > + ret = typec_get_fw_cap(&typec_cap, connector); > > > > + if (ret) > > > > + goto err_put_role; > > > > > > You are not leaving any fallback here. Are you sure all the existing > > > systems supply those properties? > > > > > > There is another problem. At least "data-role" property is optional, > > > but that will in practice make it a requirement. > > > > I missed that typec_get_fw_cap() was not setting a default if the property is > absent. > > > > > > > > I think it would be safer to use the values from the device > > > properties only if they are available. Otherwise simply use default values. > > > > I'll add back the previous values as defaults before calling > typec_get_fw_cap(). > > That will make data-role and try-power-role optional again, as intended. > > > > However, "power-role" seems to be required by the function. Is this > expected? > > Yes. > > > Or should I write my own fwnode parsing logic? > > No, don't do that. > > You can check the return value. If it's -EINVAL or -ENXIO, the properties are > not there, and you can safely use the defaults. > Otherwise, consider the failure as critical. Thanks for the hint, will do. > > thanks, > > -- > heikki
