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, -- heikki
