Re: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells

[Sebastian Reichel <sebastian.reichel@collabora.com>]

Hi,

On Wed, Jul 15, 2026 at 03:19:54PM +0200, Igor Paunovic wrote:
> For completeness: the driver part of the series currently wires
> up only the I2S interface (DAI 0); the binding now describes both
> interfaces as per the hardware.

Not implementing all features in the driver is perfectly fine.
Ideally you return -ENOTSUP if somebody tries to use the S/PDIF
DAI interface instead of I2S.

Greetings,

-- Sebastian

> 
> Thanks for the review!
> 
> Igor
> 
> On Wed, Jul 15, 2026 at 3:00 PM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > Hello Igor,
> >
> > On Wed, Jul 15, 2026 at 08:55:37AM +0200, Royal Net wrote:
> > > > Should the constraint be set to 0 to follow standard device tree
> > > > norms?
> > >
> > > Fair point. The current RFC wires up a single I2S DAI, and for a
> > > single-DAI provider 0 is indeed the norm -- rockchip,dw-hdmi does
> > > exactly that for its one DAI on the TX side.
> > >
> > > The reason I picked 1 is that the controller also has an S/PDIF
> > > audio output path (the vendor driver exposes both I2S and S/PDIF
> > > DAIs through hdmi-codec), so a DAI index becomes meaningful if that
> > > second path is wired up later.
> > >
> > > Unless the DT maintainers prefer keeping 1 from the start to avoid
> > > changing the cell count if/when the S/PDIF DAI is added, I'll switch
> > > to const: 0 in the next revision -- please let me know which you'd
> > > prefer.
> >
> > It should be 1 from the start as the DT binding is supposed to
> > describe the hardware and be stable. But in the next version you
> > should look into updating the commit descriptions to be less about
> > the kernel driver and more about the hardware. Maybe something like
> >
> > > The HDMI RX controller has two digital audio interfaces, one for
> > > I2S and one for S/PDIF. Allow referencing connections from audio
> > > codecs to these interfaces by documenting the missing #sound-dai-cells
> > > property.
> >
> > Apart from that it makes sense to document that argument 0 means
> > I2S and argument 1 means S/PDIF interface in the property description.
> >
> > Last but not least drop "A value of 1 lets a sound card reference
> > the audio DAI as <&hdmirx 0>.". Instead you can add this to the
> > example section at the end of the file.
> >
> > Greetings,
> >
> > -- Sebastian
> >
> > >
> > > Igor
> > >
> > > On Wed, Jul 15, 2026 at 7:54 AM <sashiko-bot@kernel.org> wrote:
> > > >
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [Medium] Incorrect cell count constraint for single-DAI provider.
> > > > --
> > > >
> > > > --- Patch [1]: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells ---
> > > > commit e6b0bac0b97f568ce749084e6c1e728bcd258b10
> > > > Author: Igor Paunovic <royalnet026@gmail.com>
> > > > dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
> > > >
> > > > This commit adds the #sound-dai-cells property to the HDMI RX controller
> > > > bindings, enabling a generic audio codec to capture incoming embedded audio.
> > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > > index b7f6c87d0e06a..83c10d9997c8e 100644
> > > > > --- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > > @@ -78,6 +78,13 @@ properties:
> > > > >        The phandle of the syscon node for the Video Output GRF register
> > > > >        to enable EDID transfer through SDAIN and SCLIN.
> > > > >
> > > > > +  "#sound-dai-cells":
> > > > > +    const: 1
> > > > > +    description:
> > > > > +      The HDMI RX controller registers a generic HDMI audio codec so the
> > > > > +      audio embedded in the incoming stream can be captured. A value of 1
> > > > > +      lets a sound card reference the audio DAI as <&hdmirx 0>.
> > > >
> > > > [Severity: Medium]
> > > > Is this cell count constraint correct for a single-DAI provider?
> > > >
> > > > Looking at properties:#sound-dai-cells, the hardware exposes only a single
> > > > generic audio DAI. Requiring a cell count of 1 forces device tree nodes to
> > > > use a non-standard syntax like <&hdmirx 0> instead of just <&hdmirx> for a
> > > > single resource.
> > > >
> > > > Should the constraint be set to 0 to follow standard device tree norms?
> > > >
> > > > --
> > > > Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=1