Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

From: Thierry Reding <thierry.reding@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Jingoo Han <jg1.han@samsung.com>, sunil joshi <joshi@samsung.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Ajay kumar <ajaynumb@gmail.com>,
	Thierry Reding <treding@nvidia.com>,
	Prashanth G <prashanth.g@samsung.com>,
	Ajay Kumar <ajaykumar.rs@samsung.com>
Subject: Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
Date: Mon, 3 Nov 2014 09:01:58 +0100	[thread overview]
Message-ID: <20141103080155.GA21002@ulmo.nvidia.com> (raw)
In-Reply-To: <20141031155143.GQ26941@phenom.ffwll.local>

[-- Attachment #1.1: Type: text/plain, Size: 2885 bytes --]

On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > > That makes the entire thing a bit non-trivial, which is why I think it
> > > would be better as some generic helper. Which then gets embedded or
> > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> > > Or maybe even acpi&drm_bridge, who knows ;-)
> > 
> > I worry a little about type safety. How will this generic helper know
> > what registry to look in for? Or conversely, if all these resources are
> > added to a single registry how do you know that they're of the correct
> > type? Failing to ensure this could cause situations where you're asking
> > for a panel and get a bridge in return because you've wrongly wired it
> > up in device tree for example.
> > 
> > But perhaps if both the registry and the device parts are turned into
> > helpers we could still have a single core implementation and then
> > instantiate that for each type, something roughly like this:
> > 
> > 	struct registry {
> > 		struct list_head list;
> > 		struct mutex lock;
> > 	};
> > 
> > 	struct registry_record {
> > 		struct list_head list;
> > 		struct module *owner;
> > 		struct kref *ref;
> > 
> > 		struct device *dev;
> > 	};
> > 
> > 	int registry_add(struct registry *registry, struct registry_record *record)
> > 	{
> > 		...
> > 		try_module_get(record->owner);
> > 		...
> > 	}
> > 
> > 	struct registry_record *registry_find_by_of_node(struct registry *registry,
> > 							 struct device_node *np)
> > 	{
> > 		...
> > 		kref_get(...);
> > 		...
> > 	}
> > 
> > That way it should be possible to embed these into other structures,
> > like so:
> > 
> > 	struct drm_panel {
> > 		struct registry_record record;
> > 		...
> > 	};
> > 
> > 	static struct registry drm_panels;
> > 
> > 	int drm_panel_add(struct drm_panel *panel)
> > 	{
> > 		return registry_add(&drm_panels, &panel->record);
> > 	}
> > 
> > 	struct drm_panel *of_drm_panel_find(struct device_node *np)
> > 	{
> > 		struct registry_record *record;
> > 
> > 		record = registry_find_by_of_node(&drm_panels, np);
> > 
> > 		return container_of(record, struct drm_panel, record);
> > 	}
> > 
> > Is that what you had in mind?
> 
> Yeah I've thought that we should instantiate using macros even, so that we
> have per-type registries. So you'd smash the usual set of
> DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
> a (name, key, value) tripled. For the example here(of_drm_panel, struct
> device_node *, struct drm_panel *) or similar. I might be hand-waving over
> a few too many details though ;-)

Okay, I'll take a stab at this and see if I can convert DRM panel to it.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel