Hi Laurent,
On Tue, May 30, 2023 at 09:22:09AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thu, May 25, 2023 at 12:16:14PM +0300, Sakari Ailus wrote:
> > Set the v4l2_device already in async notifier init, so struct device
> > related to it will be available before the notifier is registered.
> >
> > This is done in order to make struct device available earlier, during
> > construction of the async connections, for sensible debug prints.
>
> I'm worried that the tighter dependency between the notifier and the
> v4l2_device will cause issues later. If it's just to get a struct device
> pointer, wouldn't it be better to pass the struct device to
> v4l2_async_nf_init() ?
And add a device field to the notifier?
I'm really not too worried about this; the device field is available from
the very beginning of the driver's probe function and registering the V4L2
device does not expose any driver interfaces. So I don't see a technical
reason for this.
A better name for v4l2_device_register() would actually be
v4l2_device_init(). Other renaming may follow from that, too. But that is
out of scope of this set in any case.
>
> > This patch has been mostly generated using the following two commands:
> >
> > git grep -l v4l2_async_nf_init -- drivers/media/ drivers/staging/media/ |
> > while read i; do perl -e '
> > @a=<>; unlink("'$i'"); open(F, "> '$i'");
> > for $f ({i=>"v4l2_async_nf_init", r=>"v4l2_async_nf_register"},
> > {i=>"v4l2_async_subdev_nf_init",
> > r=>"v4l2_async_subdev_nf_register"} ) {
> > my $b; @a = map { $b = "$1, $2" if
> > s/$f->{r}\(([^,]*),\s*([^\)]*)\)/v4l2_async_nf_register\($2\)/;
> > $_; } @a; @a = map { if (defined $b) {
> > s/v4l2_async_nf_init\([^\)]*\)/$f->{i}\($b\)/;
> > s/$f->{r}\(\K[^,]*,\s*//; }; $_; } @a; }; print F @a; close F;'
> > < $i; done
> >
> > git grep -lP 'v4l2_async_(subdev_|)nf_init' | xargs perl -i -pe \
> > 's/v4l2_async_(subdev_|)nf_init\(\K([^,]*),\s*([^,]*)\)/$3, $2\)/'
>
> It may be nice to play with perl code, but I'd like more focus on the
> human-readable part of the commit message, please. The above isn't
> useful to anyone but you, while an English text that explains how the
> v4l2_async_nf_init() function has now been split into
> v4l2_async_nf_init() and v4l2_async_subdev_nf_init() is missing.
I can add a few words about this.
The Perl code was useful for re-creating the patch but by now it has been
modified manually so much it's no longer useful. I'll drop it.
--
Kind regards,
Sakari Ailus
