RE: [PATCH V3 2/4] ARM: tegra: pcie: Add tegra3 support

[Jay Agarwal]

> > 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().
Yes you are right, it is not required. I will revert it along with taking care of other comments.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/