Received-SPF: pass (google.com: domain of linux-kernel+bounces-136410-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 9 Apr 2024 11:05:09 +0300
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/5] usb: typec: ucsi: make it orientation-aware
Message-ID: <ZhT2tUFHinglhQu5@kuha.fi.intel.com>
References: <20240408-ucsi-orient-aware-v1-0-95a74a163a10@linaro.org>
 <20240408-ucsi-orient-aware-v1-4-95a74a163a10@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240408-ucsi-orient-aware-v1-4-95a74a163a10@linaro.org>

Hi Dmitry,

On Mon, Apr 08, 2024 at 07:30:52AM +0300, Dmitry Baryshkov wrote:
> The UCSI 1.0 is not orientation aware. Only UCSI 2.0 has added
> orientation status to GET_CONNECTOR_STATUS data. However the glue code
> can be able to detect cable orientation on its own (and report it via
> corresponding typec API). Add a flag to let UCSI mark registered ports
> as orientation aware.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 ++
>  drivers/usb/typec/ucsi/ucsi.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 7ad544c968e4..6f5adc335980 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1551,6 +1551,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
>  	cap->svdm_version = SVDM_VER_2_0;
>  	cap->prefer_role = TYPEC_NO_PREFERRED_ROLE;
>  
> +	cap->orientation_aware = !!(ucsi->quirks & UCSI_ORIENTATION_AWARE);
> +
>  	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY)
>  		*accessory++ = TYPEC_ACCESSORY_AUDIO;
>  	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 6599fbd09bee..e92be45e4c1c 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -410,6 +410,7 @@ struct ucsi {
>  	unsigned long quirks;
>  #define UCSI_NO_PARTNER_PDOS	BIT(0)	/* Don't read partner's PDOs */
>  #define UCSI_DELAY_DEVICE_PDOS	BIT(1)	/* Reading PDOs fails until the parter is in PD mode */
> +#define UCSI_ORIENTATION_AWARE	BIT(2)	/* UCSI is orientation aware */

You are not using that flag anywhere in this series. But why would
orientation need a "quirk" in the first place?

I'm not sure where you are going with this, but please try to avoid
the quirk flags in general in this driver rather than considering them
as the first way of solving things. Use them only as the last resort.

I don't want this driver to end up like xhci and some other drivers,
where refactoring is almost impossible because every place is full of
conditions checking the quirks, and where in worst case a quirk meant
to solve a problem on one hardware causes problems on another.

thanks,

-- 
heikki