[PATCH v2 1/2] platform: Add Amlogic Meson AO CEC Controller driver

[narmstrong@baylibre.com (Neil Armstrong)]

On 07/25/2017 03:45 PM, Hans Verkuil wrote:
> On 07/25/17 14:34, Neil Armstrong wrote:
>> Hi Hans,
> 
>>>> +static int meson_ao_cec_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct meson_ao_cec_device *ao_cec;
>>>> +	struct platform_device *hdmi_dev;
>>>> +	struct device_node *np;
>>>> +	struct resource *res;
>>>> +	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);
>>>> +	if (hdmi_dev == NULL)
>>>> +		return -EPROBE_DEFER;
>>>> +
>>>> +	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->notify = cec_notifier_get(&hdmi_dev->dev);
>>>> +	if (!ao_cec->notify)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ao_cec->adap = cec_allocate_adapter(&meson_ao_cec_ops, ao_cec,
>>>> +					    "meson_ao_cec",
>>>> +					    CEC_CAP_LOG_ADDRS |
>>>> +					    CEC_CAP_TRANSMIT |
>>>> +					    CEC_CAP_RC |
>>>> +					    CEC_CAP_PASSTHROUGH,
>>>> +					    1); /* Use 1 for now */
>>>
>>> I recommend that you add support for 2 logical addresses. More isn't allowed
>>> by the CEC 2.0 spec anyway (no such restriction for CEC 1.4, but more than
>>> two really isn't needed).
>>
>> I know, but in the "communication" register with the suspend/poweroff firmware
>> that  handles the wake up, only a single logical address is supported...
>>
>> What should I do in this case ? Which logical adress should I pass to the firmware when implementing ir ?
> 
> Ah, OK. Interesting.
> 
> From cec-adap.c:
> 
>                 if (log_addrs->num_log_addrs == 2) {
>                         if (!(type_mask & ((1 << CEC_LOG_ADDR_TYPE_AUDIOSYSTEM) |
>                                            (1 << CEC_LOG_ADDR_TYPE_TV)))) {
>                                 dprintk(1, "two LAs is only allowed for audiosystem and TV\n");
>                                 return -EINVAL;
>                         }
>                         if (!(type_mask & ((1 << CEC_LOG_ADDR_TYPE_PLAYBACK) |
>                                            (1 << CEC_LOG_ADDR_TYPE_RECORD)))) {
>                                 dprintk(1, "an audiosystem/TV can only be combined with record or playback\n");
>                                 return -EINVAL;
>                         }
>                 }
> 
> So you would store the TV or AUDIOSYSTEM logical address in the firmware, since those
> describe the system best.
> 
> I.e. it is a TV/Audiosystem with recording/playback capabilities.
> 
> The problem is that for CEC 1.4 no such restriction is imposed (the test above is
> specific to CEC 2.0). But I think it makes sense to just check if TV/Audiosystem
> is selected and pick that as the LA to store in the firmware, and otherwise just
> pick the first LA (log_addr[0]).


Ok I'll add support for dual LA, and I'll do this LA selection when I'll add firmware support.

Thanks,
Neil


> Regards,
> 
> 	Hans
>