All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Anurag Kumar Vulisha <anuragku@xilinx.com>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
Date: Mon, 6 Aug 2018 13:50:38 +0300	[thread overview]
Message-ID: <51566ca4-1002-5c62-71d0-9da610bff368@linux.intel.com> (raw)
In-Reply-To: <MWHPR02MB281619CD8CFDA93B9ADB3D89A7510@MWHPR02MB2816.namprd02.prod.outlook.com>

Hi,

Back from vacation

On 20.07.2018 18:39, Anurag Kumar Vulisha wrote:
> 
> Hi Mathias,
> 
> Thanks for providing your comments, please find my comments inline
> 
>>>
>>> -	if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
>>> -		xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>> +	/* Iterate over all parent nodes for finding quirks */
>>> +	for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {
>>
>> Isn't sysdev at this point the topmost device that can have any of those device
>> properties set?
>> We could loop from &pdev->dev up to and including sysdev.
>>
>> It doesn't matter much but maybe avoid walking some extra parents.
>>> 
> I have seen some drivers ( like dwc3 host.c) which create a child dev, which is
> the pdev that is being passed to xhci_plat_probe(). So, sysdev may not be the
> topmost parent. There could be some properties which may be present in parent
> and may not be populated in the child. So, I made this change to search on all
> available parents for finding a valid property

Ok, lets keep it like you wrote it

> 
>>>
>>> -	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>>> -		xhci->quirks |= XHCI_LPM_SUPPORT;
>>> +		if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
>>> +			xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>>
>>> -	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>>> -		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>>> +		if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
>>> +			xhci->quirks |= XHCI_LPM_SUPPORT;
>>>
>>> -	/* imod_interval is the interrupt moderation value in nanoseconds. */
>>> -	xhci->imod_interval = 40000;
>>
>> Setting the default imod_interval could be moved before the for() loop
>>
> 
> I thought it would be better if everything related to imod_interval is in one place,
> so kept it as is. I can fix this in v2 if you suggest.

Default imod_interval needs to be set before the for loop to avoid overwriting a value
got from device property

So a v2 is needed

-Mathias

WARNING: multiple messages have this Message-ID (diff)
From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Anurag Kumar Vulisha <anuragku@xilinx.com>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: usb: host: xhci-plat: Iterate over parent nodes for finding quirks
Date: Mon, 6 Aug 2018 13:50:38 +0300	[thread overview]
Message-ID: <51566ca4-1002-5c62-71d0-9da610bff368@linux.intel.com> (raw)

Hi,

Back from vacation

On 20.07.2018 18:39, Anurag Kumar Vulisha wrote:
> 
> Hi Mathias,
> 
> Thanks for providing your comments, please find my comments inline
> 
>>>
>>> -	if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
>>> -		xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>> +	/* Iterate over all parent nodes for finding quirks */
>>> +	for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {
>>
>> Isn't sysdev at this point the topmost device that can have any of those device
>> properties set?
>> We could loop from &pdev->dev up to and including sysdev.
>>
>> It doesn't matter much but maybe avoid walking some extra parents.
>>> 
> I have seen some drivers ( like dwc3 host.c) which create a child dev, which is
> the pdev that is being passed to xhci_plat_probe(). So, sysdev may not be the
> topmost parent. There could be some properties which may be present in parent
> and may not be populated in the child. So, I made this change to search on all
> available parents for finding a valid property

Ok, lets keep it like you wrote it

> 
>>>
>>> -	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>>> -		xhci->quirks |= XHCI_LPM_SUPPORT;
>>> +		if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
>>> +			xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>>
>>> -	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>>> -		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>>> +		if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
>>> +			xhci->quirks |= XHCI_LPM_SUPPORT;
>>>
>>> -	/* imod_interval is the interrupt moderation value in nanoseconds. */
>>> -	xhci->imod_interval = 40000;
>>
>> Setting the default imod_interval could be moved before the for() loop
>>
> 
> I thought it would be better if everything related to imod_interval is in one place,
> so kept it as is. I can fix this in v2 if you suggest.

Default imod_interval needs to be set before the for loop to avoid overwriting a value
got from device property

So a v2 is needed

-Mathias
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2018-08-06 10:47 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-05 15:20 [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks Anurag Kumar Vulisha
2018-06-05 15:20 ` Anurag Kumar Vulisha
2018-06-05 15:25 ` [PATCH] " Greg Kroah-Hartman
2018-06-05 15:25   ` Greg Kroah-Hartman
2018-06-05 18:41   ` [PATCH] " Anurag Kumar Vulisha
2018-06-05 18:41     ` Anurag Kumar Vulisha
2018-06-12  7:03     ` [PATCH] " Anurag Kumar Vulisha
2018-06-12  7:03       ` Anurag Kumar Vulisha
2018-06-12  8:02       ` [PATCH] " Greg Kroah-Hartman
2018-06-12  8:02         ` Greg Kroah-Hartman
2018-07-14 15:37         ` [PATCH] " Anurag Kumar Vulisha
2018-07-14 15:37           ` Anurag Kumar Vulisha
2018-07-15  8:02           ` [PATCH] " Greg Kroah-Hartman
2018-07-15  8:02             ` Greg Kroah-Hartman
2018-07-15  8:11             ` [PATCH] " Greg Kroah-Hartman
2018-07-15  8:11               ` Greg Kroah-Hartman
2018-07-20 14:53               ` [PATCH] " Mathias Nyman
2018-07-20 14:53                 ` Mathias Nyman
2018-08-06  7:58                 ` [PATCH] " Anurag Kumar Vulisha
2018-08-06  7:58                   ` Anurag Kumar Vulisha
2018-08-06 11:11                   ` [PATCH] " Mathias Nyman
2018-08-06 11:11                     ` Mathias Nyman
2018-08-06 11:41                     ` [PATCH] " Anurag Kumar Vulisha
2018-08-06 11:41                       ` Anurag Kumar Vulisha
2018-07-20 14:42 ` [PATCH] " Mathias Nyman
2018-07-20 14:42   ` Mathias Nyman
2018-07-20 15:39   ` [PATCH] " Anurag Kumar Vulisha
2018-07-20 15:39     ` Anurag Kumar Vulisha
2018-08-06 10:50     ` Mathias Nyman [this message]
2018-08-06 10:50       ` Mathias Nyman
2018-08-06 11:14       ` [PATCH] " Anurag Kumar Vulisha
2018-08-06 11:14         ` Anurag Kumar Vulisha

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51566ca4-1002-5c62-71d0-9da610bff368@linux.intel.com \
    --to=mathias.nyman@linux.intel.com \
    --cc=anuragku@xilinx.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.