Hi Hans,
On Wednesday 02 Aug 2017 15:34:33 Hans Verkuil wrote:
> On 08/02/17 15:27, Russell King - ARM Linux wrote:
> > On Wed, Aug 02, 2017 at 04:14:34PM +0300, Laurent Pinchart wrote:
> >> On Wednesday 02 Aug 2017 08:47:23 Hans Verkuil wrote:
> >>> On 08/02/2017 12:32 AM, Laurent Pinchart wrote:
> >>>>> +
> >>>>> + cec_register_cec_notifier(cec->adap, cec->notify);
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static int dw_hdmi_cec_remove(struct platform_device *pdev)
> >>>>> +{
> >>>>> + struct dw_hdmi_cec *cec = platform_get_drvdata(pdev);
> >>>>> +
> >>>>> + cec_unregister_adapter(cec->adap);
> >>>>> + cec_notifier_put(cec->notify);
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static struct platform_driver dw_hdmi_cec_driver = {
> >>>>> + .probe = dw_hdmi_cec_probe,
> >>>>> + .remove = dw_hdmi_cec_remove,
> >>>>> + .driver = {
> >>>>> + .name = "dw-hdmi-cec",
> >>>>> + },
> >>>>> +};
> >>>>> +module_platform_driver(dw_hdmi_cec_driver);
> >>>>
> >>>> Is there a particular reason why this has to be a separate module
> >>>> instead of simply calling the CEC init/cleanup functions directly from
> >>>> the main dw-hdmi driver ?
> >>>
> >>> Not all SoCs that use dw-hdmi also use the dw-hdmi CEC implementation.
> >>> Some use their own implementation (amlogic).
> >>
> >> Lovely. Of course we need to reinvent the wheel every time, where would
> >> the fun be otherwise ?
> >>
> >>> So by implementing the cec-notifier in the dw-hdmi driver and keeping
> >>> dw-hdmi CEC separate you can easily choose whether or not you want to
> >>> use this CEC driver or another SoC CEC driver.
> >>
> >> I'm certainly fine with such a split, but I don't think it requires a
> >> separate platform_driver. We could use a similar approach as with the
> >> HDMI PHY that can also differ between SoCs. The PHY is identified at
> >> runtime when possible, and the SoC-specific glue code can override that
> >> with a few data fields and function pointers.
> >
> > Excuse me if I completely lose interest in reworking the driver at this
> > point, as it's enough of an effort to follow the churn in CEC from one
> > kernel version to another. I'm not about to rewrite the driver and
> > restart the review cycle from scratch and then have several iterations
> > of having to update it as CEC continues to evolve.
> >
> > Let's get this driver in mainline, and then if we want further changes
> > we can do that later.
>
> +1
>
> I also don't think the phy idea applies here. CEC implementations range from
> tightly coupled to the HDMI implementation to a completely independent i2c
> device that is unrelated to any HDMI receiver/transmitter. And various
> shades in between those extremes.
Then it should be even simpler. Instead of creating a separate platform device
for the DW HDMI CEC, we can just call the DW HDMI CEC driver init/cleanup
functions directly when the IP core implements CEC. When it doesn't, a
separate device will be described in DT (at least to my understanding,
otherwise where would it come from ?) and a separate driver would handle it.
> The only thing that is needed is that the CEC device needs to be informed
> when an EDID is read and when the hotplug disappears. The CEC notifier does
> just that and it is already in use, so it is nothing new. No need to rework
> this.
The notifier implementation in patch 1/4 looks good to me (except for the
missing cec_notifier_put() call that I mentioned in my review).
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
