Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1365528 > unrolled thread

[PATCH 0/3 v2] Add I2S/ADV7511 audio support for ARC AXS10x boards

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3 v2] Add I2S/ADV7511 audio support for ARC AXS10x boards Jose Abreu <Jose.Abreu@synopsys.com> - 2016-03-28 16:40 +0200
    Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Archit Taneja <architt@codeaurora.org> - 2016-03-29 10:10 +0200
      Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Jose Abreu <Jose.Abreu@synopsys.com> - 2016-03-29 13:00 +0200
        Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Archit Taneja <architt@codeaurora.org> - 2016-03-29 19:10 +0200
          Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Jose Abreu <Jose.Abreu@synopsys.com> - 2016-03-31 15:00 +0200
    Re: [PATCH 0/3 v2] Add I2S/ADV7511 audio support for ARC AXS10x  boards Mark Brown <broonie@kernel.org> - 2016-03-29 19:10 +0200
    Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Emil Velikov <emil.l.velikov@gmail.com> - 2016-03-30 12:00 +0200
    Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-04-01 19:20 +0200

#1365528 — [PATCH 0/3 v2] Add I2S/ADV7511 audio support for ARC AXS10x boards

ARC AXS10x platforms consist of a mainboard with several peripherals.
One of those peripherals is an HDMI output port controlled by the ADV7511
transmitter.

This patch set adds audio for the ADV7511 transmitter and I2S audio for
the AXS10x platform.

Changes v1 -> v2:
* DT bindings moved to separate patch (as suggested by Alexey Brodkin)
* Removed defconfigs entries (as suggested by Alexey Brodkin)

Jose Abreu (3):
  drm/i2c/adv7511: Add audio support
  ASoC: dwc: Add I2S HDMI audio support
  arc: axs10x: Add support for Designware I2S on DT

 arch/arc/boot/dts/axs10x_mb.dtsi    |   49 +-
 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 ++++++++++++++++++++++++++++++++++
 include/sound/soc-dai.h             |    1 +
 sound/soc/dwc/Kconfig               |    1 +
 sound/soc/dwc/designware_i2s.c      |  385 ++++++++++++-
 10 files changed, 1788 insertions(+), 1041 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

-- 
1.9.1

[toc] | [next] | [standalone]

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

Hi,

On 03/28/2016 08:06 PM, 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.

Is there a reason why we set the mode to HDMI at probe itself?
Shouldn't it be okay to set the mode later once we read the
EDID off the panel?

Some more comments below.

>
> SPDIF DAI format was also added to ASoC as it is required
> by adv7511 audio.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> ---
>
> 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 ++++++++++++++++++++++++++++++++++
>   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>

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

If we intend to have more audio funcs being used by the core in the
future, it would be nice to have NOP audio funcs rather than having
multiple #ifdef checks in the driver when CONFIG_DRM_I2C_ADV7511_AUDIO
isn't set.

> +
> +	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;
> +}
> +
> +static int adv7511_remove(struct i2c_client *i2c)
> +{
> +	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> +

Are we missing a call to adv7511_audio_exit() here?

Thanks,
Archit

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]

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

Hi Archit,

On 29-03-2016 09:05, Archit Taneja wrote:
> Hi,
>
> On 03/28/2016 08:06 PM, 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.
>
> Is there a reason why we set the mode to HDMI at probe itself?
> Shouldn't it be okay to set the mode later once we read the
> EDID off the panel?
>
> Some more comments below.
>

Well, when I was using this in kernel 3.18 (with an older version of the driver)
I noticed that DVI mode was being used even when HDMI was connected so I forced
the driver to start in HDMI mode. There were some changes in the driver so it is
possible that this is no longer needed. Should I drop it?

>>
>> SPDIF DAI format was also added to ASoC as it is required
>> by adv7511 audio.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> ---
>>
>> 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 ++++++++++++++++++++++++++++++++++
>>   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>
>
>> +
>> +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
>
> If we intend to have more audio funcs being used by the core in the
> future, it would be nice to have NOP audio funcs rather than having
> multiple #ifdef checks in the driver when CONFIG_DRM_I2C_ADV7511_AUDIO
> isn't set.
>

I will move this ifdef to adv751_audio and use NOP functions.

