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
next prev parent 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: linkBe 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.