Re: [PATCH v5 1/5] dt-bindings: media: add mediatek ISP3.0 sensor interface

[Conor Dooley <conor.dooley@microchip.com>]

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

On Fri, Jul 05, 2024 at 09:50:59AM +0200, Julien Stephan wrote:
> Le jeu. 4 juil. 2024 à 18:27, Conor Dooley <conor@kernel.org> a écrit :
> >
> > On Thu, Jul 04, 2024 at 03:36:40PM +0200, Julien Stephan wrote:
> > > From: Louis Kuo <louis.kuo@mediatek.com>
> > >
> > > This adds the bindings, for the mediatek ISP3.0 SENINF module embedded in
> > > some Mediatek SoC, such as the mt8365
> > >
> > > Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
> > > Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com>
> > > Link: https://lore.kernel.org/r/20230807094940.329165-2-jstephan@baylibre.com
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> >
> > I'm really confused by the link tag here. At first glance this looked
> > like you were sending out something that had been applied by Laurent,
> > given the Link, Rb and SoB from him. Why does he have a SoB on this
> > patch? What did Phi-Bang Nguyen do with this patch, and should they have
> > a Co-developed-by tag?
> 
> I was not using b4 for the previous revisions of this series, so maybe
> I messed something up here :(

b4 am has an option to add a link to a patch you apply from the mailing
list (-l, --add-link) but you should not be using that as a contributor.
In this case, that link provides no value and is just confusing.

> About Phi-Bang, this series has been in our internal tree for a long
> time, and Phi-Bang has his SoB on it, so I kept it.
> 
> About Laurent's tags, they were already on v4. But maybe it was an
> error ? Should I remove them?

They were also on v1. Did Laurent write part of these bindings, and
should he have a Co-developed-by?

> > > +additionalProperties: false
> > > +
> > > +if:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        const: mediatek,mt8365-seninf
> >
> > The binding supports only a single compatible, why is this complexity
> > required? I don't see other devices being added in this series.
> 
> Right. The idea is that the number of PHYs depends on the SoC. In the
> previous revision of the series,
> the number of PHYs was not fixed, and Krzysztof asked me to fix it by
> SoC. So I wanted to make it clear
> that the number of PHYs depends on SoC but maybe I don't need that
> complexity for that?
> 
> Is something like the following enough? And if complexity is added
> later if some other SoC are added?

Yes, that looks reasonable to me. Adding conditional stuff can be done
iff another soc re-uses the binding.

Thanks,
Conor.

>     phys:
>       minItems: 2
>       maxItems: 2
>       description:
>         phandle to the PHYs connected to CSI0/A, CSI1, CSI0B
> 
>     phy-names:
>       description:
>         list of PHYs names
>       minItems: 2
>       maxItems: 2
>       items:
>         type: string
>         enum:
>           - csi0
>           - csi1
>           - csi0b
>       uniqueItems: true

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]