From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH v2 3/7] of/irq: Break out msi-map lookup (again) Date: Wed, 15 Jun 2016 11:16:16 +0100 Message-ID: <57612AF0.7020208@arm.com> References: <2cbffa9a341edfd10114994f6486f6b08d0c390c.1464966939.git.robin.murphy@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Rob Herring Cc: Joerg Roedel , Will Deacon , Linux IOMMU , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Frank Rowand , Marc Zyngier List-Id: devicetree@vger.kernel.org On 14/06/16 18:01, Rob Herring wrote: > On Fri, Jun 3, 2016 at 12:15 PM, Robin Murphy wrote: >> The PCI msi-map code is already doing double-duty translating IDs and >> retrieving MSI parents, which unsurprisingly is the same functionality >> we need for the identically-formatted PCI iommu-map property. Drag the >> core parsing routine up yet another layer into the general OF-PCI code, >> and further generalise it for either kind of lookup in either flavour >> of map property. >> >> CC: Rob Herring >> CC: Frank Rowand >> CC: Marc Zyngier >> Signed-off-by: Robin Murphy >> --- >> >> v2: No change. >> >> drivers/of/irq.c | 70 ++------------------------------- >> drivers/of/of_pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of_pci.h | 8 ++++ >> 3 files changed, 114 insertions(+), 66 deletions(-) >> >> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >> index e7bfc175b8e1..0c9118d849ee 100644 >> --- a/drivers/of/irq.c >> +++ b/drivers/of/irq.c >> @@ -24,6 +24,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> >> @@ -586,13 +587,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np, >> u32 rid_in) >> { >> struct device *parent_dev; >> - struct device_node *msi_controller_node; >> - struct device_node *msi_np = *np; >> - u32 map_mask, masked_rid, rid_base, msi_base, rid_len, phandle; >> - int msi_map_len; >> - bool matched; >> u32 rid_out = rid_in; >> - const __be32 *msi_map = NULL; >> >> /* >> * Walk up the device parent links looking for one with a >> @@ -602,71 +597,14 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np, >> if (!parent_dev->of_node) >> continue; >> >> - msi_map = of_get_property(parent_dev->of_node, >> - "msi-map", &msi_map_len); >> - if (!msi_map) >> + if (!of_property_read_bool(parent_dev->of_node, "msi-map")) > > But msi-map is not bool, right? I think we allow bools to have values > for historical reasons, but really bool with a value should be an > error. So don't rely on current behavior. Right, I now have no idea why that wasn't of_find_property() in the first place... >> continue; >> >> - if (msi_map_len % (4 * sizeof(__be32))) { >> - dev_err(parent_dev, "Error: Bad msi-map length: %d\n", >> - msi_map_len); >> - return rid_out; >> - } >> /* We have a good parent_dev and msi_map, let's use them. */ >> + of_pci_map_rid(parent_dev->of_node, "msi-map", rid_in, np, >> + &rid_out); > > Seems like this could return an error code and then you could continue > based on the return code. Then you wouldn't be looking up msi-map > twice. Probably could get rid of the !parent_dev->of_node check too. ...but I agree that approach does sound better overall, so I'll take a closer look at this whole patch again. Thanks, Robin. > > Rob > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html