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? > > 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(-) > > > -- Kind Regards, Niklas Söderlund
