Received-SPF: pass (google.com: domain of linux-kernel+bounces-156563-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <9075808c-783b-4af6-a92f-2a6d3f25d225@collabora.com>
Date: Wed, 24 Apr 2024 10:42:11 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 4/4] media: i2c: add MAX96714 driver
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
 kernel@collabora.com, linux-kernel@vger.kernel.org, mchehab@kernel.org,
 robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
References: <20240325131634.165361-1-julien.massot@collabora.com>
 <20240325131634.165361-5-julien.massot@collabora.com>
 <ZhkaR-83uciNFi2b@valkosipuli.retiisi.eu>
 <a70e3792-b938-4c20-840d-5dfa9bdc4b9c@collabora.com>
 <ZijDHtSa2vACgBjl@valkosipuli.retiisi.eu>
Content-Language: en-US
From: Julien Massot <julien.massot@collabora.com>
In-Reply-To: <ZijDHtSa2vACgBjl@valkosipuli.retiisi.eu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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