From: Robin Murphy <robin.murphy@arm.com> To: Joerg Roedel <joro@8bytes.org> Cc: Will Deacon <will.deacon@arm.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Alex Williamson <alex.williamson@redhat.com>, David Woodhouse <dwmw2@infradead.org>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Joerg Roedel <jroedel@suse.de> Subject: Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface Date: Fri, 10 Feb 2017 16:03:07 +0000 [thread overview] Message-ID: <f770249b-ad98-10d8-ea79-dc10cef7f4df@arm.com> (raw) In-Reply-To: <20170210152254.GI7339@8bytes.org> On 10/02/17 15:22, Joerg Roedel wrote: > Hi Robin, > > On Fri, Feb 10, 2017 at 02:16:54PM +0000, Robin Murphy wrote: >>> +static inline void iommu_device_set_fwnode(struct iommu_device *iommu, >>> + struct fwnode_handle *fwnode) >>> +{ >>> + iommu->fwnode = fwnode; >>> +} >> >> Would it make sense to simply make the ops and fwnode additional >> arguments to iommu_device_register() (permitting fwnode to be NULL)? >> AFAICS they should typically all have the same effective lifetime so >> there doesn't seem to be any real need to handle everything separately. > > Well, it is not yet clear what other information will end up in > 'struct iommu_device', and I don't want to add another parameter to > iommu_device_register for every new struct member. That's a fair point. I think the ops, as a core piece of the whole API, would be sufficiently self-explanatory as part of registration, but then we'd end up with a weird interface with different members initialised through different paths, and I agree that ends up just as ugly. > Also I think having these wrappers is more readable in the code, as it > is clear what the code does without looking up the function prototypes > in the header. Yeah, on reflection explicit initialisation is certainly easier to read than a bunch of arguments handled implicitly by register(), but then from that angle, even more clear would be to simply have the drivers write the relevant struct members directly - I'd be quite happy with that, and we then don't have to add another setter to iommu.h for every new struct member (and risk it looking like Java code...) Robin. > > It might make sense to set the mandatory struct members via > iommu_device_register in the future, but we'll see :) > > > Joerg >
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> To: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>, David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> Subject: Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface Date: Fri, 10 Feb 2017 16:03:07 +0000 [thread overview] Message-ID: <f770249b-ad98-10d8-ea79-dc10cef7f4df@arm.com> (raw) In-Reply-To: <20170210152254.GI7339-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> On 10/02/17 15:22, Joerg Roedel wrote: > Hi Robin, > > On Fri, Feb 10, 2017 at 02:16:54PM +0000, Robin Murphy wrote: >>> +static inline void iommu_device_set_fwnode(struct iommu_device *iommu, >>> + struct fwnode_handle *fwnode) >>> +{ >>> + iommu->fwnode = fwnode; >>> +} >> >> Would it make sense to simply make the ops and fwnode additional >> arguments to iommu_device_register() (permitting fwnode to be NULL)? >> AFAICS they should typically all have the same effective lifetime so >> there doesn't seem to be any real need to handle everything separately. > > Well, it is not yet clear what other information will end up in > 'struct iommu_device', and I don't want to add another parameter to > iommu_device_register for every new struct member. That's a fair point. I think the ops, as a core piece of the whole API, would be sufficiently self-explanatory as part of registration, but then we'd end up with a weird interface with different members initialised through different paths, and I agree that ends up just as ugly. > Also I think having these wrappers is more readable in the code, as it > is clear what the code does without looking up the function prototypes > in the header. Yeah, on reflection explicit initialisation is certainly easier to read than a bunch of arguments handled implicitly by register(), but then from that angle, even more clear would be to simply have the drivers write the relevant struct members directly - I'd be quite happy with that, and we then don't have to add another setter to iommu.h for every new struct member (and risk it looking like Java code...) Robin. > > It might make sense to set the mandatory struct members via > iommu_device_register in the future, but we'll see :) > > > Joerg >
next prev parent reply other threads:[~2017-02-10 16:04 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-09 11:32 [PATCH 00/11 v3] Let IOMMU core know about individual IOMMUs Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 01/11] iommu: Rename iommu_get_instance() Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:12 ` Robin Murphy 2017-02-10 14:12 ` Robin Murphy 2017-02-10 15:36 ` Joerg Roedel 2017-02-10 15:36 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 02/11] iommu: Rename struct iommu_device Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 03/11] iommu: Introduce new 'struct iommu_device' Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 20:42 ` kbuild test robot 2017-02-09 20:42 ` kbuild test robot 2017-02-09 11:32 ` [PATCH 04/11] iommu: Add sysfs bindings for struct iommu_device Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 05/11] iommu: Make iommu_device_link/unlink take a " Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:16 ` Robin Murphy 2017-02-10 14:16 ` Robin Murphy 2017-02-10 15:22 ` Joerg Roedel 2017-02-10 15:22 ` Joerg Roedel 2017-02-10 16:03 ` Robin Murphy [this message] 2017-02-10 16:03 ` Robin Murphy 2017-02-10 16:11 ` Joerg Roedel 2017-02-10 16:11 ` Joerg Roedel 2017-02-10 16:59 ` Robin Murphy 2017-02-10 16:59 ` Robin Murphy 2017-02-09 11:32 ` [PATCH 07/11] iommu/arm-smmu: Make use of the iommu_register interface Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:20 ` Robin Murphy 2017-02-10 14:20 ` Robin Murphy 2017-02-10 15:25 ` Joerg Roedel 2017-02-10 15:25 ` Joerg Roedel 2017-02-10 17:07 ` Robin Murphy 2017-02-10 17:07 ` Robin Murphy 2017-02-09 11:32 ` [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:35 ` Robin Murphy 2017-02-10 14:35 ` Robin Murphy 2017-02-10 15:33 ` Joerg Roedel 2017-02-10 15:33 ` Joerg Roedel 2017-02-10 17:36 ` Robin Murphy 2017-02-10 17:36 ` Robin Murphy 2017-02-09 11:32 ` [PATCH 09/11] iommu/mediatek: " Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 10/11] iommu/exynos: " Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 13:46 ` Marek Szyprowski 2017-02-10 13:46 ` Marek Szyprowski 2017-02-10 13:46 ` Marek Szyprowski 2017-02-10 13:59 ` Joerg Roedel 2017-02-10 13:59 ` Joerg Roedel 2017-02-10 13:59 ` Joerg Roedel 2017-02-09 11:33 ` [PATCH 11/11] iommu: Remove iommu_register_instance interface Joerg Roedel 2017-02-09 11:33 ` Joerg Roedel 2017-02-09 11:33 ` Joerg Roedel
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=f770249b-ad98-10d8-ea79-dc10cef7f4df@arm.com \ --to=robin.murphy@arm.com \ --cc=alex.williamson@redhat.com \ --cc=dwmw2@infradead.org \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=jroedel@suse.de \ --cc=linux-kernel@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=will.deacon@arm.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.