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
