Re: [PATCH v8 4/8] drm/mediatek: Add encoder_index function to mtk_ddp_comp_funcs

[Eugen Hristev <eugen.hristev@collabora.com>]

On 8/2/23 17:47, Jason-JH.Lin wrote:
> 1. Add encoder_index function to mtk_ddp_comp_funcs to support dynamic
> connector selection for some ddp_comp who has encoder_index.
> 2. Add mtk_ddp_comp_encoder_index_set function to set encoder_index to
> each comp.
> 

Usually a commit that does two things in a list is supposed to be two 
actual commits.


> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index febcaeef16a1..8428baca70f4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -80,6 +80,7 @@ struct mtk_ddp_comp_funcs {
>   	void (*disconnect)(struct device *dev, struct device *mmsys_dev, unsigned int next);
>   	void (*add)(struct device *dev, struct mtk_mutex *mutex);
>   	void (*remove)(struct device *dev, struct mtk_mutex *mutex);
> +	unsigned int (*encoder_index)(struct device *dev);
>   };
>   
>   struct mtk_ddp_comp {
> @@ -87,6 +88,7 @@ struct mtk_ddp_comp {
>   	int irq;
>   	unsigned int id;
>   	const struct mtk_ddp_comp_funcs *funcs;
> +	unsigned int encoder_index;

For better alignment I would suggest variables to be declared together 
and pointers afterwards, not mixed up

>   };
>   
>   static inline int mtk_ddp_comp_clk_enable(struct mtk_ddp_comp *comp)
> @@ -275,6 +277,12 @@ static inline bool mtk_ddp_comp_disconnect(struct mtk_ddp_comp *comp, struct dev
>   	return false;
>   }
>   
> +static inline void mtk_ddp_comp_encoder_index_set(struct mtk_ddp_comp *comp)
> +{
> +	if (comp->funcs && comp->funcs->encoder_index)
> +		comp->encoder_index = comp->funcs->encoder_index(comp->dev);
> +}

it's also a bit strange that you added a function that is not used 
anywhere. Don't you get like a compiler warning for it ?

> +
>   int mtk_ddp_comp_get_id(struct device_node *node,
>   			enum mtk_ddp_comp_type comp_type);
>   unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm,