lkml.org 
[lkml]   [2013]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V3 2/4] ARM: tegra: pcie: Add tegra3 support
    On 06/04/2013 12:57 PM, Jay Agarwal wrote:
    > - Enable PCIe root port 2 for Cardhu
    > - Make private data structure for each SoC
    > - Add required Tegra30 clocks and regulators
    > - Add Tegra30 specific code in enable controller
    > - Corrected logic in read/write config space to
    > display right device number on bus 0
    > - Fixed unnecessary freeing of tegra_pcie_bus structure in tegra_pcie_remove
    > - Added Tegra30 specific properties in pci binding document

    > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

    > +- avdd-supply: Power supply for controller (1.05V)

    > + "cml": The Tegra clock of that name

    Both those changes need to mention that those additions are only
    required for Tegra30, not Tegra20. In other words,

    +- avdd-supply: Power supply for controller (1.05V) (not required for
    Tegra20)

    + "cml": The Tegra clock of that name (not required for Tegra20)

    You probably also want to explicitly mention nvidia,tegra30-pcie as a
    legal compatible value.

    > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

    > +/* used to differentiate tegra chips code */
    > +struct tegra_pcie_soc_data {
    > + unsigned int num_max_ports;

    nit: "num max" seems redundant. max_ports or num_ports would be better.

    > struct tegra_pcie_port {
    > @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
    > struct tegra_pcie_port *port;
    >
    > list_for_each_entry(port, &pcie->ports, list) {
    > - if (port->index + 1 == slot) {
    > + if (port->index == slot) {

    This and the equivalent change in tegra_pcie_write_conf() seem like a
    bug-fix unrelated to the addition of Tegra30 support. Hence, they should
    be a separate patch.


    > @@ -398,7 +422,6 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
    > *value = 0xffffffff;
    > return PCIBIOS_DEVICE_NOT_FOUND;
    > }
    > -
    > addr += tegra_pcie_conf_offset(devfn, where);

    Unnecessary white-space change.

    > @@ -1549,10 +1660,9 @@ static int tegra_pcie_remove(struct platform_device *pdev)
    > struct tegra_pcie_bus *bus;
    > int err;
    >
    > - list_for_each_entry(bus, &pcie->busses, list) {
    > + list_for_each_entry(bus, &pcie->busses, list)
    > vunmap(bus->area->addr);
    > - kfree(bus);
    > - }
    > + kfree(bus);

    This doesn't look right. Can you please explain it further? This is
    looping over every bus in a dynamic list, so surely each entry needs to
    be freed? Did you do this to avoid a crash? If so, the issue is likely
    that the loop should use list_for_each_entry_safe() rather than
    list_for_each_entry().


    \
     
     \ /
      Last update: 2013-06-04 21:41    [W:3.996 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site