From:   Jani Nikula <jani.nikula@linux.intel.com>
To:     Stephen Boyd <swboyd@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Douglas Anderson <dianders@chromium.org>
Cc:     Saravana Kannan <saravanak@google.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Russell King <rmk+kernel@arm.linux.org.uk>,
        freedreno@lists.freedesktop.org, Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH v5 03/32] component: Move struct aggregate_device out to
 header file
In-Reply-To: <20220106214556.2461363-4-swboyd@chromium.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20220106214556.2461363-1-swboyd@chromium.org>
 <20220106214556.2461363-4-swboyd@chromium.org>
Date:   Fri, 07 Jan 2022 15:07:59 +0200
Message-ID: <87pmp3lmmo.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Thu, 06 Jan 2022, Stephen Boyd <swboyd@chromium.org> wrote:
> This allows aggregate driver writers to use the device passed to their
> probe/remove/shutdown functions properly instead of treating it as an
> opaque pointer.

You say it like having opaque pointers with interfaces instead of
exposed data is a bad thing.

Data is not an interface. IMO if you can get by with keeping the types
private, go for it. Unless I'm missing something, you only need the
parent dev pointer. Maybe add a helper function for it?

It's trivial to expose the guts like this, but it's usually a lot of
hard work to go the other way. Look at the dependencies of component.h
now. To keep it self-contained, i.e. buildable without implicit
dependencies, you'd need to add #include <device.h>, which goes on to
include the world. Then have a look at [1].

Please at least let's not do this lightly.


BR,
Jani.


[1] https://lwn.net/ml/linux-kernel/YdIfz+LMewetSaEB@gmail.com/


>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/base/component.c  | 15 ---------------
>  include/linux/component.h | 19 ++++++++++++++++---
>  2 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index eec82caeae5e..dc38a8939ae6 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -56,21 +56,6 @@ struct component_match {
>  	struct component_match_array *compare;
>  };
>  
> -struct aggregate_device {
> -	const struct component_master_ops *ops;
> -	struct device *parent;
> -	struct device dev;
> -	struct component_match *match;
> -	struct aggregate_driver *adrv;
> -
> -	int id;
> -};
> -
> -static inline struct aggregate_device *to_aggregate_device(struct device *d)
> -{
> -	return container_of(d, struct aggregate_device, dev);
> -}
> -
>  struct component {
>  	struct list_head node;
>  	struct aggregate_device *adev;
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 95d1b23ede8a..e99cf8e910f0 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -5,6 +5,8 @@
>  #include <linux/stddef.h>
>  #include <linux/device.h>
>  
> +struct component_match;
> +
>  /**
>   * struct component_ops - callbacks for component drivers
>   *
> @@ -39,8 +41,6 @@ void component_del(struct device *, const struct component_ops *);
>  int component_bind_all(struct device *master, void *master_data);
>  void component_unbind_all(struct device *master, void *master_data);
>  
> -struct aggregate_device;
> -
>  /**
>   * struct component_master_ops - callback for the aggregate driver
>   *
> @@ -80,7 +80,20 @@ struct component_master_ops {
>  	void (*unbind)(struct device *master);
>  };
>  
> -struct component_match;
> +struct aggregate_device {
> +	const struct component_master_ops *ops;
> +	struct device *parent;
> +	struct device dev;
> +	struct component_match *match;
> +	struct aggregate_driver *adrv;
> +
> +	int id;
> +};
> +
> +static inline struct aggregate_device *to_aggregate_device(struct device *d)
> +{
> +	return container_of(d, struct aggregate_device, dev);
> +}
>  
>  /**
>   * struct aggregate_driver - Aggregate driver (made up of other drivers)

-- 
Jani Nikula, Intel Open Source Graphics Center