On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote: > From: Arto Merilainen <amerilainen@xxxxxxxxxx> [...] > 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
Attachment:
pgpALUCCNrrMk.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel