From mboxrd@z Thu Jan 1 00:00:00 1970 From: andrew.smirnov@gmail.com (Andrey Smirnov) Date: Mon, 26 Nov 2018 10:24:31 -0800 Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ In-Reply-To: References: <20181117181225.10737-1-andrew.smirnov@gmail.com> <20181117181225.10737-4-andrew.smirnov@gmail.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez wrote: > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote: > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > - case IMX7D: > > + case IMX8MQ: > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x", > > + &imx6_pcie->gpr1x)) { > > + dev_err(dev, "Failed to get GPR1x address\n"); > > + return -EINVAL; > > + } > > This is for distinguishing multiple controllers on the SOC but other > registers and bits might differ. Isn't it preferable to have a property > for controller id instead of adding many registers to DT? > I liked encoding necessary info in DT directly slightly better than encoding abstract ID and then decoding it further in the driver code. OTOH, I am not really attached to that path. Lucas, can you comment on this please? > > + > > + if (of_property_read_u32_array( > > + node, "fsl,gpr12-device-type", > > + imx6_pcie->device_type, > > + ARRAY_SIZE(imx6_pcie->device_type))) { > > + dev_err(dev, "Failed to get device type > > mask/value\n"); > > + return -EINVAL; > > + } > > The device type can be set on multiple SOCs, why are you adding a > mandatory property only for 8m? My thinking was that other SoCs don't really have two controllers, so they don't really need to have that property, but, more importantly, not forcing them to have this property should preserve backwards compatibility with old DTBs. > > There should probably be a separate patch with documented DT bindings. > Yes, definitely, I just wanted to come up with a set of bindings agreed on by the driver maintainers first. Thanks, Andrey Smirnov