Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support

[Laurent Pinchart]

Hi Jose,

On Monday 04 Apr 2016 10:05:39 Jose Abreu wrote:
> On 01-04-2016 18:10, Laurent Pinchart wrote:
> > On Monday 28 Mar 2016 15:36:09 Jose Abreu wrote:
> >> This patch adds audio support for the ADV7511 HDMI transmitter
> >> using ALSA SoC.
> >> 
> >> The code was ported from Analog Devices linux tree from
> >> commit 1770c4a1e32b ("Merge remote-tracking branch
> >> 
> >> 'xilinx/master' into xcomm_zynq"), which is available at:
> >> 	- https://github.com/analogdevicesinc/linux/
> >> 
> >> The main core file was renamed from adv7511.c to adv7511_core.c
> >> so that audio and video compile into a single adv7511.ko module
> >> and to keep up with Analog Devices kernel tree.
> >> 
> >> The audio can be disabled using menu-config so it is possible
> >> to use only video mode.
> >> 
> >> The HDMI mode is automatically started at boot and the audio
> >> (when enabled) registers as a codec into ALSA.
> >> 
> >> SPDIF DAI format was also added to ASoC as it is required
> >> by adv7511 audio.
> >> 
> >> Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>
> >> ---
> >> 
> >> No changes v1 -> v2.
> >> 
> >>  drivers/gpu/drm/i2c/Kconfig         |   11 +
> >>  drivers/gpu/drm/i2c/Makefile        |    2 +
> >>  drivers/gpu/drm/i2c/adv7511.c       | 1024 ----------------------------
> >>  drivers/gpu/drm/i2c/adv7511.h       |   41 ++
> >>  drivers/gpu/drm/i2c/adv7511_audio.c |  310 +++++++++++
> >>  drivers/gpu/drm/i2c/adv7511_core.c  | 1005 ++++++++++++++++++++++++++++
> > 
> > Please use git-format-patch -M to detect renames if you send a new version
> > of this series, it will help with review.
> 
> Ok, will do that in next version.
> 
> >>  include/sound/soc-dai.h             |    1 +
> >>  7 files changed, 1370 insertions(+), 1024 deletions(-)
> >>  delete mode 100644 drivers/gpu/drm/i2c/adv7511.c
> >>  create mode 100644 drivers/gpu/drm/i2c/adv7511_audio.c
> >>  create mode 100644 drivers/gpu/drm/i2c/adv7511_core.c
> > 
> > [snip]
> > 
> >> diff --git a/drivers/gpu/drm/i2c/adv7511_core.c
> >> b/drivers/gpu/drm/i2c/adv7511_core.c new file mode 100644
> >> index 0000000..d54256a
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i2c/adv7511_core.c
> > 
> > [snip]
> > 
> >> +static int adv7511_probe(struct i2c_client *i2c, const struct
> >> i2c_device_id *id) +{
> >> +	struct adv7511_link_config link_config;
> >> +	struct adv7511 *adv7511;
> >> +	struct device *dev = &i2c->dev;
> >> +	unsigned int val;
> >> +	int ret;
> >> +
> >> +	if (!dev->of_node)
> >> +		return -EINVAL;
> >> +
> >> +	adv7511 = devm_kzalloc(dev, sizeof(*adv7511), GFP_KERNEL);
> >> +	if (!adv7511)
> >> +		return -ENOMEM;
> >> +
> >> +	adv7511->powered = false;
> >> +	adv7511->status = connector_status_disconnected;
> >> +
> >> +	ret = adv7511_parse_dt(dev->of_node, &link_config);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/*
> >> +	 * The power down GPIO is optional. If present, toggle it from active
> >> to
> >> +	 * inactive to wake up the encoder.
> >> +	 */
> >> +	adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", 
GPIOD_OUT_HIGH);
> >> +	if (IS_ERR(adv7511->gpio_pd))
> >> +		return PTR_ERR(adv7511->gpio_pd);
> >> +
> >> +	if (adv7511->gpio_pd) {
> >> +		mdelay(5);
> >> +		gpiod_set_value_cansleep(adv7511->gpio_pd, 0);
> >> +	}
> >> +
> >> +	adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config);
> >> +	if (IS_ERR(adv7511->regmap))
> >> +		return PTR_ERR(adv7511->regmap);
> >> +
> >> +	ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val);
> >> +	if (ret)
> >> +		return ret;
> >> +	dev_dbg(dev, "Rev. %d\n", val);
> >> +
> >> +	ret = regmap_register_patch(adv7511->regmap, adv7511_fixed_registers,
> >> +				    ARRAY_SIZE(adv7511_fixed_registers));
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> >> edid_i2c_addr);
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> >> +		     packet_i2c_addr);
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, 
cec_i2c_addr);
> >> +	adv7511_packet_disable(adv7511, 0xffff);
> >> +
> >> +	adv7511->i2c_main = i2c;
> >> +	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> >> +	if (!adv7511->i2c_edid)
> >> +		return -ENOMEM;
> >> +
> >> +	if (i2c->irq) {
> >> +		init_waitqueue_head(&adv7511->wq);
> >> +
> >> +		ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> >> +						adv7511_irq_handler,
> >> +						IRQF_ONESHOT, dev_name(dev),
> >> +						adv7511);
> >> +		if (ret)
> >> +			goto err_i2c_unregister_device;
> >> +	}
> >> +
> >> +	/* CEC is unused for now */
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
> >> +		     ADV7511_CEC_CTRL_POWER_DOWN);
> >> +
> >> +	adv7511_power_off(adv7511);
> >> +
> >> +	i2c_set_clientdata(i2c, adv7511);
> >> +
> >> +#ifdef CONFIG_DRM_I2C_ADV7511_AUDIO
> >> +	adv7511_audio_init(&i2c->dev);
> >> +#endif
> > 
> > Shouldn't we condition this to the audio channel being somehow described
> > in DT ? If a board doesn't route audio signals to the ADV7511 audio
> > input, there's no need to register an audio codec.
> 
> I can do this but the audio is already conditionally compiled using
> menuconfig. Is it really necessary to add this extra layer of condition?

The idea is that enabling support for ADV7511 audio in the kernel isn't 
coupled with whether the system includes audio support. It would be confusing, 
and would also waste resources, to create a Linux sound device when no sound 
channel is routed on the board.

> >> +
> >> +	adv7511_set_link_config(adv7511, &link_config);
> >> +
> >> +	/* Enable HDMI mode */
> >> +	regmap_update_bits(adv7511->regmap, ADV7511_REG_HDCP_HDMI_CFG,
> >> +			ADV7511_HDMI_CFG_MODE_MASK,
> >> +			ADV7511_HDMI_CFG_MODE_HDMI);
> >> +
> >> +	return 0;
> >> +
> >> +err_i2c_unregister_device:
> >> +	i2c_unregister_device(adv7511->i2c_edid);
> >> +
> >> +	return ret;
> >> +}

-- 
Regards,

Laurent Pinchart