Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4900 bytes --]

On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
> From: Arto Merilainen <amerilainen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[...]
> 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 #2: Type: application/pgp-signature, Size: 836 bytes --]