On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
From: Arto Merilainen amerilainen@nvidia.com
[...]
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
[...]
+static int tegra_drm_add_client(struct device *dev, void *data) +{
- static const char * const compat[] = {
"nvidia,tegra20-dc","nvidia,tegra20-hdmi","nvidia,tegra30-dc","nvidia,tegra30-hdmi",- };
- struct tegradrm *tegradrm = data;
- struct tegra_drm_client_entry *client;
- unsigned int i;
- for (i = 0; i < ARRAY_SIZE(compat); i++) {
if (of_device_is_compatible(dev->of_node, compat[i]) &&of_device_is_available(dev->of_node)) {client = kzalloc(sizeof(*client), GFP_KERNEL);if (!client)return -ENOMEM;INIT_LIST_HEAD(&client->list);client->np = of_node_get(dev->of_node);list_add_tail(&client->list, &tegradrm->drm_clients);dev_set_drvdata(dev, tegradrm);
This should go away now that we have an accessor for this, right?
+static int tegra_drm_parse_dt(struct tegradrm *tegradrm) +{
- int err;
- struct device *dev;
- /* host1x is parent of all devices */
- dev = bus_find_device_by_name(&platform_bus_type, NULL, "host1x");
- if (!dev)
return -ENODEV;- /* find devices that are available and add them into the 'required'
* list */- err = device_for_each_child(dev, tegradrm, tegra_drm_add_client);
- return err;
+}
I don't see why we can't keep the original code here. The problem with your approach is that you'll match on a global device host1x, regardless of it's relationship to tegra-drm. Instead what you should be doing is use the hierarchical information that you have to make this work. So the dummy device should be registered as a child device of host1x, because that allows the host1x device to be obtained from the dummy's parent. In that way you can use the host1x device's of_node to search through the devicetree. That's precisely what the existing code does and I see no reason to change that.
+static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp) +{
+}
This can be removed, right?
+static struct platform_driver tegra_drm_platform_driver = {
- .driver = {
.name = "tegradrm",
This should be "tegra-drm" to match the module name.
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
[...]
index 3a843a7..34cc3a1 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -17,6 +17,7 @@ #include <drm/drm_gem_cma_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fixed.h> +#include <drm/tegra_drm.h>
struct tegra_framebuffer { struct drm_framebuffer base; @@ -28,17 +29,11 @@ static inline struct tegra_framebuffer *to_tegra_fb(struct drm_framebuffer *fb) return container_of(fb, struct tegra_framebuffer, base); }
-struct host1x {
- struct drm_device *drm;
+struct tegradrm {
Similarly, this should be tegra_drm.
-struct host1x_client; +struct tegra_drm_client;
I don't see the point in renaming this. All of the devices are still host1x clients, right? This patch would be a whole shorter if we didn't rename these. None of these symbols are exported either so there's not much chance for them to clash with anything.
static int tegra_hdmi_probe(struct platform_device *pdev) {
- struct host1x *host1x = dev_get_drvdata(pdev->dev.parent); struct tegra_hdmi *hdmi; struct resource *regs; int err;
- struct tegradrm *tegradrm = platform_get_drvdata(pdev);
I think we all agreed already that you shouldn't be mucking with the driver private data in this way.
diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h new file mode 100644 index 0000000..8632f49 --- /dev/null +++ b/include/drm/tegra_drm.h @@ -0,0 +1,20 @@ +/*
- Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved.
- This program is free software; you can redistribute it and/or modify it
- under the terms and conditions of the GNU General Public License,
- version 2, as published by the Free Software Foundation.
- This program is distributed in the hope it will be useful, but WITHOUT
- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
- more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
- */
+#ifndef _TEGRA_DRM_H_ +#define _TEGRA_DRM_H_
+#endif
This can be removed as well.
Thierry