Re: [PATCH v4 3/4] media: raspberrypi: Add support for RP1-CFE
From: Tomi Valkeinen
Date: Mon Sep 09 2024 - 06:05:07 EST
On 09/09/2024 12:13, Jacopo Mondi wrote:
Hi Tomi
On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
Hi Laurent, Jacopo,
On 09/09/2024 08:08, Tomi Valkeinen wrote:
Hi,
On 05/09/2024 14:11, Laurent Pinchart wrote:
On Thu, Sep 05, 2024 at 06:50:48PM +0800, kernel test robot wrote:
Hi Tomi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]
url: https://github.com/intel-lab-lkp/linux/commits/Tomi-
Valkeinen/media-uapi-Add-meta-formats-for-PiSP-FE-config-and-
stats/20240904-192729
base: 431c1646e1f86b949fa3685efc50b660a364c2b6
patch link: https://lore.kernel.org/r/20240904-rp1-cfe-v4-3-
f1b5b3d69c81%40ideasonboard.com
patch subject: [PATCH v4 3/4] media: raspberrypi: Add support
for RP1-CFE
config: m68k-allmodconfig (https://download.01.org/0day-ci/
archive/20240905/202409051822.ZzUGw3XQ-lkp@xxxxxxxxx/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/
archive/20240905/202409051822.ZzUGw3XQ-lkp@xxxxxxxxx/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a
new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@xxxxxxxxx>
| Closes: https://lore.kernel.org/oe-kbuild-
all/202409051822.ZzUGw3XQ-lkp@xxxxxxxxx/
All warnings (new ones prefixed by >>):
drivers/media/platform/raspberrypi/rp1-cfe/cfe.c:2445:12:
warning: 'cfe_runtime_resume' defined but not used
[-Wunused-function]
2445 | static int cfe_runtime_resume(struct device *dev)
| ^~~~~~~~~~~~~~~~~~
drivers/media/platform/raspberrypi/rp1-cfe/cfe.c:2435:12:
warning: 'cfe_runtime_suspend' defined but not used
[-Wunused-function]
2435 | static int cfe_runtime_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~
vim +/cfe_runtime_resume +2445
drivers/media/platform/raspberrypi/ rp1-cfe/cfe.c
The recommended way to fix this is to switch from SET_RUNTIME_PM_OPS()
to RUNTIME_PM_OPS() and use pm_ptr() to set .driver.pm. This being said,
the driver won't work on a kernel with !CONFIG_PM given how you
implemented probe() and remove().
The pisp-be driver suffered from the same issue and Jacopo fixed it in
the last version. You can have a look at implement something similar.
I can't right away think of any reason to not just depend on CONFIG_PM
and be done with it without any tricks. Do you know if there's a reason?
We had the same discussion, and even if I would be fine depending on
CONFIG_PM, supporting !CONFIG_PM is not that much work, I kept it as
an optional dependency (it was suggested during the review as well)
Also, I don't think pisp-be is correct. It just calls
pispbe_runtime_resume() in probe() to wake the IP up (which only enables
pisp clock), without telling the runtime PM about it. This means the parent
device and the suppliers may not be powered up.
Are you referring to the code currently in the tree or to this patch ?
https://patchwork.linuxtv.org/project/linux-media/patch/20240904095836.344833-5-jacopo.mondi@xxxxxxxxxxxxxxxx/
Ah, I missed that one.
I don't think it fixes the issue I mentioned. If we have PM enabled, and
the parent device requires powering up for the child device (BE) to be
accessible, the driver will crash when calling pispbe_hw_init(). I think
you should call pm_runtime_set_active() before calling
pispbe_runtime_resume().
However, you said above that "supporting !CONFIG_PM is not that much
work". Maybe not. But how much work is it to get it right (for both PM
and !PM), and make sure it stays right? =).
Just my opinion, but if there are zero use cases for the !PM, I would
just go with "depends on PM" to keep the driver simpler, less bug-prone,
and easier to maintain.
Tomi