On Mon, Dec 02, 2024 at 02:20:04PM +0100, Maxime Ripard wrote: > Hi, > > Sorry, I've been drowning under work and couldn't review that series before. No worries, at this point I'm more concerned about my IGT series rather than this one. > > I'll review the driver API for now, and we can focus on the exact > implementation later on. > > On Sun, Dec 01, 2024 at 02:44:12AM +0200, Dmitry Baryshkov wrote: > > Drop driver-specific implementation and use the generic HDMI Codec > > framework in order to implement the HDMI audio support. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 68 ++++++++++-------------------------------- > > drivers/gpu/drm/vc4/vc4_hdmi.h | 2 -- > > 2 files changed, 15 insertions(+), 55 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 7295834e75fb1ab0cd241ed274e675567e66870b..d0a9aff7ad43016647493263c00d593296a1e3ad 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -595,6 +595,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, > > if (vc4_hdmi->variant->supports_hdr) > > max_bpc = 12; > > > > + connector->hdmi_codec.max_i2s_channels = 8; > > + connector->hdmi_codec.i2s = 1; > > + > > I guess it's a similar discussion than we had with HDMI2.0+ earlier > today, but I don't really like initializing by structs. Struct fields > are easy to miss, and can be easily uninitialized by mistake. > > I think I'd prefer to have them as argument to the init function. And if > they are optional, we can explicitly mark them as unused. Do you mean drm_connector_hdmi_init()? I think it's overloaded already, but I defintely can think about: drmm_connector_hdmi_init(..., max_bpc, HDMI_CODEC_I2S_PLAYBACK(8) | HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_DAI_ID(4)); or ... | HDMI_CODEC_NO_DAI_ID) The default (0) being equivalent to: HDMI_CODEC_NO_I2S | HDMI_CODEC_NO_SPDIF | HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_NO_DAI_ID WDYT? > > Like, it looks like the get_dai_id implementation relies on it being set > to < 0 for it to be ignored, but it's not here, so I'd assume it's used > with an ID of 0, even though the driver didn't support get_dai_id so > far? -- With best wishes Dmitry
