Re: [PATCH 4/4] drm: adv7511/33: add HDMI CEC support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/08/17 10:49, Archit Taneja wrote:
> 
> 
> On 07/30/2017 06:37 PM, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Add support for HDMI CEC to the drm adv7511/adv7533 drivers.
>>
>> The CEC registers that we need to use are identical for both drivers,
>> but they appear at different offsets in the register map.
> 
> Thanks for the patch. Some minor comments below.
> 
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>>   drivers/gpu/drm/bridge/adv7511/Kconfig       |   8 +
>>   drivers/gpu/drm/bridge/adv7511/Makefile      |   1 +
>>   drivers/gpu/drm/bridge/adv7511/adv7511.h     |  45 +++-
>>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 314 +++++++++++++++++++++++++++
>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 152 +++++++++++--
>>   drivers/gpu/drm/bridge/adv7511/adv7533.c     |  30 +--
>>   6 files changed, 500 insertions(+), 50 deletions(-)
>>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
>> index 2fed567f9943..592b9d2ec034 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
>> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
>> @@ -21,3 +21,11 @@ config DRM_I2C_ADV7533
>>       default y
>>       help
>>         Support for the Analog Devices ADV7533 DSI to HDMI encoder.
>> +
>> +config DRM_I2C_ADV7511_CEC
>> +    bool "ADV7511/33 HDMI CEC driver"
>> +    depends on DRM_I2C_ADV7511
>> +    select CEC_CORE
>> +    default y
>> +    help
>> +      When selected the HDMI transmitter will support the CEC feature.
>> diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile b/drivers/gpu/drm/bridge/adv7511/Makefile
>> index 5ba675534f6e..5bb384938a71 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/Makefile
>> +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
>> @@ -1,4 +1,5 @@
>>   adv7511-y := adv7511_drv.o
>>   adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
>> +adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
>>   adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
>>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index fe18a5d2d84b..4fd7b14f619b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -195,6 +195,25 @@
>>   #define ADV7511_PACKET_GM(x)        ADV7511_PACKET(5, x)
>>   #define ADV7511_PACKET_SPARE(x)        ADV7511_PACKET(6, x)
>>   +#define ADV7511_REG_CEC_TX_FRAME_HDR    0x00
>> +#define ADV7511_REG_CEC_TX_FRAME_DATA0    0x01
>> +#define ADV7511_REG_CEC_TX_FRAME_LEN    0x10
>> +#define ADV7511_REG_CEC_TX_ENABLE    0x11
>> +#define ADV7511_REG_CEC_TX_RETRY    0x12
>> +#define ADV7511_REG_CEC_TX_LOW_DRV_CNT    0x14
>> +#define ADV7511_REG_CEC_RX_FRAME_HDR    0x15
>> +#define ADV7511_REG_CEC_RX_FRAME_DATA0    0x16
>> +#define ADV7511_REG_CEC_RX_FRAME_LEN    0x25
>> +#define ADV7511_REG_CEC_RX_ENABLE    0x26
>> +#define ADV7511_REG_CEC_RX_BUFFERS    0x4a
>> +#define ADV7511_REG_CEC_LOG_ADDR_MASK    0x4b
>> +#define ADV7511_REG_CEC_LOG_ADDR_0_1    0x4c
>> +#define ADV7511_REG_CEC_LOG_ADDR_2    0x4d
>> +#define ADV7511_REG_CEC_CLK_DIV        0x4e
>> +#define ADV7511_REG_CEC_SOFT_RESET    0x50
>> +
>> +#define ADV7533_REG_CEC_OFFSET        0x70
>> +
>>   enum adv7511_input_clock {
>>       ADV7511_INPUT_CLOCK_1X,
>>       ADV7511_INPUT_CLOCK_2X,
>> @@ -297,6 +316,8 @@ enum adv7511_type {
>>       ADV7533,
>>   };
>>   +#define ADV7511_MAX_ADDRS 3
>> +
>>   struct adv7511 {
>>       struct i2c_client *i2c_main;
>>       struct i2c_client *i2c_edid;
>> @@ -343,15 +364,29 @@ struct adv7511 {
>>         enum adv7511_type type;
>>       struct platform_device *audio_pdev;
>> +
>> +    struct cec_adapter *cec_adap;
>> +    u8   cec_addr[ADV7511_MAX_ADDRS];
>> +    u8   cec_valid_addrs;
>> +    bool cec_enabled_adap;
>> +    struct clk *cec_clk;
>> +    u32 cec_clk_freq;
>>   };
>>   +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +extern const struct cec_adap_ops adv7511_cec_adap_ops;
>> +
>> +void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset);
>> +int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511);
>> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>> +#endif
>> +
>>   #ifdef CONFIG_DRM_I2C_ADV7533
>>   void adv7533_dsi_power_on(struct adv7511 *adv);
>>   void adv7533_dsi_power_off(struct adv7511 *adv);
>>   void adv7533_mode_set(struct adv7511 *adv, struct drm_display_mode *mode);
>>   int adv7533_patch_registers(struct adv7511 *adv);
>> -void adv7533_uninit_cec(struct adv7511 *adv);
>> -int adv7533_init_cec(struct adv7511 *adv);
>> +int adv7533_patch_cec_registers(struct adv7511 *adv);
>>   int adv7533_attach_dsi(struct adv7511 *adv);
>>   void adv7533_detach_dsi(struct adv7511 *adv);
>>   int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
>> @@ -374,11 +409,7 @@ static inline int adv7533_patch_registers(struct adv7511 *adv)
>>       return -ENODEV;
>>   }
>>   -static inline void adv7533_uninit_cec(struct adv7511 *adv)
>> -{
>> -}
>> -
>> -static inline int adv7533_init_cec(struct adv7511 *adv)
>> +static inline int adv7533_patch_cec_registers(struct adv7511 *adv)
>>   {
>>       return -ENODEV;
>>   }
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> new file mode 100644
>> index 000000000000..74081cbfb5db
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> @@ -0,0 +1,314 @@
>> +/*
>> + * adv7511_cec.c - Analog Devices ADV7511/33 cec driver
>> + *
>> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>> + *
>> + * This program is free software; you may redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + *
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +
>> +#include <media/cec.h>
>> +
>> +#include "adv7511.h"
>> +
>> +#define ADV7511_INT1_CEC_MASK \
>> +    (ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
>> +     ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
>> +
>> +static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>> +{
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    unsigned int val;
>> +
>> +    if (regmap_read(adv7511->regmap_cec,
>> +            ADV7511_REG_CEC_TX_ENABLE + offset, &val))
>> +        return;
>> +
>> +    if ((val & 0x01) == 0)
>> +        return;
>> +
>> +    if (tx_raw_status & 0x10) {
> 
> Should we try to use IRQ1 masks (ADV7511_INT1_CEC_TX_ARBIT_LOST here) to make the
> code more legible?
> 
> Same comments for the rest of this func and adv7511_cec_irq_process below.

Done.

> 
>> +        cec_transmit_attempt_done(adv7511->cec_adap,
>> +                      CEC_TX_STATUS_ARB_LOST);
>> +        return;
>> +    }
>> +    if (tx_raw_status & 0x08) {
>> +        u8 status;
>> +        u8 err_cnt = 0;
>> +        u8 nack_cnt = 0;
>> +        u8 low_drive_cnt = 0;
>> +        unsigned int cnt;
>> +
>> +        /*
>> +         * We set this status bit since this hardware performs
>> +         * retransmissions.
>> +         */
>> +        status = CEC_TX_STATUS_MAX_RETRIES;
>> +        if (regmap_read(adv7511->regmap_cec,
>> +                ADV7511_REG_CEC_TX_LOW_DRV_CNT + offset, &cnt)) {
>> +            err_cnt = 1;
>> +            status |= CEC_TX_STATUS_ERROR;
>> +        } else {
>> +            nack_cnt = cnt & 0xf;
>> +            if (nack_cnt)
>> +                status |= CEC_TX_STATUS_NACK;
>> +            low_drive_cnt = cnt >> 4;
>> +            if (low_drive_cnt)
>> +                status |= CEC_TX_STATUS_LOW_DRIVE;
>> +        }
>> +        cec_transmit_done(adv7511->cec_adap, status,
>> +                  0, nack_cnt, low_drive_cnt, err_cnt);
>> +        return;
>> +    }
>> +    if (tx_raw_status & 0x20) {
>> +        cec_transmit_attempt_done(adv7511->cec_adap, CEC_TX_STATUS_OK);
>> +        return;
>> +    }
>> +}
>> +
>> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>> +{
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    struct cec_msg msg = {};
>> +    unsigned int len;
>> +    unsigned int val;
>> +    u8 i;
>> +
>> +    if (irq1 & 0x38) > +        adv_cec_tx_raw_status(adv7511, irq1);
>> +
>> +    if (!(irq1 & 1))
>> +        return;
>> +
>> +    if (regmap_read(adv7511->regmap_cec,
>> +            ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
>> +        return;
>> +
>> +    msg.len = len & 0x1f;
>> +
>> +    if (msg.len > 16)
>> +        msg.len = 16;
>> +
>> +    if (!msg.len)
>> +        return;
>> +
>> +    for (i = 0; i < msg.len; i++) {
>> +        regmap_read(adv7511->regmap_cec,
>> +                i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
>> +        msg.msg[i] = val;
>> +    }
>> +
>> +    /* toggle to re-enable rx 1 */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
>> +    cec_received_msg(adv7511->cec_adap, &msg);
>> +}
>> +
>> +static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +    struct adv7511 *adv7511 = cec_get_drvdata(adap);
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +
>> +    if (adv7511->i2c_cec == NULL)
>> +        return -EIO;
>> +
>> +    if (!adv7511->cec_enabled_adap && enable) {
>> +        /* power up cec section */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_CLK_DIV + offset,
>> +                   0x03, 0x01);
>> +        /* legacy mode and clear all rx buffers */
>> +        regmap_write(adv7511->regmap_cec,
>> +                 ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
>> +        regmap_write(adv7511->regmap_cec,
>> +                 ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
>> +        /* initially disable tx */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
>> +        /* enabled irqs: */
>> +        /* tx: ready */
>> +        /* tx: arbitration lost */
>> +        /* tx: retry timeout */
>> +        /* rx: ready 1 */
>> +        regmap_update_bits(adv7511->regmap,
>> +                   ADV7511_REG_INT_ENABLE(1), 0x3f,
>> +                   ADV7511_INT1_CEC_MASK);
>> +    } else if (adv7511->cec_enabled_adap && !enable) {
>> +        regmap_update_bits(adv7511->regmap,
>> +                   ADV7511_REG_INT_ENABLE(1), 0x3f, 0);
>> +        /* disable address mask 1-3 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x70, 0x00);
>> +        /* power down cec section */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_CLK_DIV + offset,
>> +                   0x03, 0x00);
>> +        adv7511->cec_valid_addrs = 0;
>> +    }
>> +    adv7511->cec_enabled_adap = enable;
>> +    return 0;
>> +}
>> +
>> +static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>> +{
>> +    struct adv7511 *adv7511 = cec_get_drvdata(adap);
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    unsigned int i, free_idx = ADV7511_MAX_ADDRS;
>> +
>> +    if (!adv7511->cec_enabled_adap)
>> +        return addr == CEC_LOG_ADDR_INVALID ? 0 : -EIO;
>> +
>> +    if (addr == CEC_LOG_ADDR_INVALID) {
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x70, 0);
>> +        adv7511->cec_valid_addrs = 0;
>> +        return 0;
>> +    }
>> +
>> +    for (i = 0; i < ADV7511_MAX_ADDRS; i++) {
>> +        bool is_valid = adv7511->cec_valid_addrs & (1 << i);
>> +
>> +        if (free_idx == ADV7511_MAX_ADDRS && !is_valid)
>> +            free_idx = i;
>> +        if (is_valid && adv7511->cec_addr[i] == addr)
>> +            return 0;
>> +    }
>> +    if (i == ADV7511_MAX_ADDRS) {
>> +        i = free_idx;
>> +        if (i == ADV7511_MAX_ADDRS)
>> +            return -ENXIO;
>> +    }
>> +    adv7511->cec_addr[i] = addr;
>> +    adv7511->cec_valid_addrs |= 1 << i;
>> +
>> +    switch (i) {
>> +    case 0:
>> +        /* enable address mask 0 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x10, 0x10);
>> +        /* set address for mask 0 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
>> +                   0x0f, addr);
>> +        break;
>> +    case 1:
>> +        /* enable address mask 1 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x20, 0x20);
>> +        /* set address for mask 1 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
>> +                   0xf0, addr << 4);
>> +        break;
>> +    case 2:
>> +        /* enable address mask 2 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x40, 0x40);
>> +        /* set address for mask 1 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_2 + offset,
>> +                   0x0f, addr);
>> +        break;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>> +                     u32 signal_free_time, struct cec_msg *msg)
>> +{
>> +    struct adv7511 *adv7511 = cec_get_drvdata(adap);
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    u8 len = msg->len;
>> +    unsigned int i;
>> +
>> +    /*
>> +     * The number of retries is the number of attempts - 1, but retry
>> +     * at least once. It's not clear if a value of 0 is allowed, so
>> +     * let's do at least one retry.
>> +     */
>> +    regmap_update_bits(adv7511->regmap_cec,
>> +               ADV7511_REG_CEC_TX_RETRY + offset,
>> +               0x70, max(1, attempts - 1) << 4);
>> +
>> +    /* blocking, clear cec tx irq status */
>> +    regmap_update_bits(adv7511->regmap, ADV7511_REG_INT(1), 0x38, 0x38);
>> +
>> +    /* write data */ > +    for (i = 0; i < len; i++)
>> +        regmap_write(adv7511->regmap_cec, i + offset, msg->msg[i]);
> 
> Maybe "i + ADV7511_REG_CEC_TX_FRAME_HDR + offset" here for more clarity?

Done.

> 
>> +
>> +    /* set length (data + header) */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_TX_FRAME_LEN + offset, len);
>> +    /* start transmit, enable tx */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_TX_ENABLE + offset, 0x01);
>> +    return 0;
>> +}
>> +
>> +const struct cec_adap_ops adv7511_cec_adap_ops = {
>> +    .adap_enable = adv7511_cec_adap_enable,
>> +    .adap_log_addr = adv7511_cec_adap_log_addr,
>> +    .adap_transmit = adv7511_cec_adap_transmit,
>> +};
>> +
>> +void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset)
>> +{
>> +    regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0);
>> +    /* cec soft reset */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_SOFT_RESET + offset, 0x01);
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
>> +
>> +    /* legacy mode */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
>> +
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_CLK_DIV + offset,
>> +             ((adv7511->cec_clk_freq / 750000) - 1) << 2);
>> +}
>> +
>> +int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
>> +{
>> +    adv7511->cec_clk = devm_clk_get(dev, "cec");
>> +    if (IS_ERR(adv7511->cec_clk)) {
>> +        int ret = PTR_ERR(adv7511->cec_clk);
>> +
>> +        adv7511->cec_clk = NULL;
>> +        return ret;
>> +    }
>> +    clk_prepare_enable(adv7511->cec_clk);
>> +    adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
>> +    return 0;
>> +}
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index f75ab6278113..1bef33e99358 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -11,12 +11,15 @@
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>>   #include <linux/slab.h>
>> +#include <linux/clk.h>
>>     #include <drm/drmP.h>
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_edid.h>
>>   +#include <media/cec.h>
>> +
>>   #include "adv7511.h"
>>     /* ADI recommended values for proper operation. */
>> @@ -339,8 +342,10 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>>            */
>>           regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
>>                    ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
>> -        regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
>> -                 ADV7511_INT1_DDC_ERROR);
>> +        regmap_update_bits(adv7511->regmap,
>> +                   ADV7511_REG_INT_ENABLE(1),
>> +                   ADV7511_INT1_DDC_ERROR,
>> +                   ADV7511_INT1_DDC_ERROR);
>>       }
>>         /*
>> @@ -376,6 +381,9 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
>>       regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>>                  ADV7511_POWER_POWER_DOWN,
>>                  ADV7511_POWER_POWER_DOWN);
>> +    regmap_update_bits(adv7511->regmap,
>> +               ADV7511_REG_INT_ENABLE(1),
>> +               ADV7511_INT1_DDC_ERROR, 0);
>>       regcache_mark_dirty(adv7511->regmap);
>>   }
>>   @@ -426,6 +434,8 @@ static void adv7511_hpd_work(struct work_struct *work)
>>         if (adv7511->connector.status != status) {
>>           adv7511->connector.status = status;
>> +        if (status == connector_status_disconnected)
>> +            cec_phys_addr_invalidate(adv7511->cec_adap);
>>           drm_kms_helper_hotplug_event(adv7511->connector.dev);
>>       }
>>   }
>> @@ -456,6 +466,10 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>>               wake_up_all(&adv7511->wq);
>>       }
>>   +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +    adv7511_cec_irq_process(adv7511, irq1);
>> +#endif
>> +
>>       return 0;
>>   }
>>   @@ -599,6 +613,8 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>         adv7511_set_config_csc(adv7511, connector, adv7511->rgb);
>>   +    cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
>> +
>>       return count;
>>   }
>>   @@ -920,6 +936,84 @@ static void adv7511_uninit_regulators(struct adv7511 *adv)
>>       regulator_bulk_disable(adv->num_supplies, adv->supplies);
>>   }
>>   +static bool adv7533_cec_register_volatile(struct device *dev, unsigned int reg)
>> +{
>> +    switch (reg) {
>> +    case ADV7511_REG_CEC_RX_FRAME_HDR + ADV7533_REG_CEC_OFFSET:
>> +    case ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET...
>> +        ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET + 14:
>> +    case ADV7511_REG_CEC_RX_FRAME_LEN + ADV7533_REG_CEC_OFFSET:
>> +    case ADV7511_REG_CEC_RX_BUFFERS + ADV7533_REG_CEC_OFFSET:
>> +    case ADV7511_REG_CEC_TX_LOW_DRV_CNT + ADV7533_REG_CEC_OFFSET:
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static const struct regmap_config adv7533_cec_regmap_config = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +
>> +    .max_register = 0xff,
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = adv7533_cec_register_volatile,
>> +};
>> +
>> +static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>> +{
> 
> Maybe we could combine the two register_volatile() funcs and the remap_config structs
> for adv7511 and adv7533 by passing (reg + offset) to switch?

How? How would I know in the volatile function whether it is an adv7511 or adv7533?
Is there an easy way to go from the struct device to a struct adv7511?

> 
>> +    switch (reg) {
>> +    case ADV7511_REG_CEC_RX_FRAME_HDR:
>> +    case ADV7511_REG_CEC_RX_FRAME_DATA0...
>> +        ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
>> +    case ADV7511_REG_CEC_RX_FRAME_LEN:
>> +    case ADV7511_REG_CEC_RX_BUFFERS:
>> +    case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static const struct regmap_config adv7511_cec_regmap_config = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +
>> +    .max_register = 0xff,
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = adv7511_cec_register_volatile,
>> +};
>> +
>> +static int adv7511_init_cec_regmap(struct adv7511 *adv)
>> +{
>> +    int ret;
>> +
>> +    adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> +                     adv->i2c_main->addr - 1);
>> +    if (!adv->i2c_cec)
>> +        return -ENOMEM;
>> +
>> +    adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
>> +                           adv->type == ADV7533 ?
>> +                           &adv7533_cec_regmap_config :
>> +                    &adv7511_cec_regmap_config);
>> +    if (IS_ERR(adv->regmap_cec)) {
>> +        ret = PTR_ERR(adv->regmap_cec);
>> +        goto err;
>> +    }
>> +
>> +    if (adv->type == ADV7533) {
>> +        ret = adv7533_patch_cec_registers(adv);
>> +        if (ret)
>> +            goto err;
>> +    }
>> +
>> +    return 0;
>> +err:
>> +    i2c_unregister_device(adv->i2c_cec);
>> +    return ret;
>> +}
>> +
>>   static int adv7511_parse_dt(struct device_node *np,
>>                   struct adv7511_link_config *config)
>>   {
>> @@ -1010,6 +1104,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>       struct device *dev = &i2c->dev;
>>       unsigned int main_i2c_addr = i2c->addr << 1;
>>       unsigned int edid_i2c_addr = main_i2c_addr + 4;
>> +    unsigned int offset;
>>       unsigned int val;
>>       int ret;
>>   @@ -1035,6 +1130,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>           ret = adv7511_parse_dt(dev->of_node, &link_config);
>>       else
>>           ret = adv7533_parse_dt(dev->of_node, adv7511);
>> +
> 
> This line seems unnecessary.

Removed.

> 
>>       if (ret)
>>           return ret;
>>   @@ -1093,11 +1189,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>           goto uninit_regulators;
>>       }
>>   -    if (adv7511->type == ADV7533) {
>> -        ret = adv7533_init_cec(adv7511);
>> -        if (ret)
>> -            goto err_i2c_unregister_edid;
>> -    }
>> +    ret = adv7511_init_cec_regmap(adv7511);
>> +    if (ret)
>> +        goto err_i2c_unregister_edid;
>>         INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>   @@ -1112,10 +1206,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>               goto err_unregister_cec;
>>       }
>>   -    /* 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);
>> @@ -1134,10 +1224,39 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>         adv7511_audio_init(dev, adv7511);
>>   +    offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
>> +
>> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +    ret = adv7511_cec_parse_dt(dev, adv7511);
>> +    if (ret)
>> +        goto err_unregister_cec;
>> +
>> +    adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops,
>> +        adv7511, dev_name(&i2c->dev), CEC_CAP_TRANSMIT |
>> +        CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH | CEC_CAP_RC,
>> +        ADV7511_MAX_ADDRS);
>> +    ret = PTR_ERR_OR_ZERO(adv7511->cec_adap);
>> +    if (ret)
>> +        goto err_unregister_cec;
>> +
>> +    adv7511_cec_init(adv7511, offset);
>> +
>> +    ret = cec_register_adapter(adv7511->cec_adap, &i2c->dev);
>> +    if (ret) {
>> +        cec_delete_adapter(adv7511->cec_adap);
>> +        goto err_unregister_cec;
>> +    }
> 
> We could ideally put this code in a single func and make adv7511_cec_init,
> adv7511_cec_parse_dt and adv7511_cec_adap_ops within the scope of adv7511_cec.c.
> It's not necessary to do, though.

Done. Not sure why I didn't do that in the first place...

> 
>> +#else
>> +    regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>> +             ADV7511_CEC_CTRL_POWER_DOWN);
>> +#endif
>> +
>>       return 0;
>>     err_unregister_cec:
>> -    adv7533_uninit_cec(adv7511);
>> +    i2c_unregister_device(adv7511->i2c_cec);
>> +    if (adv7511->cec_clk)
>> +        clk_disable_unprepare(adv7511->cec_clk);
>>   err_i2c_unregister_edid:
>>       i2c_unregister_device(adv7511->i2c_edid);
>>   uninit_regulators:
>> @@ -1150,10 +1269,11 @@ static int adv7511_remove(struct i2c_client *i2c)
>>   {
>>       struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>>   -    if (adv7511->type == ADV7533) {
>> +    if (adv7511->type == ADV7533)
>>           adv7533_detach_dsi(adv7511);
>> -        adv7533_uninit_cec(adv7511);
>> -    }
>> +    i2c_unregister_device(adv7511->i2c_cec);
>> +    if (adv7511->cec_clk)
>> +        clk_disable_unprepare(adv7511->cec_clk);
>>         adv7511_uninit_regulators(adv7511);
>>   @@ -1161,6 +1281,8 @@ static int adv7511_remove(struct i2c_client *i2c)
>>         adv7511_audio_exit(adv7511);
>>   +    cec_unregister_adapter(adv7511->cec_adap);
>> +
>>       i2c_unregister_device(adv7511->i2c_edid);
>>         kfree(adv7511->edid);
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> index ac804f81e2f6..0e173abb913c 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> @@ -145,37 +145,11 @@ int adv7533_patch_registers(struct adv7511 *adv)
>>                        ARRAY_SIZE(adv7533_fixed_registers));
>>   }
>>   -void adv7533_uninit_cec(struct adv7511 *adv)
>> +int adv7533_patch_cec_registers(struct adv7511 *adv)
>>   {
>> -    i2c_unregister_device(adv->i2c_cec);
>> -}
>> -
>> -int adv7533_init_cec(struct adv7511 *adv)
>> -{
>> -    int ret;
>> -
>> -    adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> -                     adv->i2c_main->addr - 1);
>> -    if (!adv->i2c_cec)
>> -        return -ENOMEM;
>> -
>> -    adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
>> -                    &adv7533_cec_regmap_config);
> 
> adv7533_cec_regmap_config struct isn't needed in the file anymore, that too
> can be deleted.

Oops, must have missed that. Removed.

> 
> Looks good to me otherwise.

Thanks!

I will test the dragonboard again on Monday using your tree. Strange.

Regards,

	Hans

> 
> Thanks,
> Archit
> 
>> -    if (IS_ERR(adv->regmap_cec)) {
>> -        ret = PTR_ERR(adv->regmap_cec);
>> -        goto err;
>> -    }
>> -
>> -    ret = regmap_register_patch(adv->regmap_cec,
>> +    return regmap_register_patch(adv->regmap_cec,
>>                       adv7533_cec_fixed_registers,
>>                       ARRAY_SIZE(adv7533_cec_fixed_registers));
>> -    if (ret)
>> -        goto err;
>> -
>> -    return 0;
>> -err:
>> -    adv7533_uninit_cec(adv);
>> -    return ret;
>>   }
>>     int adv7533_attach_dsi(struct adv7511 *adv)
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux