On Tue, Jan 21, 2025 at 03:38:42PM +0100, Andrea della Porta wrote:

Hi Greg,

On 15:18 Tue 21 Jan     , Greg Kroah-Hartman wrote:

quoted

On Tue, Jan 21, 2025 at 02:59:21PM +0100, Andrea della Porta wrote:

quoted

Hi Greg,

On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:

quoted

On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:

quoted

Hi Greg,

On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:

quoted

On Mon, Jan 13, 2025 at 03:58:07PM +0100, Andrea della Porta wrote:

quoted

The RaspberryPi RP1 is a PCI multi function device containing
peripherals ranging from Ethernet to USB controller, I2C, SPI
and others.

Implement a bare minimum driver to operate the RP1, leveraging
actual OF based driver implementations for the on-board peripherals
by loading a devicetree overlay during driver probe.

The peripherals are accessed by mapping MMIO registers starting
from PCI BAR1 region.

With the overlay approach we can achieve more generic and agnostic
approach to managing this chipset, being that it is a PCI endpoint
and could possibly be reused in other hw implementations. The
presented approach is also used by Bootlin's Microchip LAN966x
patchset (see link) as well, for a similar chipset.

For reasons why this driver is contained in drivers/misc, please
check the links.

Links aren't always around all the time, please document it here why
this is needed, and then links can "add to" that summary.

Ack.

quoted

quoted

This driver is heavily based on downstream code from RaspberryPi
Foundation, and the original author is Phil Elwell.

Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf

...

quoted

quoted

diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
new file mode 100644
index 000000000000..3e8ba3fa7fd5
--- /dev/null
+++ b/drivers/misc/rp1/rp1_pci.c

@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018-24 Raspberry Pi Ltd.
+ * All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include "rp1_pci.h"

Why does a self-contained .c file need a .h file?  Please put it all in
here.

I agree with you. Indeed, the very first version of this patch had the header
file placed inside the .c, but I received concerns about it and some advice to
do it differently, as you can see here:
https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/ (local)
so I've changed it accordingly in V2. So right now I'm not sure what the
acceptable behaviour should be ...

It's a pretty simple rule:
	Only use a .h file if multiple .c files need to see the symbol.

So no .h file is needed here.

Perfect, I'll revert back that two lines to V1 then. Please be aware
though that this will trigger the following checkpatch warning:

WARNING: externs should be avoided in .c files

Well where are those externs defined at?  Shouldn't there be a .h file
for them somewhere in the tree if they really are global?

Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.
They are just placeholders that contains rp1-pci.dtbo as
a binary blob, in order for the driver (rp1_pci.c) to be able to use
the binary buffer representing the overlay and address it from the
driver probe function.
So there's no other reference from outside rp1_pci.c to those two symbols.
In comparison, this is the very same approach used by a recently accepted
patch involving drivers/misc/lan966x_pci.c, which also has the two externs
in it and triggers the same checkpatch warning.

Ok, that's fine, checkpatch is just a hint, not a hard-and-fast-rule.

thanks,

greg k-h