Re: [PATCH 1/2 v3] drm/bridge: adv7511: Add Audio support.

[Lars-Peter Clausen <lars@metafoo.de>]

On 09/07/2016 01:22 AM, John Stultz wrote:
> This patch adds support to Audio for both adv7511 and adv7533
> bridge chips.
> 
> This patch was originally from [1] by Lars-Peter Clausen <lars@metafoo.de>
> and was adapted by Archit Taneja <architt@codeaurora.org> and
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org>.
> 
> Then I heavily reworked it to use the hdmi-codec driver. And also
> folded in some audio packet initialization done by Andy Green
> <andy.green@linaro.org>. So credit to them, but blame to me.
> 
> [1] https://github.com/analogdevicesinc/linux/blob/xcomm_zynq/drivers/gpu/drm/i2c/adv7511_audio.c
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Andy Green <andy@warmcat.com>
> Cc: Dave Long <dave.long@linaro.org>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Jose Abreu <joabreu@synopsys.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>

I'm not to fond of the hdmi-codec stuff, but within its context this looks
ok. Thanks for getting this upstream ready. Just one tiny bit regarding the
Kconfig entry.

If that is fixed feel free to add on the next revision:

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
> v3:
> * Allowed audio support to be configured in or out, as suggested by Laurent
> * Minor cleanups suggested by Laurent
> * Folded in Andy's audio packet initialization patch, as suggested by Archit
> 
>  drivers/gpu/drm/bridge/adv7511/Kconfig         |   9 ++
>  drivers/gpu/drm/bridge/adv7511/Makefile        |   1 +
>  drivers/gpu/drm/bridge/adv7511/adv7511.h       |  16 ++
>  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 213 +++++++++++++++++++++++++
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |   4 +
>  5 files changed, 243 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
> index d2b0499..b303ad1 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
> @@ -3,9 +3,18 @@ config DRM_I2C_ADV7511
>  	depends on OF
>  	select DRM_KMS_HELPER
>  	select REGMAP_I2C
> +	select SND_SOC_HDMI_CODEC if SND_SOC

This seems to be redundant considering the select done by DRM_I2C_ADV7511_AUDIO.

>  	help
>  	  Support for the Analog Device ADV7511(W) and ADV7513 HDMI encoders.
>  
> +config DRM_I2C_ADV7511_AUDIO
> +	tristate "ADV7511 HDMI Audio driver"

This should be bool. It either gets built into the ADV7511 driver (which
itself could either be a module or built-in) or not. But setting this option
itself to module wont work.

> +	depends on DRM_I2C_ADV7511 && SND_SOC
> +	select SND_SOC_HDMI_CODEC
> +	help
> +	  Support the ADV7511 HDMI Audio interface. This is used in
> +	  conjunction with the AV7511  HDMI driver.
> +

[...]
> +static void audio_shutdown(struct device *dev, void *data)
> +{
> +}

Unrelated to this patch, but it looks like the shutdown callback should
maybe be made optional.

> +static const struct hdmi_codec_ops adv7511_codec_ops = {
> +	.hw_params	= adv7511_hdmi_hw_params,
> +	.audio_shutdown = audio_shutdown,
> +	.audio_startup	= audio_startup,
> +};