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
