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

[Igor Paunovic]

Hello Sebastian,

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

Done for v2: the codec registration now exposes the S/PDIF DAI as
well, so the DAI indexes match the binding (0 = I2S, 1 = S/PDIF),
and hw_params() rejects the S/PDIF interface with -EOPNOTSUPP until
that path is actually wired up.

Thanks!

Igor

On Wed, Jul 15, 2026 at 7:28 PM Sebastian Reichel
<sebastian.reichel@xxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > > > 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@xxxxxxxxx?part=1