Re: [PATCH v2 00/11] media: adv7180: Improve the control over decoder power

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 03/09/2025 16:35, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your review effort!
> 
> On 2025-09-03 10:46:11 +0200, Hans Verkuil wrote:
>> On 28/08/2025 18:06, Niklas Söderlund wrote:
>>> Hello,
>>>
>>> This series started as an effort to fix issues with querystd. To do that
>>> it turned out the whole drivers design around how it controls the power
>>> to the video decoder block inside the chip had to be reworked. As a
>>> bonus this works removes the now deprecated .s_power callback from
>>> adv7180.
>>>
>>> The adv7180 drivers comes from a time before media controller and all
>>> operation callbacks are, more or less, designed around the concept that
>>> a video device is the only user-space facing API. In that world a vdev
>>> would attached the subdevice, call .s_power and then perform format
>>> configuration using the other operation callbacks and then start
>>> streaming with .s_stream. Needles to say this mode of operation don't
>>> work well with media controller where the subdevices itself have a
>>> user-space API exposed thru a subdev device.
>>>
>>> The initial problem I tried to solve (querystd) was that it stopped
>>> functioning as expected after the subdev had been used to stream once
>>> (.s_power(1), .s_power(0)). As it turns out different variants of the
>>> adv7180 device have different reset beaver for if its video decoder
>>> block is left running or powered off. On my device it was left running
>>> so querystd functioned the first time, but not after the video decoder
>>> had been switched off once by .s_power(0).
>>>
>>> I first tried to fix this by introducing proper PM handling in the
>>> driver to be able to remove the .s_power callback. I quickly learnt the
>>> power on/off logic happening in the driver had noting to do with
>>> controlling power to the chip itself, but to control if the chips video
>>> decoder block was turned off.
>>>
>>> When this block is powered on the device process video data, if there is
>>> a video source else it free runs. However when the block is turned off
>>> the device can still be configured, in fact some configuration requires
>>> it to be off.
>>>
>>> For this reason I dropped the effort to add proper PM handling and
>>> treated the decoder power as a stream on/off switch. I still think
>>> proper PM handling would be beneficial for this driver but to not
>>> explode this already large series I left that for another time. Solving
>>> the issue around .s_power will make that work easier as well as other
>>> task such as converting to the v4l2_subdev active state API.
>>>
>>> Patch 1/11 just moves code around to make the consecutive changes easier
>>> to read. Patch 2/11 fix a locking issues when suspending the device.
>>> Patch 3/11 and 4/11 improves the locking design to prepare to improve
>>> the driver.
>>>
>>> Patch 5/11 make sure the device controls are always programmed after the
>>> device have been reset, fixing a possible issue when the device where
>>> resumed from system sleep.
>>>
>>> Patches 6/11, 7/11 and 8/11 is the real change where the .s_power
>>> callback is reworked to fit the design of .s_stream instead.
>>>
>>> And finally patch 9/11, 10/11 and 11/11 removes programming of the
>>> device from operation callbacks and solves the issue with querystd.
>>>
>>> The work is tested on R-Car M2 together with a ADV7180 device.
>>>
>>> See individual patches for changelog.
>>
>> This series looks good to me, other than the one typo and the
>> control handler kAPI issue, but that can be done in a follow-up
>> series.
>>
>> If you want I can take this series, let me know.
> 
> Thanks for pointing out the improvement in the kAPI, I agree with it.  
> But as you point out I can do that in follow up work so I would be happy 
> if you would take this series. Can you correct the typo while applying, 
> or would you prefer I send an undated series?

I'll correct the typo.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Niklas Söderlund (11):
>>>   media: adv7180: Move adv7180_set_power() and init_device()
>>>   media: adv7180: Add missing lock in suspend callback
>>>   media: adv7180: Move state mutex handling outside init_device()
>>>   media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking
>>>   media: adv7180: Setup controls every time the device is reset
>>>   media: adv7180: Power down decoder when configuring the device
>>>   media: adv7180: Split device initialization and reset
>>>   media: adv7180: Remove the s_power callback
>>>   media: adv7180: Do not write format to device in set_fmt
>>>   media: adv7180: Only validate format in s_std
>>>   media: adv7180: Only validate format in querystd
>>>
>>>  drivers/media/i2c/adv7180.c | 338 +++++++++++++++++++-----------------
>>>  1 file changed, 174 insertions(+), 164 deletions(-)
>>>
>>
>