[RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
Greg KH
gregkh at linuxfoundation.org
Thu Sep 17 07:53:56 UTC 2020
On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
> MAINTAINERS | 6 +
> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 67 +++-
staging drivers should be self-contained, and not modify stuff outside
of drivers/staging/
> drivers/staging/media/ipu3/Kconfig | 15 +
> drivers/staging/media/ipu3/Makefile | 1 +
> drivers/staging/media/ipu3/cio2-bridge.c | 448 +++++++++++++++++++++++
Why does this have to be in drivers/staging/ at all? Why not spend the
time to fix it up properly and get it merged correctly? It's a very
small driver, and should be smaller, so it should not be a lot of work
to do. And it would be faster to do that than to take it through
staging...
> 5 files changed, 534 insertions(+), 3 deletions(-)
> create mode 100644 drivers/staging/media/ipu3/cio2-bridge.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b5cfab015bd6..55b0b9888bc0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9152,6 +9152,12 @@ S: Maintained
> W: http://www.adaptec.com/
> F: drivers/scsi/ips*
>
> +IPU3 CIO2 Bridge Driver
> +M: Daniel Scally <djrscally at gmail.com>
> +L: linux-media at vger.kernel.org
> +S: Maintained
> +F: drivers/staging/media/ipu3/cio2-bridge.c
> +
> IPVS
> M: Wensong Zhang <wensong at linux-vs.org>
> M: Simon Horman <horms at verge.net.au>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 92f5eadf2c99..fd941d2c7581 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)
> cio2_queue_exit(cio2, &cio2->queue[i]);
> }
>
> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
> +{
> + void *sensor;
This is a huge flag that something is wrong, why void?
> +
> + /*
> + * On ACPI platforms, we need to probe _after_ sensors wishing to connect
> + * to cio2 have added a device link. If there are no consumers yet, then
> + * we need to defer. The .sync_state() callback will then be called after
> + * all linked sensors have probed
> + */
> +
> + if (IS_ENABLED(CONFIG_ACPI)) {
> + sensor = (struct device *)list_first_entry_or_null(
And you cast it??? Not right at all.
> + &pci_dev->dev.links.consumers,
> + struct dev_links_info,
> + consumers);
How do you "know" this is the first link? This feels really really
wrong and very fragile.
> +
> + if (!sensor)
> + return -EPROBE_DEFER;
So any random value will work? I doubt it :)
> + }
> +
> + return 0;
> +}
> +
> +void cio2_sync_state(struct device *dev)
> +{
> + struct cio2_device *cio2;
> + int ret = 0;
> +
> + if (IS_ENABLED(CONFIG_ACPI)) {
> + cio2 = dev_get_drvdata(dev);
> +
> + if (!cio2) {
> + dev_err(dev, "Failed to retrieve driver data\n");
How can this fail?
> + return;
No error value?
> + }
> +
> + /* insert the bridge driver to connect sensors via software nodes */
> + ret = request_module("cio2-bridge");
> +
> + if (ret)
> + dev_err(dev, "Failed to insert cio2-bridge\n");
Yet you keep on in the function???
> +
> + ret = cio2_parse_firmware(cio2);
> +
> + if (ret) {
> + v4l2_async_notifier_unregister(&cio2->notifier);
> + v4l2_async_notifier_cleanup(&cio2->notifier);
> + cio2_queues_exit(cio2);
But you clean up after this error?
> + }
> + }
And again, do not tell anyone?
Feels really wrong...
> +}
> +
> /**************** PCI interface ****************/
>
> static int cio2_pci_config_setup(struct pci_dev *dev)
> @@ -1746,6 +1799,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
> void __iomem *const *iomap;
> int r;
>
> + r = cio2_probe_can_progress(pci_dev);
> +
> + if (r)
> + return -EPROBE_DEFER;
> +
> cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);
> if (!cio2)
> return -ENOMEM;
> @@ -1821,9 +1879,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
> v4l2_async_notifier_init(&cio2->notifier);
>
> /* Register notifier for subdevices we care */
> - r = cio2_parse_firmware(cio2);
> - if (r)
> - goto fail_clean_notifier;
> + if (!IS_ENABLED(CONFIG_ACPI)) {
> + r = cio2_parse_firmware(cio2);
> + if (r)
> + goto fail_clean_notifier;
> + }
>
> r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,
> IRQF_SHARED, CIO2_NAME, cio2);
> @@ -2052,6 +2112,7 @@ static struct pci_driver cio2_pci_driver = {
> .remove = cio2_pci_remove,
> .driver = {
> .pm = &cio2_pm_ops,
> + .sync_state = cio2_sync_state
> },
> };
>
> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
> index 3e9640523e50..08842fd8c0da 100644
> --- a/drivers/staging/media/ipu3/Kconfig
> +++ b/drivers/staging/media/ipu3/Kconfig
> @@ -14,3 +14,18 @@ config VIDEO_IPU3_IMGU
>
> Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
> camera. The module will be called ipu3-imgu.
> +
> +config VIDEO_CIO2_BRIDGE
> + tristate "IPU3 CIO2 Sensor Bridge Driver"
> + depends on PCI && VIDEO_V4L2
> + depends on ACPI
> + depends on X86
Why x86?
Why not CONFIG_TEST?
> + help
> + This module provides a bridge connecting sensors (I.E. cameras) to
> + the CIO2 device infrastructure via software nodes built from information
> + parsed from the SSDB buffer.
> +
> + Say Y or M here if your platform's cameras use IPU3 with connections
> + that should be defined in ACPI. The module will be called cio2-bridge.
> +
> + If in doubt, say N here.
> \ No newline at end of file
> diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile
> index 9def80ef28f3..12dff56dbb9e 100644
> --- a/drivers/staging/media/ipu3/Makefile
> +++ b/drivers/staging/media/ipu3/Makefile
> @@ -10,3 +10,4 @@ ipu3-imgu-objs += \
> ipu3-css.o ipu3-v4l2.o ipu3.o
>
> obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
> +obj-$(CONFIG_VIDEO_CIO2_BRIDGE) += cio2-bridge.o
> \ No newline at end of file
> diff --git a/drivers/staging/media/ipu3/cio2-bridge.c b/drivers/staging/media/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..5115aeeb35a1
> --- /dev/null
> +++ b/drivers/staging/media/ipu3/cio2-bridge.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include <linux/fwnode.h>
> +#include <linux/kref.h>
Why kref.h?
> +
> +static void cio2_bridge_exit(void);
> +static int cio2_bridge_init(void);
> +
> +#define MAX_CONNECTED_DEVICES 4
> +#define SWNODE_SENSOR_HID 0
> +#define SWNODE_SENSOR_PORT 1
> +#define SWNODE_SENSOR_ENDPOINT 2
> +#define SWNODE_CIO2_PORT 3
> +#define SWNODE_CIO2_ENDPOINT 4
> +#define SWNODE_NULL_TERMINATOR 5
> +
> +#define CIO2_HID "INT343E"
> +#define CIO2_PCI_ID 0x9d32
> +
> +#define ENDPOINT_SENSOR 0
> +#define ENDPOINT_CIO2 1
> +
> +#define NODE_HID(_HID) \
> +((const struct software_node) { \
> + _HID, \
> +})
> +
> +#define NODE_PORT(_PORT, _HID_NODE) \
> +((const struct software_node) { \
> + _PORT, \
> + _HID_NODE, \
> +})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS) \
> +((const struct software_node) { \
> + _EP, \
> + _PORT, \
> + _PROPS, \
> +})
> +
> +#define PROPERTY_ENTRY_NULL \
> +((const struct property_entry) { })
> +#define SOFTWARE_NODE_NULL \
> +((const struct software_node) { })
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be
> + * working
> + */
> +
> +static char *supported_devices[] = {
> + "INT33BE",
> + "OVTI2680",
> + "OVTI5648",
> +};
> +
> +/*
> + * software_node needs const char * names. Can't snprintf a const char *,
> + * so instead we need an array of them and use the port num from SSDB as
> + * an index.
> + */
> +
> +const char *port_names[] = {
> + "port0", "port1", "port2", "port3", "port4",
> + "port5", "port6", "port7", "port8", "port9"
> +};
> +
> +struct software_node cio2_hid_node = { CIO2_HID, };
> +
> +struct sensor {
> + struct device *dev;
> + struct software_node swnodes[5];
> + struct property_entry sensor_props[6];
> + struct property_entry cio2_props[3];
> + struct fwnode_handle *fwnode;
> +};
> +
> +struct cio2_bridge {
> + int n_sensors;
> + struct sensor sensors[MAX_CONNECTED_DEVICES];
> + struct pci_dev *cio2;
> + struct fwnode_handle *cio2_fwnode;
> +};
> +
> +struct cio2_bridge bridge = { 0, };
> +
> +static const struct property_entry remote_endpoints[] = {
> + PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */
> + &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */
> + &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint",
> + &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint",
> + &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint",
> + &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint",
> + &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint",
> + &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),
> + PROPERTY_ENTRY_REF("remote-endpoint",
> + &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),
> + { }
> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct sensor_bios_data_packed {
> + u8 version;
> + u8 sku;
> + u8 guid_csi2[16];
> + u8 devfunction;
> + u8 bus;
> + u32 dphylinkenfuses;
> + u32 clockdiv;
> + u8 link;
> + u8 lanes;
> + u32 csiparams[10];
> + u32 maxlanespeed;
> + u8 sensorcalibfileidx;
> + u8 sensorcalibfileidxInMBZ[3];
> + u8 romtype;
> + u8 vcmtype;
> + u8 platforminfo;
> + u8 platformsubinfo;
> + u8 flash;
> + u8 privacyled;
> + u8 degree;
> + u8 mipilinkdefined;
> + u32 mclkspeed;
> + u8 controllogicid;
> + u8 reserved1[3];
> + u8 mclkport;
> + u8 reserved2[13];
> +} __attribute__((__packed__));
Endian issues???
This doesn't look "packed" to me, did you check it?
I've stopped here, sorry, ran out of time...
greg k-h
More information about the devel
mailing list