On Wed, 5 Jun 2024 at 20:09, Mark Pearson <mpearson-lenovo@xxxxxxxxx> wrote:
>
> Thanks Dmitry (& Diogo from the other thread)
>
> On Tue, Jun 4, 2024, at 7:45 PM, Dmitry Baryshkov wrote:
> > On Tue, Jun 04, 2024 at 03:40:44PM -0400, Mark Pearson wrote:
> >> On systems where the UCSI PDOs are not supported, the UCSI driver is
> >> giving an error message. This can cause users to believe there is a HW
> >> issue with their system when in fact it is working as designed.
> >>
> >> Downgrade message to dev_info for EOPNOTSUPP condition.
> >>
> >> Tested on Lenovo L14 G5 AMD and confirmed with Lenovo FW team that PDOs
> >> are not supported on this platform.
> >>
> >> Signed-off-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> >> ---
> >> drivers/usb/typec/ucsi/ucsi.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> >> index cb52e7b0a2c5..090be87d5485 100644
> >> --- a/drivers/usb/typec/ucsi/ucsi.c
> >> +++ b/drivers/usb/typec/ucsi/ucsi.c
> >> @@ -632,8 +632,12 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
> >> command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
> >> ret = ucsi_send_command(ucsi, command, pdos + offset,
> >> num_pdos * sizeof(u32));
> >> - if (ret < 0 && ret != -ETIMEDOUT)
> >> - dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
> >> + if (ret < 0 && ret != -ETIMEDOUT) {
> >> + if (ret == -EOPNOTSUPP)
> >> + dev_info(ucsi->dev, "UCSI_GET_PDOS not supported on this hardware\n");
> >
> > Maybe it would be enough to guard GET_PDOS commands with the
> > UCSI_CAP_PDO_DETAILS check? Is it cleared on affected platforms?
> >
>
> I checked on the system I have and the features are 0x84, so the CAP_PDO_DETAILS aren't set.
> I can do a formal patch if the approach is better, I ended up doing:
>
> @@ -645,9 +645,13 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
> static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
> int is_partner, u32 *pdos)
> {
> + struct ucsi *ucsi = con->ucsi;
> u8 num_pdos;
> int ret;
>
> + if (!(ucsi->cap.features & UCSI_CAP_PDO_DETAILS))
> + return 0;
> +
> /* UCSI max payload means only getting at most 4 PDOs at a time */
> ret = ucsi_read_pdos(con, role, is_partner, pdos, 0, UCSI_MAX_PDOS);
>
> And this did indeed squelch the 'error' message.
>
> Couple of notes:
> - I don't know this area very well, so don't know if there are risks of any regressions in other circumstances. I think it's pretty safe, but if any experts have an opinion that would be appreciated.
> - It means that there isn't a log message saying that PDO capabilities are not available. Are there going to be power related tooling that won't work and it would be useful to have that message available?
