Message-ID: <c8f22d07-6445-f297-c445-87af3f2461da@xs4all.nl>
Date:   Wed, 27 Mar 2019 14:47:23 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Neil Armstrong <narmstrong@...libre.com>, mchehab@...nel.org
Cc:     linux-amlogic@...ts.infradead.org, linux-media@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] media: platform: meson: Add Amlogic Meson G12A AO CEC
 Controller driver

On 3/27/19 2:19 PM, Neil Armstrong wrote:
> Hi Hans,
> 
> On 27/03/2019 13:52, Hans Verkuil wrote:
>> On 3/25/19 6:35 PM, Neil Armstrong wrote:
>>> The Amlogic G12A SoC embeds a second CEC controller with a totally
>>> different design.
>>>
>>> The two controller can work in the same time since the CEC line can
>>> be set to two different pins on the two controllers.
>>>
>>> This second CEC controller is documented as "AO-CEC-B", thus the
>>> registers will be named "CECB_" to differenciate with the other
>>> AO-CEC driver.
>>>
>>> Unlike the other AO-CEC controller, this one takes the Oscillator
>>> clock as input and embeds a dual-divider to provide a precise
>>> 32768Hz clock for communication. This is handled by registering
>>> a clock in the driver.
>>>
>>> Unlike the other AO-CEC controller, this controller supports setting
>>> up to 15 logical adresses and supports the signal_free_time settings
>>> in the transmit function.
>>>
>>> Unfortunately, this controller does not support "monitor" mode.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
>>> ---
>>>  drivers/media/platform/Kconfig             |  13 +
>>>  drivers/media/platform/meson/Makefile      |   1 +
>>>  drivers/media/platform/meson/ao-cec-g12a.c | 783 +++++++++++++++++++++
>>>  3 files changed, 797 insertions(+)
>>>  create mode 100644 drivers/media/platform/meson/ao-cec-g12a.c
>>>
>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>> index 4acbed189644..92ea07ddc609 100644
>>> --- a/drivers/media/platform/Kconfig
>>> +++ b/drivers/media/platform/Kconfig
>>> @@ -578,6 +578,19 @@ config VIDEO_MESON_AO_CEC
>>>  	  generic CEC framework interface.
>>>  	  CEC bus is present in the HDMI connector and enables communication
>>>  
>>> +config VIDEO_MESON_G12A_AO_CEC
>>> +	tristate "Amlogic Meson G12A AO CEC driver"
>>> +	depends on ARCH_MESON || COMPILE_TEST
>>> +	select CEC_CORE
>>> +	select CEC_NOTIFIER
>>> +	---help---
>>> +	  This is a driver for Amlogic Meson G12A SoCs AO CEC interface.
>>> +	  This driver if for the new AO-CEC module found in G12A SoCs,
>>> +	  usually named AO_CEC_B in documentation.
>>> +	  It uses the generic CEC framework interface.
>>> +	  CEC bus is present in the HDMI connector and enables communication
>>> +	  between compatible devices.
>>> +
>>>  config CEC_GPIO
>>>  	tristate "Generic GPIO-based CEC driver"
>>>  	depends on PREEMPT || COMPILE_TEST
>>> diff --git a/drivers/media/platform/meson/Makefile b/drivers/media/platform/meson/Makefile
>>> index 597beb8f34d1..f611c23c3718 100644
>>> --- a/drivers/media/platform/meson/Makefile
>>> +++ b/drivers/media/platform/meson/Makefile
>>> @@ -1 +1,2 @@
>>>  obj-$(CONFIG_VIDEO_MESON_AO_CEC)	+= ao-cec.o
>>> +obj-$(CONFIG_VIDEO_MESON_G12A_AO_CEC)	+= ao-cec-g12a.o
>>> diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c
>>> new file mode 100644
>>> index 000000000000..8977ae994164
>>> --- /dev/null
>>> +++ b/drivers/media/platform/meson/ao-cec-g12a.c
>>> @@ -0,0 +1,783 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Driver for Amlogic Meson AO CEC G12A Controller
>>> + *
>>> + * Copyright (C) 2017 Amlogic, Inc. All rights reserved
>>> + * Copyright (C) 2019 BayLibre, SAS
>>> + * Author: Neil Armstrong <narmstrong@...libre.com>
>>> + */
>>> +
> 
> [...]
> 
>>> +
>>> +static irqreturn_t meson_ao_cec_g12a_irq_thread(int irq, void *data)
>>> +{
>>> +	struct meson_ao_cec_g12a_device *ao_cec = data;
>>> +	u32 stat;
>>> +
>>> +	regmap_read(ao_cec->regmap, CECB_INTR_STAT_REG, &stat);
>>> +	regmap_write(ao_cec->regmap, CECB_INTR_CLR_REG, stat);
>>> +
>>> +	if (stat & CECB_INTR_DONE)
>>> +		cec_transmit_attempt_done(ao_cec->adap, CEC_TX_STATUS_OK);
>>> +
>>> +	if (stat & CECB_INTR_EOM)
>>> +		meson_ao_cec_g12a_irq_rx(ao_cec);
>>> +
>>> +	if (stat & CECB_INTR_NACK)
>>> +		cec_transmit_attempt_done(ao_cec->adap, CEC_TX_STATUS_NACK);
>>> +
>>> +	if (stat & CECB_INTR_ARB_LOSS) {
>>> +		regmap_write(ao_cec->regmap_cec, CECB_TX_CNT, 0);
>>> +		regmap_update_bits(ao_cec->regmap_cec, CECB_CTRL,
>>> +				   CECB_CTRL_SEND | CECB_CTRL_TYPE, 0);
>>> +		cec_transmit_attempt_done(ao_cec->adap, CEC_TX_STATUS_ARB_LOST);
>>> +	}
>>> +
>>> +	if (stat & CECB_INTR_INITIATOR_ERR)
>>> +		cec_transmit_attempt_done(ao_cec->adap, CEC_TX_STATUS_NACK);
>>> +
>>> +	if (stat & CECB_INTR_FOLLOWER_ERR) {
>>> +		regmap_write(ao_cec->regmap_cec, CECB_LOCK_BUF, 0);
>>> +		cec_transmit_attempt_done(ao_cec->adap, CEC_TX_STATUS_NACK);
>>
>> Any idea what CECB_INTR_INITIATOR_ERR and CECB_INTR_FOLLOWER_ERR actually
>> mean? I.e. is returning NACK right here, or would TX_STATUS_ERROR be a
>> better choice? I invented that status precisely for cases where there is
>> an error, but we don't know what it means.
>>
>> Are you sure that CECB_INTR_FOLLOWER_ERR applies to a transmit and not a
>> receive? 'Follower' suggests that some error occurred while receiving
>> a message. If I am right, then just ignore it.
> 
> Vendor describes it as "Follower Error interrupt flag status", indeed it
> would apply to a receive nack. I'll ignore it.
> 
>>
>> Regarding CECB_INTR_INITIATOR_ERR: I suspect that this indicates a LOW
>> DRIVE error situation, in which case you'd return that transmit status.
> 
> Vendor describes it as "Initiator Error interrupt flag status", I suspect it
> means a generic bus error, and it should certainly be a low drive situation.
> 
> Would CEC_TX_STATUS_ERROR be more appropriate since we don't know exactly ?

Yes, that would be better.

Regards,

	Hans

> 
>>
>>> +	}
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int
>>> +meson_ao_cec_g12a_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>> +{
>>> +	struct meson_ao_cec_g12a_device *ao_cec = adap->priv;
>>> +	int ret = 0;
>>> +
>>> +	if (logical_addr == CEC_LOG_ADDR_INVALID) {
>>> +		ret = regmap_write(ao_cec->regmap_cec, CECB_LADD_LOW, 0);
>>> +		ret = regmap_write(ao_cec->regmap_cec, CECB_LADD_HIGH, 0);
>>
>> Just ignore the error codes and return 0 here.
>>
>> The CEC core will WARN if this function returns anything other than 0
>> when invalidating the logical addresses. It assumes this will always
>> succeed.
> 
> Ok
> 
>>
>>> +	} else if (logical_addr < 8) {
>>> +		ret = regmap_update_bits(ao_cec->regmap_cec, CECB_LADD_LOW,
>>> +					 BIT(logical_addr),
>>> +					 BIT(logical_addr));
>>> +	} else {
>>> +		ret = regmap_update_bits(ao_cec->regmap_cec, CECB_LADD_HIGH,
>>> +					 BIT(logical_addr - 8),
>>> +					 BIT(logical_addr - 8));
>>> +	}
>>> +
>>> +	/* Always set Broadcast/Unregistered 15 address */
>>> +	ret |= regmap_update_bits(ao_cec->regmap_cec, CECB_LADD_HIGH,
>>
>> I'd just do:
>>
>> 	if (!ret)
>> 		ret = regmap_...
>>
>> Error codes are not a bitmask after all.
>>
>> I see that elsewhere as well.
>>
>> It's OK to use |=, but then when you return from the function you
>> would have to do something like:
>>
>> 	return ret ? -EIO : 0;
> 
> I'll do this when errors can only come from regmap, and check each
> calls for other situations.
> 
>>
>> Regards,
>>
>> 	Hans
> 
> Thanks,
> Neil
> 
>>
>>> +				  BIT(CEC_LOG_ADDR_UNREGISTERED - 8),
>>> +				  BIT(CEC_LOG_ADDR_UNREGISTERED - 8));
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int meson_ao_cec_g12a_transmit(struct cec_adapter *adap, u8 attempts,
>>> +				 u32 signal_free_time, struct cec_msg *msg)
>>> +{
>>> +	struct meson_ao_cec_g12a_device *ao_cec = adap->priv;
>>> +	unsigned int type;
>>> +	int ret = 0;
>>> +	u32 val;
>>> +	int i;
>>> +
>>> +	/* Check if RX is in progress */
>>> +	ret = regmap_read(ao_cec->regmap_cec, CECB_LOCK_BUF, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +	if (val & CECB_LOCK_BUF_EN)
>>> +		return -EBUSY;
>>> +
>>> +	/* Check if TX Busy */
>>> +	ret = regmap_read(ao_cec->regmap_cec, CECB_CTRL, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +	if (val & CECB_CTRL_SEND)
>>> +		return -EBUSY;
>>> +
>>> +	switch (signal_free_time) {
>>> +	case CEC_SIGNAL_FREE_TIME_RETRY:
>>> +		type = CECB_CTRL_TYPE_RETRY;
>>> +		break;
>>> +	case CEC_SIGNAL_FREE_TIME_NEXT_XFER:
>>> +		type = CECB_CTRL_TYPE_NEXT;
>>> +		break;
>>> +	case CEC_SIGNAL_FREE_TIME_NEW_INITIATOR:
>>> +	default:
>>> +		type = CECB_CTRL_TYPE_NEW;
>>> +		break;
>>> +	}
>>> +
>>> +	for (i = 0; i < msg->len; i++)
>>> +		ret |= regmap_write(ao_cec->regmap_cec, CECB_TX_DATA00 + i,
>>> +				    msg->msg[i]);
>>> +
>>> +	ret |= regmap_write(ao_cec->regmap_cec, CECB_TX_CNT, msg->len);
>>> +	ret = regmap_update_bits(ao_cec->regmap_cec, CECB_CTRL,
>>> +				 CECB_CTRL_SEND |
>>> +				 CECB_CTRL_TYPE,
>>> +				 CECB_CTRL_SEND |
>>> +				 FIELD_PREP(CECB_CTRL_TYPE, type));
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int meson_ao_cec_g12a_adap_enable(struct cec_adapter *adap, bool enable)
>>> +{
>>> +	struct meson_ao_cec_g12a_device *ao_cec = adap->priv;
>>> +
>>> +	meson_ao_cec_g12a_irq_setup(ao_cec, false);
>>> +
>>> +	regmap_update_bits(ao_cec->regmap, CECB_GEN_CNTL_REG,
>>> +			   CECB_GEN_CNTL_RESET, CECB_GEN_CNTL_RESET);
>>> +
>>> +	if (!enable)
>>> +		return 0;
>>> +
>>> +	/* Setup Filter */
>>> +	regmap_update_bits(ao_cec->regmap, CECB_GEN_CNTL_REG,
>>> +			   CECB_GEN_CNTL_FILTER_TICK_SEL |
>>> +			   CECB_GEN_CNTL_FILTER_DEL,
>>> +			   FIELD_PREP(CECB_GEN_CNTL_FILTER_TICK_SEL,
>>> +				      CECB_GEN_CNTL_FILTER_TICK_1US) |
>>> +			   FIELD_PREP(CECB_GEN_CNTL_FILTER_DEL, 7));
>>> +
>>> +	/* Enable System Clock */
>>> +	regmap_update_bits(ao_cec->regmap, CECB_GEN_CNTL_REG,
>>> +			   CECB_GEN_CNTL_SYS_CLK_EN,
>>> +			   CECB_GEN_CNTL_SYS_CLK_EN);
>>> +
>>> +	/* Enable gated clock (Normal mode). */
>>> +	regmap_update_bits(ao_cec->regmap, CECB_GEN_CNTL_REG,
>>> +			   CECB_GEN_CNTL_CLK_CTRL_MASK,
>>> +			    FIELD_PREP(CECB_GEN_CNTL_CLK_CTRL_MASK,
>>> +				       CECB_GEN_CNTL_CLK_ENABLE));
>>> +
>>> +	/* Release Reset */
>>> +	regmap_update_bits(ao_cec->regmap, CECB_GEN_CNTL_REG,
>>> +			   CECB_GEN_CNTL_RESET, 0);
>>> +
>>> +	meson_ao_cec_g12a_irq_setup(ao_cec, true);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct cec_adap_ops meson_ao_cec_g12a_ops = {
>>> +	.adap_enable = meson_ao_cec_g12a_adap_enable,
>>> +	.adap_log_addr = meson_ao_cec_g12a_set_log_addr,
>>> +	.adap_transmit = meson_ao_cec_g12a_transmit,
>>> +};
>>> +
>>> +static int meson_ao_cec_g12a_probe(struct platform_device *pdev)
>>> +{
>>> +	struct meson_ao_cec_g12a_device *ao_cec;
>>> +	struct platform_device *hdmi_dev;
>>> +	struct device_node *np;
>>> +	struct resource *res;
>>> +	void __iomem *base;
>>> +	int ret, irq;
>>> +
>>> +	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
>>> +	if (!np) {
>>> +		dev_err(&pdev->dev, "Failed to find hdmi node\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	hdmi_dev = of_find_device_by_node(np);
>>> +	of_node_put(np);
>>> +	if (hdmi_dev == NULL)
>>> +		return -EPROBE_DEFER;
>>> +
>>> +	put_device(&hdmi_dev->dev);
>>> +	ao_cec = devm_kzalloc(&pdev->dev, sizeof(*ao_cec), GFP_KERNEL);
>>> +	if (!ao_cec)
>>> +		return -ENOMEM;
>>> +
>>> +	spin_lock_init(&ao_cec->cec_reg_lock);
>>> +	ao_cec->pdev = pdev;
>>> +
>>> +	ao_cec->notify = cec_notifier_get(&hdmi_dev->dev);
>>> +	if (!ao_cec->notify)
>>> +		return -ENOMEM;
>>> +
>>> +	ao_cec->adap = cec_allocate_adapter(&meson_ao_cec_g12a_ops, ao_cec,
>>> +					    "meson_g12a_ao_cec",
>>> +					    CEC_CAP_DEFAULTS,
>>> +					    CEC_MAX_LOG_ADDRS);
>>> +	if (IS_ERR(ao_cec->adap)) {
>>> +		ret = PTR_ERR(ao_cec->adap);
>>> +		goto out_probe_notify;
>>> +	}
>>> +
>>> +	ao_cec->adap->owner = THIS_MODULE;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	base = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(base)) {
>>> +		ret = PTR_ERR(base);
>>> +		goto out_probe_adapter;
>>> +	}
>>> +
>>> +	ao_cec->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>>> +					       &meson_ao_cec_g12a_regmap_conf);
>>> +	if (IS_ERR(ao_cec->regmap)) {
>>> +		ret = PTR_ERR(ao_cec->regmap);
>>> +		goto out_probe_adapter;
>>> +	}
>>> +
>>> +	ao_cec->regmap_cec = devm_regmap_init(&pdev->dev, NULL, ao_cec,
>>> +					   &meson_ao_cec_g12a_cec_regmap_conf);
>>> +	if (IS_ERR(ao_cec->regmap_cec)) {
>>> +		ret = PTR_ERR(ao_cec->regmap_cec);
>>> +		goto out_probe_adapter;
>>> +	}
>>> +
>>> +	irq = platform_get_irq(pdev, 0);
>>> +	ret = devm_request_threaded_irq(&pdev->dev, irq,
>>> +					meson_ao_cec_g12a_irq,
>>> +					meson_ao_cec_g12a_irq_thread,
>>> +					0, NULL, ao_cec);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "irq request failed\n");
>>> +		goto out_probe_adapter;
>>> +	}
>>> +
>>> +	ao_cec->oscin = devm_clk_get(&pdev->dev, "oscin");
>>> +	if (IS_ERR(ao_cec->oscin)) {
>>> +		dev_err(&pdev->dev, "oscin clock request failed\n");
>>> +		ret = PTR_ERR(ao_cec->oscin);
>>> +		goto out_probe_adapter;
>>> +	}
>>> +
>>> +	ret = meson_ao_cec_g12a_setup_clk(ao_cec);
>>> +	if (ret)
>>> +		goto out_probe_clk;
>>> +
>>> +	ret = clk_prepare_enable(ao_cec->core);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "core clock enable failed\n");
>>> +		goto out_probe_clk;
>>> +	}
>>> +
>>> +	device_reset_optional(&pdev->dev);
>>> +
>>> +	platform_set_drvdata(pdev, ao_cec);
>>> +
>>> +	ret = cec_register_adapter(ao_cec->adap, &pdev->dev);
>>> +	if (ret < 0) {
>>> +		cec_notifier_put(ao_cec->notify);
>>> +		goto out_probe_core_clk;
>>> +	}
>>> +
>>> +	/* Setup Hardware */
>>> +	regmap_write(ao_cec->regmap, CECB_GEN_CNTL_REG, CECB_GEN_CNTL_RESET);
>>> +
>>> +	cec_register_cec_notifier(ao_cec->adap, ao_cec->notify);
>>> +
>>> +	return 0;
>>> +
>>> +out_probe_core_clk:
>>> +	clk_disable_unprepare(ao_cec->core);
>>> +
>>> +out_probe_clk:
>>> +	clk_disable_unprepare(ao_cec->oscin);
>>> +
>>> +out_probe_adapter:
>>> +	cec_delete_adapter(ao_cec->adap);
>>> +
>>> +out_probe_notify:
>>> +	cec_notifier_put(ao_cec->notify);
>>> +
>>> +	dev_err(&pdev->dev, "CEC controller registration failed\n");
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int meson_ao_cec_g12a_remove(struct platform_device *pdev)
>>> +{
>>> +	struct meson_ao_cec_g12a_device *ao_cec = platform_get_drvdata(pdev);
>>> +
>>> +	clk_disable_unprepare(ao_cec->oscin);
>>> +
>>> +	cec_unregister_adapter(ao_cec->adap);
>>> +
>>> +	cec_notifier_put(ao_cec->notify);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id meson_ao_cec_g12a_of_match[] = {
>>> +	{ .compatible = "amlogic,meson-g12a-ao-cec-b", },
>>> +	{ /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, meson_ao_cec_g12a_of_match);
>>> +
>>> +static struct platform_driver meson_ao_cec_g12a_driver = {
>>> +	.probe   = meson_ao_cec_g12a_probe,
>>> +	.remove  = meson_ao_cec_g12a_remove,
>>> +	.driver  = {
>>> +		.name = "meson-ao-cec-g12a",
>>> +		.of_match_table = of_match_ptr(meson_ao_cec_g12a_of_match),
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(meson_ao_cec_g12a_driver);
>>> +
>>> +MODULE_DESCRIPTION("Meson AO CEC G12A Controller driver");
>>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@...libre.com>");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>> Regards,
>>
>> 	Hans
>>
> 

Powered by blists - more mailing lists