Hi Hans,
thanks for your reply
On Mon, Jun 14, 2021 at 10:51:25AM +0200, Hans Verkuil wrote:
> On 12/04/2021 11:34, Jacopo Mondi wrote:
> > The init() subdev core operation is deemed to be deprecated for new
> > subdevice drivers. However it could prove useful for complex
> > architectures to defer operation that require access to the
> > communication bus if said bus is not available (or fully configured)
> > at the time when the subdevice probe() function is run.
> >
> > As an example, the GMSL architecture requires the GMSL configuration
> > link to be configured on the host side after the remote subdevice
> > has completed its probe function. After the configuration on the host
> > side has been performed, the subdevice registers can be accessed through
> > the communication bus.
> >
> > In particular:
> >
> > HOST REMOTE
> >
> > probe()
> > |
> > ---------------------> |
> > probe() {
> > bus config()
> > }
> > |<--------------------|
> > v4l2 async bound {
> > bus config()
> > call subdev init()
> > |-------------------->|
> > init() {
> > access register on the bus()
> > }
> > |<-------------------
> > }
> >
> > In the GMSL use case the bus configuration requires the enablement of the
> > noise immunity threshold on the remote side which ensures reliability
> > of communications in electrically noisy environments. After the subdevice
> > has enabled the threshold at the end of its probe() sequence the host
> > side shall compensate it with an higher signal amplitude. Once this
> > sequence has completed the bus can be accessed with noise protection
> > enabled and all the operations that require a considerable number of
> > transactions on the bus (such as the image sensor configuration
> > sequence) are run in the subdevice init() operation implementation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> > include/media/v4l2-subdev.h | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d0e9a5bdb08b..3068d9940669 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > * each pin being configured. This function could be called at times
> > * other than just subdevice initialization.
> > *
> > - * @init: initialize the sensor registers to some sort of reasonable default
> > - * values. Do not use for new drivers and should be removed in existing
> > - * drivers.
> > + * @init: initialize the subdevice registers to some sort of reasonable default
> > + * values. Do not use for new drivers (and should be removed in existing
> > + * ones) for regular architectures where the image sensor is connected to
> > + * the host receiver. For more complex architectures where the subdevice
> > + * initialization should be deferred to the completion of the probe
> > + * sequence of some intermediate component, or the communication bus
> > + * requires configurations on the host side that depend on the completion
> > + * of the probe sequence of the remote subdevices, the usage of this
> > + * operation could be considered to allow the devices along the pipeline to
> > + * probe and register in the media graph and to defer any operation that
> > + * require actual access to the communication bus to their init() function
> > + * implementation.
>
> I don't like de-deprecating init. It was deprecated for a good reason, and
> I'd like to keep it that way.
I see, fair enough :)
>
> There are two alternatives: one is a bit quick-and-dirty, the other is a hint
> towards a more generic solution (just a hint since it will require more research):
>
> 1) Quick-and-dirty: use the core callback op to create a custom INIT callback.
> This depends on this patch:
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20210610214305.4170835-8-arnd@xxxxxxxxxx/
>
> This will make it clear to the reader that this is a highly specific interaction
> between two drivers that are tightly coupled. It works in the current situation,
> but not if we want to make this more generic.
Depends what you mean with 'generic' :) I think such a solution would
slightly abuse a generic API like 'command' is, but the GMSL
deserializers/serializers are tighly coupled by definition, so this is
less a concern, as long as we have a single driver for the whole
camera module. If we're going to split it in 3 subdev drivers then
yes, they will all have to implement .command() and they can be used
with in isolation with a generic receiver driver.
>
> 2) Subdev drivers can implement the registered() op which is called by
> v4l2_device_register_subdev(). This in turn is called from v4l2_async_match_notify().
>
> What you want is that when max9286 calls v4l2_async_subdev_notifier_register, it
> can set a flag or something indicating that initialization has to be postponed.
> Then, when v4l2_async_match_notify() calls the register() callback, that flag can
> be read. If false, then the register() callback will initialize the device, if
> true then that won't happen. Instead, it will do that when the max9286 calls a
> post_register() callback.
2 questions to help me better understand this:
1) s/register()/registered() in this paragraph ?
2) $ git grep post_register drivers/media/ include/media/
gives me back nothing.
Are you suggesting a new operation ?
Thanks
j
>
> This is a lot more work (and research, since this is just a brainstorm from my
> side), but it is a way towards making this a generic solution.
>
> Regards,
>
> Hans
>
> > *
> > * @load_fw: load firmware.
> > *
> > --
> > 2.31.1
> >
>
