Re: [PATCH v3 13/15] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Hans de Goede <hansg@kernel.org>]

Hi,

On 23-Dec-25 5:50 PM, Laurent Pinchart wrote:
> On Tue, Dec 23, 2025 at 02:37:39PM +0100, Hans de Goede wrote:
>> On 2-Jul-25 03:08, Laurent Pinchart wrote:
>>> On Sun, Jun 29, 2025 at 10:56:23PM +0200, Hans de Goede wrote:
>>>> Drop the start-, stop-streaming sequence from initialize.
>>>>
>>>> When streaming is started with a runtime-suspended sensor,
>>>> mt9m114_start_streaming() will runtime-resume the sensor which calls
>>>> mt9m114_initialize() immediately followed by calling
>>>> mt9m114_set_state(ENTER_CONFIG_CHANGE).
>>>>
>>>> This results in the following state changes in quick succession:
>>>>
>>>> mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
>>>> mt9m114_set_state(ENTER_SUSPEND)       -> transitions to SUSPENDED
>>>> mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
>>>>
>>>> these quick state changes confuses the CSI receiver on atomisp devices
>>>> causing streaming to not work.
>>>>
>>>> Drop the state changes from mt9m114_initialize() so that only
>>>> a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
>>>> when streaming is started with a runtime-suspend sensor.
>>>>
>>>> This means that the sensor may have config changes pending when
>>>> mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
>>>> when streaming was not started within the 1 second runtime-pm timeout.
>>>> Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
>>>> mt9m114_runtime_suspend() if necessary.
>>>
>>> Could you please update the commit message with the result of the
>>> discussion on v2 ? I'd like to record that mt9m114_power_on() leaves the
>>> sensor in the STANDBY state, either because it is already in standby for
>>> CSI-2 mode, or with an explicit state change in parallel mode (with the
>>> mode configured through the CONFIG pin). That's in my opinion the reason
>>> we can drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND in
>>> mt9m114_initialize().
>>
>> Ack, I'll add a comment to the code about this and update the commit
>> message as requested.
>>
>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>>> ---
>>>>  drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
>>>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>>>> index 1280d90cd411..ec5e9ce24d1c 100644
>>>> --- a/drivers/media/i2c/mt9m114.c
>>>> +++ b/drivers/media/i2c/mt9m114.c
>>>> @@ -389,6 +389,7 @@ struct mt9m114 {
>>>>  
>>>>  	unsigned int pixrate;
>>>>  	bool streaming;
>>>> +	bool config_change_pending;
>>>>  	u32 clk_freq;
>>>>  
>>>>  	/* Pixel Array */
>>>> @@ -781,14 +782,7 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> -
>>>> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> -
>>>> +	sensor->config_change_pending = true;
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -987,6 +981,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>>>>  	if (ret)
>>>>  		goto error;
>>>>  
>>>> +	sensor->config_change_pending = false;
>>>>  	sensor->streaming = true;
>>>>  
>>>>  	return 0;
>>>> @@ -2315,6 +2310,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
>>>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>>>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>>>>  
>>>> +	if (sensor->config_change_pending) {
>>>> +		/* mt9m114_set_state() prints errors itself, no need to check */
>>>> +		mt9m114_set_state(sensor,
>>>> +				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>>>> +		mt9m114_set_state(sensor,
>>>> +				  MT9M114_SYS_STATE_ENTER_SUSPEND);
>>>> +	}
>>>
>>> What's the point of this if we're powering the sensor right after ?
> 
> (I meant "powering the sensor down")
> 
>> The config-change state command is necessary to allow
>> the suspend command to succeed.
>>
>> Before this initialize() would do the suspend for
>> the probe() path and mt9m114_stop_streaming()
>> does this after a stop-stream.
>>
>> The mean reason is to preserve the old behavior of
>> always putting the sensor in suspend, which could
>> be useful if the regulators are shared (or just always
>> on) and the reset pin is not connected.
> 
> True, but that's done in mt9m114_stop_streaming(), isn't it ?
> 
> There's still something that bothers me about this patch. The
> config_change_pending flag makes the logic hard to follow, it should be
> simplified somehow.
> 
> Re-reading the code, could we
> 
> - drop the mt9m114_initialize() from mt9m114_probe()
> - move the mt9m114_initialize() from mt9m114_runtime_resume() to
>   mt9m114_start_streaming()
> - drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND from
>   mt9m114_initialize()
> 
> Am I missing something ?

Nope that should work, but at the cost of re-doing
the mt9m114_initialize() when doing stop-stream,
change some things, start-stream. Not that I expect
that to happen often with this sensor, but this is why
I kept the initialize in runtime-resume.

Regards,

Hans



>