lkml.org 
[lkml]   [2020]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
From
Date
Hi Greg - thanks for the comments, appreciate it (sorry there's so many,
I'm new to both C and kernel work)

On 17/09/2020 08:53, Greg KH wrote:
> 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...
I was just under the impression that that was the process to be honest,
if that's not right I'll just move it directly to drivers/media/ipu3
>> 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@gmail.com>
>> +L: linux-media@vger.kernel.org
>> +S: Maintained
>> +F: drivers/staging/media/ipu3/cio2-bridge.c
>> +
>> IPVS
>> M: Wensong Zhang <wensong@linux-vs.org>
>> M: Simon Horman <horms@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.

Yeah sorry; misunderstood entirely how that was supposed to work. From
the following comment, this probably needs re-implementing as
list_for_each_entry anyway

>
>> + &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 :)
So the intention was just to check that there is _a_ linked device,
which I had been assuming would be a sensor that wanted to use the cio2
device. That's probably not very smart in retrospect; I hadn't
considered other none-me pieces of code linking in. I guess a better
approach would be to check all the linked devices with list_each_entry
and determine if at least one of them is a sensor.
>
>> + }
>> +
>> + 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?
Yeah I guess if the cio2_pci_probe() never made it to setting driver
data then the sync_state() shouldn't get called; thanks.
>
>> + return;
> No error value?
The prototype for sync_state callbacks is to return void, so my
understanding is it can't return an error value.  I guess a better thing
to do might be call another function performing cleanup and unloading
the driver before the return or something along those lines though.
>
>> + }
>> +
>> + /* 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?
>
If the bridge doesn't work, the cio2_parse_firmware() call should behave
as it does now on these platforms - I.E. just not connect anything. If
felt ok for that to happen, but I can have it perform cleanup at this
point if that's a better approach.
>> + }
>> + }
> And again, do not tell anyone?
>
> Feels really wrong...
I think return type void prevents that, but I could be completely wrong
about that.
>> +}
>> +
>> /**************** 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?
X86 because as far as I know this is already working properly on other
platforms, so wouldn't be needed. Not sure what CONFIG_TEST is for; sorry.
>> + 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?
Oops; a leftover from debugging some reference count issues in the
earlier days - I'll take that out.
>> +
>> +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...
>
I didn't - I'll do that. I appreciate you spending the time at all -
thanks, and sorry it's been so messy!

\
 
 \ /
  Last update: 2020-09-17 11:49    [W:0.247 / U:0.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site