RE: [PATCH 1/4] usb: common: Consider only available nodes for dr_mode

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

 



Hi Fabrizio-san,

> From: Fabrizio Castro, Sent: Tuesday, April 2, 2019 6:22 PM
> 
> Hi Yoshihiro-san,
> 
> Thank you for your feedback.
> 
> > From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > Sent: 02 April 2019 02:54
> > Subject: RE: [PATCH 1/4] usb: common: Consider only available nodes for dr_mode
> >
> > Hi Fabrizio-san,
> >
> > Thank you for the patch!
> >
> > > From: Fabrizio Castro, Sent: Friday, March 1, 2019 8:06 PM
> > >
> > > There are cases where multiple device tree nodes point to the
> > > same phy node by means of the "phys" property, but we should
> > > only consider those nodes that are marked as available rather
> > > than just any node.
> > >
> > > Fixes: 98bfb3946695 ("usb: of: add an api to get dr_mode by the phy node")
> > > Cc: stable@xxxxxxxxxxxxxxx # v4.4+
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > > ---
> >
> > I'm guessing this code needs for phy-rcar-gen3-usb2.c only because
> > the phy driver only gets the dr_mode from index 0 like below:
> >
> > 	channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0);
> >
> > Yesterday, I submitted patches to get multiple indexes from controller
> > device nodes:
> >
> > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=99561
> >
> > So, would you check the phy patches can get the dr_mode without this changing common patch?
> 
> I will go through your patch series, but I can't see why any driver would be interested
> in considering a disabled node for getting dr_mode, can you?
> Do you have a use case for this?

This is not a use case though, the commit 98bfb3946695 is made by a TI engineer so that
I checked TI's dts/dtsi file on armv7 arch (am335x-boneblue.dts) a little and then
it has one usb controller node only. So, the commit didn't take care of a disabled node, I guess.
However, as you know, on R-Car Gen3 and RZ/G2, they have multiple controller nodes. So,
one of the controller can be disabled.

By the way, if we applied this patch, since the behavior will be changed at least,
so all of this api user have to test whether this code doesn't break or not.

Best regards,
Yoshihiro Shimoda

> Thanks,
> Fab
> 
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > >  drivers/usb/common/common.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > > index 48277bb..73c8e65 100644
> > > --- a/drivers/usb/common/common.c
> > > +++ b/drivers/usb/common/common.c
> > > @@ -145,6 +145,8 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0)
> > >
> > >  	do {
> > >  		controller = of_find_node_with_property(controller, "phys");
> > > +		if (!of_device_is_available(controller))
> > > +			continue;
> > >  		index = 0;
> > >  		do {
> > >  			if (arg0 == -1) {
> > > --
> > > 2.7.4





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux