Re: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux