Hi Sakari,
On 4/24/24 10:30, Sakari Ailus wrote:
Hi Julien,
On Tue, Apr 23, 2024 at 04:01:16PM +0200, Julien Massot wrote:
...
+static int max96714_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ u32 source_pad, u64 streams_mask)
+{
+ struct max96714_priv *priv = sd_to_max96714(sd);
+ u64 sink_streams;
+ int ret;
+
+ if (!priv->enabled_source_streams)
+ max96714_enable_tx_port(priv);
+
+ ret = max96714_apply_patgen(priv, state);
+ if (ret)
+ goto err;
+
+ if (!priv->pattern) {
+ if (!priv->rxport.source.sd) {
+ ret = -ENODEV;
+ goto err;
+ }
On enable streams the check is here :)
Streaming is not possible without a remote serializer when pattern
generator is disabled.
I may refactor this code later when we will have the internal pad, to
declare
properly this stream.
.
+
+ sink_streams =
+ v4l2_subdev_state_xlate_streams(state,
+ MAX96714_PAD_SOURCE,
+ MAX96714_PAD_SINK,
+ &streams_mask);
+
+ ret = v4l2_subdev_enable_streams(priv->rxport.source.sd,
+ priv->rxport.source.pad,
+ sink_streams);
+ if (ret)
+ goto err;
+ }
+
+ priv->enabled_source_streams |= streams_mask;
+
+ return 0;
+
+err:
+ if (!priv->enabled_source_streams)
+ max96714_disable_tx_port(priv);
+
+ return ret;
+}
+
+static int max96714_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ u32 source_pad, u64 streams_mask)
+{
+ struct max96714_priv *priv = sd_to_max96714(sd);
+ u64 sink_streams;
+ int ret;
+
+ if (!priv->pattern && priv->rxport.source.sd) {
When will priv->rxport.source.sd be NULL here?
Indeed it should not, the priv->rxport.source.sd can only be null if:
- There is no serializer
- The stream has been started with pattern generator and the pattern
generator
has been disabled while streaming.
It seems priv->rxport.source.sd is also accessed in
max96714_enable_streams() without such a check.
Please see my reply above :)
In V7 I will drop this check and add another one to prevent disabling the
pattern
generator while streaming.
Sounds good.
+static void max96714_v4l2_notifier_unregister(struct max96714_priv *priv)
+{
+ v4l2_async_nf_unregister(&priv->notifier);
+ v4l2_async_nf_cleanup(&priv->notifier);
It'd be nicer to call these directly IMO. Maybe we could introduce
v4l2_async_nf_unregister_cleanup()? Feel free to post a patch. :-)
Ok, I will call these directly, and I will do the same for the MAX96717
serializer.
I will post a patchset later introducing the
`v4l2_async_nf_unregister_cleanup`
and converting all the drivers calling these two functions.
That would be nice. :-) It should be easy to do that with Coccinelle.
...
+ ret = max96714_enable_core_hw(priv);
Please switch to runtime PM.
Ok, the v7 will use runtime PM and I will use the powerdown gpio
to poweroff the device. However it implies to move some functions arround
e.g initialize the tx or the pattern generator ..
So it it will be done as separate patches.
Playing with the pm_runtime operation also showed up that the connection
doesn't always resume properly, I will extra patches to fix that.
Ack.
--
Julien Massot
Senior Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
