<pre>
Hi Eugen,

Thanks for the reviews.

On Thu, 2023-08-03 at 16:27 +0300, Eugen Hristev wrote:
> 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.
>
OK, I'll move the second part to [PATCH v8 6/8].

>
> > 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
>

OK, I'll move it between `unsigned int id` and `const struct
mtk_ddp_comp_funcs *funcs`.


> > };
> >
> > 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 ?
>
Because it's used in mtk_drm_crtc_create() in [PATCH v8 6/8].
I'll move this part into [PATCH v8 6/8].

Regards,
Jason-JH.Lin

> > +
> > 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,
>
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->