Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

Hi Jose,

On 06/28/2017 11:25 AM, Jose Abreu wrote:
> On 27-06-2017 21:34, Sylwester Nawrocki wrote:
> > On 06/27/2017 10:43 AM, Jose Abreu wrote:
> > > On 25-06-2017 22:13, Sylwester Nawrocki wrote:
> > > > On 06/20/2017 07:26 PM, Jose Abreu wrote:
> > > > > This is an initial submission for the Synopsys Designware HDMI RX
> > > > > Controller Driver. This driver interacts with a phy driver so that
> > > > > a communication between them is created and a video pipeline is
> > > > > configured.
> > > > > Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>

> The modules are installed but I think I don't have udev :/ I'm
> running this on an embedded platform called ARC AXS and I'm using
> buildroot with minimal options.

OK, then let's keep this request_module.

> > > > > +static int dw_hdmi_v4l2_notify_complete(struct v4l2_async_notifier *notifier)
> > > > > +{
> > > > > +	struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier);
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = v4l2_device_register_subdev_nodes(&dw_dev->v4l2_dev);
> > > > There shouldn't be multiple struct v4l2_device instances, instead we should
> > > > have only one created by the main driver. AFAIU, in your case it would be
> > > > driver associated with the dw-hdmi-soc DT node.  And normally such a top level
> > > > driver creates subdev device nodes when its all required sub-devices are
> > > > available.
> > > >
> > > > I think this patch could be useful for you:
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.
> > > > org_patch_41834&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19I
> > > > Jiuxx_p_Rzo2g-uHDKw&m=uNHRQkbP_-z8v5d30KFx9pcPEUhlr4ciWY3ZDAVExTA&s=dB9wpgeP
> > > > 7AJg1eDRty0-RKhq3DY-7J5srIzyVoJey5I&e=
> > > >
> > > > With that the dw-hdmi-soc driver would have it's v4l2-async notifier's
> > > > notify_complete() callback called only when both the hdmi-rx and the
> > > > hdmi-phy subdevs are registered.
> > > Yeah, I saw the patches. I just implemented this way because they
> > > are not merged yet, right?
>>
> > I think these patches will be merged in v4.14-rc1, so together with your driver.
> > You could apply them locally and indicate that your series depends on them in
> > the cover letter.
>
> Ok, will apply them locally and re-test.

Thanks.

> > > > > +	if (ret) {
> > > > > +		dev_err(dw_dev->dev, "failed to register subdev nodes\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +static int dw_hdmi_rx_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	/* V4L2 initialization */
> > > > > +	sd = &dw_dev->sd;
> > > > > +	v4l2_subdev_init(sd, &dw_hdmi_sd_ops);
> > > > > +	strlcpy(sd->name, dev_name(dev), sizeof(sd->name));
> > > > > +	sd->dev = dev;
> > > > > +	sd->internal_ops = &dw_hdmi_internal_ops;
> > > > > +	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
>>>>
> > > > Don't you also need V4L2_SUBDEV_FL_HAS_DEVNODE flag set?
> > > Ouch. Yes I need otherwise the subdev will not be associated with
> > > the v4l2_device.
>>
> > This flag indicates that the v4l2 subdev device node (/dev/v4l-subdev?)
> > should be created for this subdevice.
>
> Ok, will add for controller driver only then: I think for phy
> this should not be added because controller is responsible to
> manage phy entirely so creating a /dev/ which can be seen by
> userspace can lead to confusion, maybe?

Right, there should be no need for the PHY. It's up to you to
set it or not for the RX controller subdev. I just got confused
because in your dw_hdmi_sd_ops data structure there are ops which
would suggest that device node is used.


Regards,
Sylwester