Re: [PATCH v3] media: bcm2835-unicam: Fix log status runtime access

[Eugen Hristev <ehristev@kernel.org>]

On 6/11/26 20:34, Laurent Pinchart wrote:
> On Thu, Jun 11, 2026 at 08:18:10PM +0300, Eugen Hristev wrote:
>> On 6/11/26 11:03, Laurent Pinchart wrote:
>>> On Thu, Jun 11, 2026 at 08:29:55AM +0300, Eugen Hristev wrote:
>>>> When requesting log status, the block might be powered off, but registers
>>>> are being read.
>>>> Avoid reading the registers if the device is not resumed, thus also avoid
>>>> powering up the device just for log status.
>>>>
>>>> Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
>>>> Signed-off-by: Eugen Hristev <ehristev@kernel.org>
>>>> ---
>>>> Changes in v3:
>>>> - Changed to check return value of pm_runtime_get_if_active() and only call
>>>>   pm_runtime_put() if the device is active.
>>>> - Link to v2: https://patch.msgid.link/20260522-bcmpipm-v2-1-a3da66cbc9f0@kernel.org
>>>>
>>>> Changes in v2:
>>>> - changed to use pm_runtime_get_if_active()
>>>> - add corresponding put()
>>>> - Link to v1: https://patch.msgid.link/20260521-bcmpipm-v1-1-3eba88d88045@kernel.org
>>>>
>>>> To: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
>>>> To: Mauro Carvalho Chehab <mchehab@kernel.org>
>>>> To: Florian Fainelli <florian.fainelli@broadcom.com>
>>>> To: Ray Jui <rjui@broadcom.com>
>>>> To: Scott Branden <sbranden@broadcom.com>
>>>> To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
>>>> To: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
>>>> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> To: Hans Verkuil <hverkuil@kernel.org>
>>>> To: Naushir Patuck <naush@raspberrypi.com>
>>>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>> Cc: linux-media@vger.kernel.org
>>>> Cc: linux-rpi-kernel@lists.infradead.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>  drivers/media/platform/broadcom/bcm2835-unicam.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
>>>> index 8d28ba0b59a3..96b51e29bba4 100644
>>>> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
>>>> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
>>>> @@ -2043,6 +2043,7 @@ static int unicam_log_status(struct file *file, void *fh)
>>>>  	struct unicam_node *node = video_drvdata(file);
>>>>  	struct unicam_device *unicam = node->dev;
>>>>  	u32 reg;
>>>> +	int pm_active;
>>>>  
>>>>  	/* status for sub devices */
>>>>  	v4l2_device_call_all(&unicam->v4l2_dev, 0, core, log_status);
>>>> @@ -2052,6 +2053,14 @@ static int unicam_log_status(struct file *file, void *fh)
>>>>  		 node->fmt.fmt.pix.width, node->fmt.fmt.pix.height);
>>>>  	dev_info(unicam->dev, "V4L2 format:         %08x\n",
>>>>  		 node->fmt.fmt.pix.pixelformat);
>>>> +
>>>> +	pm_active = pm_runtime_get_if_active(unicam->dev);
>>>> +	if (!pm_active) {
>>>> +		dev_info(unicam->dev,
>>>> +			 "Live data N/A due to device inactive\n");
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	reg = unicam_reg_read(unicam, UNICAM_IPIPE);
>>>>  	dev_info(unicam->dev, "Unpacking/packing:   %u / %u\n",
>>>>  		 unicam_get_field(reg, UNICAM_PUM_MASK),
>>>> @@ -2065,6 +2074,9 @@ static int unicam_log_status(struct file *file, void *fh)
>>>>  	dev_info(unicam->dev, "Write pointer:       %08x\n",
>>>>  		 unicam_reg_read(unicam, UNICAM_IBWP));
>>>>  
>>>> +	if (pm_active == 1)
>>>> +		pm_runtime_put(unicam->dev);
>>>
>>> As far as I understand, the discussion on v2 concluded there was no need
>>> to test pm_active here. Did I miss anything ?
>>
>> Sorry, I saw the message from Sakari and he was pretty confident on the
>> right way, he even mentioned that all sensors should be fixed, and has
>> not come up with a follow up since.
>> If v2 is the right way, please disregard this v3
> 
> Let's see what Sakari has to say :-)

Hi Sakari,

Could you please let me know whether this v3 is the way you want this to
move forward ?
I have some pending things on top of this. If something is not right let
me know so I can send a v4.

Thanks,
Eugen

> 
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>>
>>>> ---
>>>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
>>>> change-id: 20260521-bcmpipm-6c578e73239c
>