Received-SPF: pass (google.com: domain of linux-kernel+bounces-195149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 30 May 2024 11:16:08 +0200
From: Maxime Ripard <mripard@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Robert Foss <rfoss@kernel.org>, 
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>, 
	Jernej Skrabec <jernej.skrabec@gmail.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, Rob Clark <robdclark@gmail.com>, 
	Abhinav Kumar <quic_abhinavk@quicinc.com>, Sean Paul <sean@poorly.run>, 
	Marijn Suijten <marijn.suijten@somainline.org>, dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, 
	freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/7] drm/msm/hdmi: make use of the drm_connector_hdmi
 framework
Message-ID: <20240530-poised-burgundy-petrel-7affed@houat>
References: <20240530-bridge-hdmi-connector-v3-0-a1d184d68fe3@linaro.org>
 <20240530-bridge-hdmi-connector-v3-5-a1d184d68fe3@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha384;
	protocol="application/pgp-signature"; boundary="5forzva7txjtgmqb"
Content-Disposition: inline
In-Reply-To: <20240530-bridge-hdmi-connector-v3-5-a1d184d68fe3@linaro.org>


--5forzva7txjtgmqb
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Thu, May 30, 2024 at 02:12:28AM GMT, Dmitry Baryshkov wrote:
> Setup the HDMI connector on the MSM HDMI outputs. Make use of
> atomic_check hook and of the provided Infoframe infrastructure.
>=20
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

As a general comment: I really like it, it looks super tidy. Thanks!

There's a couple of minor issues below

> -int msm_hdmi_audio_update(struct hdmi *hdmi)
> +static int msm_hdmi_audio_update(struct hdmi *hdmi)
>  {
>  	struct hdmi_audio *audio =3D &hdmi->audio;
> -	struct hdmi_audio_infoframe *info =3D &audio->infoframe;
>  	const struct hdmi_msm_audio_arcs *arcs =3D NULL;
>  	bool enabled =3D audio->enabled;
>  	uint32_t acr_pkt_ctrl, vbi_pkt_ctrl, aud_pkt_ctrl;
> -	uint32_t infofrm_ctrl, audio_config;
> +	uint32_t audio_config;
> +
> +	if (!hdmi->hdmi_mode)
> +		return -EINVAL;
> +
> +	DBG("audio: enabled=3D%d, channels=3D%d, rate=3D%d",
> +	    audio->enabled, audio->channels, audio->rate);
> =20
> -	DBG("audio: enabled=3D%d, channels=3D%d, channel_allocation=3D0x%x, "
> -		"level_shift_value=3D%d, downmix_inhibit=3D%d, rate=3D%d",
> -		audio->enabled, info->channels,  info->channel_allocation,
> -		info->level_shift_value, info->downmix_inhibit, audio->rate);
>  	DBG("video: power_on=3D%d, pixclock=3D%lu", hdmi->power_on, hdmi->pixcl=
ock);

pixclock should come from the connector state too. It's still calculated
by the driver in msm_hdmi_bridge_mode_set

> @@ -341,8 +425,11 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
>  	bridge->funcs =3D &msm_hdmi_bridge_funcs;
>  	bridge->ddc =3D hdmi->i2c;
>  	bridge->type =3D DRM_MODE_CONNECTOR_HDMIA;
> +	bridge->vendor =3D "Qualcomm";
> +	bridge->product =3D "Snapdragon";
>  	bridge->ops =3D DRM_BRIDGE_OP_HPD |
>  		DRM_BRIDGE_OP_DETECT |
> +		DRM_BRIDGE_OP_HDMI |
>  		DRM_BRIDGE_OP_EDID;
> =20
>  	ret =3D devm_drm_bridge_add(hdmi->dev->dev, bridge);

It looks like you're not setting either the supported formats or bpc?

Maxime

--5forzva7txjtgmqb
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iJUEABMJAB0WIQTkHFbLp4ejekA/qfgnX84Zoj2+dgUCZlhD2AAKCRAnX84Zoj2+
dvdkAYCryHF7SD2wn1N8TdrGd15DJPIga+r9drDaxTr0rBTOxzbQF7x7YiyX2hH0
CTeY9AsBgKWaEKe7Dbe67TtNLnv9K8XLecp8Gdd4Q7qOdun/xGlo9UWqcOTNeHVb
uIQEISEO9A==
=tPbA
-----END PGP SIGNATURE-----

--5forzva7txjtgmqb--