From: Hans Verkuil <hverkuil@xs4all.nl>
To: Dariusz Marcinkiewicz <darekm@google.com>
Cc: linux-media@vger.kernel.org, hans.verkuil@cisco.com,
linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2] media: cec: expose HDMI connector to CEC dev mapping
Date: Thu, 16 May 2019 16:21:58 +0200 [thread overview]
Message-ID: <cea89a32-c04d-1acb-5b6b-aeb8dda3fccc@xs4all.nl> (raw)
In-Reply-To: <CALFZZQF9qJbDsG1nzdjTxaG=89Pu0k=pxg3TChMQcf+hDxaeuw@mail.gmail.com>
On 5/16/19 4:10 PM, Dariusz Marcinkiewicz wrote:
> Hi Hans.
>
> From: Hans Verkuil <hverkuil@xs4all.nl>
> Date: Thu, May 9, 2019 at 11:31 AM
> To: Dariusz Marcinkiewicz
> Cc: <linux-media@vger.kernel.org>, <hans.verkuil@cisco.com>,
> <linux-kernel@vger.kernel.org>
>
>> On 5/9/19 9:52 AM, Dariusz Marcinkiewicz wrote:
>>> Hi Hans.
>>>
>>> On Wed, Apr 24, 2019 at 2:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> Hi Dariusz,
>>>>
>>>> This is getting close, so I think for the next version you can drop
>>>> the RFC tag.
>>>>
>>>> Some comments:
>>>>
>>> ...
>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
>>>>> @@ -261,7 +261,7 @@ static int dw_hdmi_cec_probe(struct platform_device *pdev)
>>>>> cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi",
>>>>> CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT |
>>>>> CEC_CAP_RC | CEC_CAP_PASSTHROUGH,
>>>>> - CEC_MAX_LOG_ADDRS);
>>>>> + CEC_MAX_LOG_ADDRS, NULL);
>>>>
>>>> Hmm, the connector information is actually available through cec->hdmi.
>>>>
>>>> I think it would make sense to create a helper function that fills in
>>>> struct cec_connector_info based on a struct drm_connector pointer.
>>>> And add a function to drivers/gpu/drm/bridge/synopsys/dw-hdmi.c that
>>>> dw-hdmi-cec.c can call that does the same.
>>>
>>> Looking at the code here, is the connector info guaranteed to be
>>> available at the time cec_allocate_adapter is called here?
>>> drm_connector won't be initialized until dw_hdmi_bridge_attach is
>>> called, which happens after the cec platform device is created.
>>
>> Good point. The creation of the cec platform device should probably
>> be moved to dw_hdmi_bridge_attach.
>>
>>> ...
>>>>> priv->adap = cec_allocate_adapter(&tda9950_cec_ops, priv, "tda9950",
>>>>> CEC_CAP_DEFAULTS,
>>>>> - CEC_MAX_LOG_ADDRS);
>>>>> + CEC_MAX_LOG_ADDRS,
>>>>> + NULL);
>>>>
>>>> Here too the drm_connector can be found via struct tda9950_glue.
>>>> So it is easy to provide proper connector information.
>>>
>>> The same concern as with the comment before.
>>
>> Same solution: this has to be moved.
>>
>> I have hardware to test patches for both drivers. It might take 2-3 weeks
>> before I can test as I don't always has access to the hardware, but at
>> least I can verify that moving this code won't break anything.
>>
>> It's best to first move the code in separate patches before applying the
>> "expose HDMI connector to CEC dev mapping" patch on top of them.
>>
>
> I've submitted another revision of the changes, with those 2 patches
> added on top. Hope that is ok.
>
> Please take a look. It would be great if you could give those 2
> patches a go on an actual hardware.
I should be able to test the dw-hdmi patch next week, but testing the tda9950 patch
has to wait until the week after that.
Regards,
Hans
>
> Thank you and best regards.
>
prev parent reply other threads:[~2019-05-16 14:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-16 8:38 [RFC PATCH v2] media: cec: expose HDMI connector to CEC dev mapping Dariusz Marcinkiewicz
2019-04-24 12:09 ` Hans Verkuil
2019-05-09 7:52 ` Dariusz Marcinkiewicz
2019-05-09 9:31 ` Hans Verkuil
2019-05-16 14:10 ` Dariusz Marcinkiewicz
2019-05-16 14:21 ` Hans Verkuil [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cea89a32-c04d-1acb-5b6b-aeb8dda3fccc@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=darekm@google.com \
--cc=hans.verkuil@cisco.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).