>> +
>> +    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;
>> +}
>> +
>> +static int adv7511_remove(struct i2c_client *i2c)
>> +{
>> +    struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>> +
>
> Are we missing a call to adv7511_audio_exit() here?

I followed the code in Analog Devices tree where there is no call to
audio_exit() but indeed you are correct. I will add this call.

>
> Thanks,
> Archit
>

Thanks for your comments!

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

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

On 3/29/2016 4:22 PM, Jose Abreu wrote:
> Hi Archit,
>
> On 29-03-2016 09:05, Archit Taneja wrote:
>> Hi,
>>
>> On 03/28/2016 08:06 PM, 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.
>>
>> Is there a reason why we set the mode to HDMI at probe itself?
>> Shouldn't it be okay to set the mode later once we read the
>> EDID off the panel?
>>
>> Some more comments below.
>>
>
> Well, when I was using this in kernel 3.18 (with an older version of the driver)
> I noticed that DVI mode was being used even when HDMI was connected so I forced
> the driver to start in HDMI mode. There were some changes in the driver so it is
> possible that this is no longer needed. Should I drop it?

Mode selection works fine with ADV7533 on a 4.5 kernel. I'm assuming it
should work out of the box for ADV7511 too. We should drop this.


>
>>>
>>> SPDIF DAI format was also added to ASoC as it is required
>>> by adv7511 audio.
>>>
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>> ---
>>>
>>> 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 ++++++++++++++++++++++++++++++++++
>>>    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>
>>
>>> +
>>> +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
>>
>> If we intend to have more audio funcs being used by the core in the
>> future, it would be nice to have NOP audio funcs rather than having
>> multiple #ifdef checks in the driver when CONFIG_DRM_I2C_ADV7511_AUDIO
>> isn't set.
>>
>
> I will move this ifdef to adv751_audio and use NOP functions.

Thanks, I think it should help in the longer run.

>
>>> +
>>> +    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;
>>> +}
>>> +
>>> +static int adv7511_remove(struct i2c_client *i2c)
>>> +{
>>> +    struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>>> +
>>
>> Are we missing a call to adv7511_audio_exit() here?
>
> I followed the code in Analog Devices tree where there is no call to
> audio_exit() but indeed you are correct. I will add this call.
>

Since we have 3 files for adv7511 now, could we also move the driver
to a separate folder? The long term plan is to convert all the i2c
slave encoder drivers to bridges. Keeping them together would be nicer
when we migrate this driver to the bridge folder.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

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

Hi Archit,


On 29-03-2016 18:03, Archit Taneja wrote:
>
>
> On 3/29/2016 4:22 PM, Jose Abreu wrote:
>> Hi Archit,
>>
>> On 29-03-2016 09:05, Archit Taneja wrote:
>>> Hi,
>>>
>>> On 03/28/2016 08:06 PM, 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.
>>>
>>> Is there a reason why we set the mode to HDMI at probe itself?
>>> Shouldn't it be okay to set the mode later once we read the
>>> EDID off the panel?
>>>
>>> Some more comments below.
>>>
>>
>> Well, when I was using this in kernel 3.18 (with an older version of the driver)
>> I noticed that DVI mode was being used even when HDMI was connected so I forced
>> the driver to start in HDMI mode. There were some changes in the driver so it is
>> possible that this is no longer needed. Should I drop it?
>
> Mode selection works fine with ADV7533 on a 4.5 kernel. I'm assuming it
> should work out of the box for ADV7511 too. We should drop this.
>
>

Ok, will drop.

>>
>>>>
>>>> SPDIF DAI format was also added to ASoC as it is required
>>>> by adv7511 audio.
>>>>
>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>> ---
>>>>
>>>> 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
>>>> ++++++++++++++++++++++++++++++++++
>>>>    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>
>>>
>>>> +
>>>> +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
>>>
>>> If we intend to have more audio funcs being used by the core in the
>>> future, it would be nice to have NOP audio funcs rather than having
>>> multiple #ifdef checks in the driver when CONFIG_DRM_I2C_ADV7511_AUDIO
>>> isn't set.
>>>
>>
>> I will move this ifdef to adv751_audio and use NOP functions.
>
> Thanks, I think it should help in the longer run.
>
>>
>>>> +
>>>> +    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;
>>>> +}
>>>> +
>>>> +static int adv7511_remove(struct i2c_client *i2c)
>>>> +{
>>>> +    struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>>>> +
>>>
>>> Are we missing a call to adv7511_audio_exit() here?
>>
>> I followed the code in Analog Devices tree where there is no call to
>> audio_exit() but indeed you are correct. I will add this call.
>>
>
> Since we have 3 files for adv7511 now, could we also move the driver
> to a separate folder? The long term plan is to convert all the i2c
> slave encoder drivers to bridges. Keeping them together would be nicer
> when we migrate this driver to the bridge folder.

Ok, will move to separate folder.

>
> Thanks,
> Archit
>

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1366482 — Re: [PATCH 0/3 v2] Add I2S/ADV7511 audio support for ARC AXS10x boards

[Multipart message — attachments visible in raw view] — view raw

On Mon, Mar 28, 2016 at 03:36:08PM +0100, Jose Abreu wrote:
> ARC AXS10x platforms consist of a mainboard with several peripherals.
> One of those peripherals is an HDMI output port controlled by the ADV7511
> transmitter.

I'm going to tell you the same thing I tell everyone else working on
HDMI audio integration: you all need to talk to each other and review
each other's code and unless there is a very good reason for it there
should be at least some code sharing.  For example take a look at the
patches Jyri Sarha has been posting recently.  I don't have detailed
knowledge of HDMI and the range of hardware that's out there for it but
I am seeing a number of different people posting patch serieses that
look a lot like each other and like they should be sharing things.

Please also try to keep your CC lists reasonable, the set of people
you've copied on this stuff is enormous and I'm having trouble seeing
why a lot of tehm are included.

[toc] | [prev] | [next] | [standalone]

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

Hi Jose,

On 28 March 2016 at 15:36, Jose Abreu <Jose.Abreu@synopsys.com> 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@synopsys.com>
> ---
>
> 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 keep the file rename separate (and use -M flag when generating
the patch) from the introduction of audio support. Having to check 1k
LOC movement alongside the introduction of new one is a bit...
suboptimal.

Regards,
Emil

[toc] | [prev] | [next] | [standalone]

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

Hi Jose,

Thank you for the patch.

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@synopsys.com>
> ---
> 
> 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.

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web