On Wed, Jul 31, 2024 at 05:51:17PM GMT, Elson Serrao wrote:
>
>
> On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote:
> > On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
> >> Since EUD is physically present between the USB connector and
> >> the USB controller, it should relay the usb role notifications
> >> from the connector. Hence register a role switch handler to
> >> process and relay these roles to the USB controller. This results
> >> in a common framework to send both connector related events
> >> and eud attach/detach events to the USB controller.
> >>
> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@xxxxxxxxxxx>
> >> ---
> >> drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
> >> 1 file changed, 69 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> >> index 3de7d465912c..9a49c934e8cf 100644
> >> --- a/drivers/usb/misc/qcom_eud.c
> >> +++ b/drivers/usb/misc/qcom_eud.c
> >> @@ -10,6 +10,7 @@
> >> #include <linux/iopoll.h>
> >> #include <linux/kernel.h>
> >> #include <linux/module.h>
> >> +#include <linux/mutex.h>
> >> #include <linux/of.h>
> >> #include <linux/phy/phy.h>
> >> #include <linux/platform_device.h>
> >> @@ -35,12 +36,16 @@ struct eud_chip {
> >> struct device *dev;
> >> struct usb_role_switch *role_sw;
> >> struct phy *usb2_phy;
> >> +
> >> + /* mode lock */
> >> + struct mutex mutex;
> >> void __iomem *base;
> >> void __iomem *mode_mgr;
> >> unsigned int int_status;
> >> int irq;
> >> bool enabled;
> >> bool usb_attached;
> >> + enum usb_role current_role;
> >> };
> >>
> >> static int eud_phy_enable(struct eud_chip *chip)
> >> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
> >> phy_exit(chip->usb2_phy);
> >> }
> >>
> >> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
> >> +{
> >> + struct usb_role_switch *sw;
> >> + int ret = 0;
> >> +
> >> + mutex_lock(&chip->mutex);
> >> +
> >> + /* Avoid duplicate role handling */
> >> + if (role == chip->current_role)
> >> + goto err;
> >> +
> >> + sw = usb_role_switch_get(chip->dev);
> >
> > Why isn't chip->role_sw good enough? Why do you need to get it each
> > time?
> >
>
> Hi Dmitry
>
> chip->role_sw is the eud role switch handler to receive role switch notifications from the
> USB connector. The 'sw' I am getting above is the role switch handler of the USB controller.
> As per this design, EUD receives role switch notification from the connector
> (via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller.
The fact that you have repurposed existing structure field is not a
waiver for the inefficiency.
So what about keeping chip->role_sw as is and adding _new_ field for the
self-provided role switch?
--
With best wishes
Dmitry
