| Message ID | 20201001093326.6208-1-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
On Thu, Oct 01, 2020 at 10:33:26AM +0100, Daniel Scally wrote: Awesome work! My, almost minor, comments below. > Currently on platforms designed for Windows, connections between CIO2 and > sensors are not properly defined in DSDT. This patch extends the ipu3-cio2 > driver to compensate by building software_node connections, parsing the > connection properties from the sensor's SSDB buffer. Drop trailing space here and in other places if any. > Suggested-by: Jordan Hand <jorhand@linux.microsoft.com> > > Signed-off-by: Daniel Scally <djrscally@gmail.com> In the tag block should not be blank lines. ... > * Built against media_tree instead of linus's tree - there's no T: entry in > maintainers for the ipu3-cio2 driver but I see there're recent changes in > media_tree so thought this was the better option. Make sense to include T: entry as well (maybe as a separate patch). ... > +config CIO2_BRIDGE > + bool "IPU3 CIO2 Sensors Bridge" > + depends on VIDEO_IPU3_CIO2 > + help > + This extension provides an API for the ipu3-cio2 driver to create > + connections to cameras that are hidden in SSDB buffer in ACPI. It > + can be used to enable support for cameras in detachable / hybrid > + devices that ship with Windows. > + > + Say y here if your device is a detachable / hybrid laptop that comes y -> Y > + with Windows installed by the OEM, for example the Microsoft Surface > + line, Lenovo Miix line or Dell 7285. Reads better in a form of an enumeration, like - Microsoft Surface (all existing) - Lenovo Miix (all existing) - Dell 7285 > + If in doubt, say n here. n -> N ... > -obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o > +obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2-driver.o > + > +ipu3-cio2-driver-objs += ipu3-cio2.o > + > +ifeq ($(CONFIG_CIO2_BRIDGE), y) > +ipu3-cio2-driver-objs += cio2-bridge.o > +endif objs is for userspace tools, here use ipu3-cio2-driver-y += ipu3-cio2.o ipu3-cio2-driver-$(CONFIG_CIO2_BRIDGE) = cio2-bridge.o And actually would be nice to keep the name, so means rename ipu3-cio2.c to something like cio2-main.c. In such case you will have: obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o ipu3-cio2-y += cio2-main.o ipu3-cio2-$(CONFIG_CIO2_BRIDGE) = cio2-bridge.o ... > +// SPDX-License-Identifier: GPL-2.0 Author line perhaps? ... > +/* > + * Extend this array with ACPI Hardware ID's of devices known to be > + * working > + */ > + Redundant blank like. > +static const struct ipu3_sensor supported_devices[] = { > + IPU3_SENSOR("INT33BE", "INT33BE:00"), > + IPU3_SENSOR("OVTI2680", "OVTI2680:00"), > + IPU3_SENSOR("OVTI5648", "OVTI5648:00") In such cases please leave comma at the last item as well. Easier to extend w/o an additional churn. On top of that, please avoid putting *instance* names, i.e. the second parameters in your macro call. What code should do is to take _HID (first parameter) and call acpi_dev_match_first_dev() or so. > +}; > +static struct software_node cio2_hid_node = { CIO2_HID, }; Here, nevertheless, comma can be removed, since update will need to change entire line anyway. > +static const char * const port_names[] = { > + "port0", "port1", "port2", "port3" > +}; + Comma (can be prolonged in the next / previous lines). ... > +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size) > +{ > + union acpi_object *obj; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct acpi_handle *handle = ACPI_HANDLE(dev); > + acpi_status status; > + int ret; > + > + status = acpi_evaluate_object(handle, id, NULL, &buffer); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + obj = buffer.pointer; > + if (!obj || obj->type != ACPI_TYPE_BUFFER) { !obj case doesn't require freeing and I would say the error message can be amended in such case. > + dev_err(dev, "Could't read acpi buffer\n"); > + ret = -ENODEV; > + goto err_free_buff; > + } > + > + if (obj->buffer.length > size) { > + dev_err(dev, "Given buffer is too small\n"); > + ret = -ENODEV; > + goto err_free_buff; > + } > + > + memcpy(data, obj->buffer.pointer, obj->buffer.length); > + kfree(buffer.pointer); > + > + return obj->buffer.length; > +err_free_buff: > + kfree(buffer.pointer); > + return ret; This may be deduplicated by adding ret = obj->buffer.length; in the success branch. In such case the label would be named out_free_buf. > +} ... > +static int create_fwnode_properties(struct sensor *sensor, > + struct sensor_bios_data *ssdb) > +{ > + struct property_entry *ep_props; > + struct property_entry *cio2_props; > + struct property_entry *dev_props; > + u32 *data_lanes; > + int i; > + /* device fwnode properties */ > + dev_props = kcalloc(3, sizeof(*dev_props), GFP_KERNEL); > + if (!dev_props) > + return -ENOMEM; > + > + dev_props[0] = PROPERTY_ENTRY_U32("clock-frequency", ssdb->mclkspeed); > + dev_props[1] = PROPERTY_ENTRY_U8("rotation", ssdb->degree); > + > + memcpy(sensor->dev_props, dev_props, sizeof(*dev_props) * 3); > + kfree(dev_props); Why you can't do it on stack? > + /* endpoint fwnode properties */ > + Redundant blank like > + data_lanes = devm_kmalloc_array(sensor->dev, ssdb->lanes, sizeof(u32), > + GFP_KERNEL); > + > + if (!data_lanes) > + return -ENOMEM; > + > + for (i = 0; i < ssdb->lanes; i++) > + data_lanes[i] = i + 1; > + ep_props = kcalloc(4, sizeof(*ep_props), GFP_KERNEL); > + if (!ep_props) > + return -ENOMEM; > + > + ep_props[0] = PROPERTY_ENTRY_U32("bus-type", 5); > + ep_props[1] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", > + data_lanes, > + ssdb->lanes); > + ep_props[2] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR]; > + > + memcpy(sensor->ep_props, ep_props, sizeof(*ep_props) * 4); > + kfree(ep_props); Why can't be done on stack? > + > + /* cio2 endpoint props */ > + > + cio2_props = kcalloc(3, sizeof(*cio2_props), GFP_KERNEL); > + if (!cio2_props) > + return -ENOMEM; > + > + cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", > + data_lanes, > + ssdb->lanes); > + cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2]; > + > + memcpy(sensor->cio2_props, cio2_props, sizeof(*cio2_props) * 3); > + kfree(cio2_props); Ditto. Something like union { ... cio2_properties[3]; } properties; memset(properties, 0, sizeof(properties)); (and please spell properties instead of props) > + return 0; > +} > + > +static int create_connection_swnodes(struct sensor *sensor, > + struct sensor_bios_data *ssdb) > +{ > + struct software_node *nodes; > + > + nodes = kcalloc(6, sizeof(*nodes), GFP_KERNEL); > + if (!nodes) > + return -ENOMEM; > + > + nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name, > + sensor->dev_props); > + nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0", > + &sensor->swnodes[SWNODE_SENSOR_HID]); > + nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0", > + &sensor->swnodes[SWNODE_SENSOR_PORT], > + sensor->ep_props); > + nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[ssdb->link], > + &cio2_hid_node); > + nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0", > + &sensor->swnodes[SWNODE_CIO2_PORT], > + sensor->cio2_props); > + > + memcpy(sensor->swnodes, nodes, sizeof(*nodes) * 6); > + kfree(nodes); Also, why not on stack? > + return 0; > +} ... > + for (j = 4; j >= 0; j--) > + software_node_unregister(&sensor->swnodes[j]); Seems we may need a simple helper for this (test_printf.c has similar case), so, what about do it here for now and probably then move to somewhere like swnode.h or whatever holds it. static inline software_node_unregister_nodes_reverse(const struct software_node *nodes) { unsigned int i = 0; while (nodes[i].name) i++; while (i--) software_node_unregister(&nodes[i]); } ... > + client = container_of(sensor->dev, struct i2c_client, dev); Shouldn't be rather i2c_verify_client() ? ... > + sensor->old_drv = container_of(sensor->dev->driver, struct i2c_driver, > + driver); to_i2c_driver() ? ... > + ret = 0; > + No need to have a blank line here (check other similar places in the code). > + for (i = 0; i < ARRAY_SIZE(supported_devices); i++) { > + adev = acpi_dev_get_first_match_dev(supported_devices[i].hid, > + NULL, -1); Keep it one line, easy to read. > + if (!adev) > + continue; > + > + dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev); > + if (!dev) { > + ret = -EPROBE_DEFER; > + goto err_rollback; > + } > + > + /* > + * We need to clone the driver of any sensors that we connect, > + * so if they're probing we need to wait until they're finished > + */ > + > + if (dev->links.status == DL_DEV_PROBING) { > + ret = -EPROBE_DEFER; > + goto err_free_dev; > + } > + > + /* > + * If a sensor has no driver, we want to continue to try and > + * link others > + */ > + sd = dev_get_drvdata(dev); > + if (!sd) > + goto cont_free_dev; > + > + sensor = &bridge.sensors[bridge.n_sensors]; > + sensor->dev = dev; > + > + snprintf(sensor->name, 20, "%s", supported_devices[i].hid); include/linux/mod_devicetable.h:454:#define I2C_NAME_SIZE 20 > + sensor->fwnode = fwnode_handle_get(dev->fwnode); > + if (!sensor->fwnode) > + goto err_free_dev; > + > + ret = get_acpi_ssdb_sensor_data(dev, &ssdb); > + if (ret) > + goto err_free_fwnode; > + > + ret = create_fwnode_properties(sensor, &ssdb); > + if (ret) > + goto err_free_fwnode; > + > + ret = create_connection_swnodes(sensor, &ssdb); > + if (ret) > + goto err_free_fwnode; > + > + ret = software_node_register_nodes(sensor->swnodes); > + if (ret) > + goto err_free_fwnode; > + > + fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]); > + if (!fwnode) { > + ret = -ENODEV; > + goto err_free_swnodes; > + } > + > + fwnode->secondary = ERR_PTR(-ENODEV); > + dev->fwnode = fwnode; > + > + ret = cio2_bridge_reprobe_sensor(sensor, i); > + if (ret) > + goto err_free_swnodes; > + pr_info("cio2-bridge: Found supported device %s\n", > + supported_devices[i].hid); Use dev_*() [dev_info()] instead of pr_*() ones. > + bridge.n_sensors++; > + continue; > +cont_free_dev: > + put_device(dev); > + continue; > +err_free_swnodes: > + for (j = 4; j >= 0; j--) > + software_node_unregister(&sensor->swnodes[j]); See above. > +err_free_fwnode: > + fwnode_handle_put(sensor->fwnode); > +err_free_dev: > + put_device(dev); > +err_rollback: > + /* > + * If an iteration of this loop results in -EPROBE_DEFER then > + * we need to roll back any sensors that were successfully > + * registered. Any other error and we'll skip that step, as > + * it seems better to have one successfully connected sensor. > + */ > + > + if (ret == -EPROBE_DEFER) > + cio2_bridge_unregister_sensors(); > + > + break; This is (all err_* labels) invariant to the loop (you break it), so, move out of the loop... > + } > + > + return ret; ...somewhere here. ... > + ret = software_node_register(&cio2_hid_node); > + if (ret < 0) { > + pr_err("cio2-bridge: Failed to register the CIO2 HID node\n"); dev_err() and so on. > + goto err_put_cio2; > + } ... > +#define __NO_VERSION__ What is this for? ... > +#define NODE_SENSOR(_HID, _PROPS) \ > + ((const struct software_node) { \ > + .name = _HID, \ > + .properties = _PROPS \ Leave comma. > + }) ... > +#define IPU3_SENSOR(_HID, _CLIENT) \ > + { \ > + .hid = _HID, \ > + .i2c_id = { \ > + {_CLIENT, 0}, \ > + { }, \ No comma for terminator type of entries. > + } \ Leave comma. > + } ... > +struct ipu3_sensor { > + const char hid[20]; Shouldn't be rather 9? #define ACPI_ID_LEN 9 > + const struct i2c_device_id i2c_id[2]; > +}; ... > +struct sensor { > + char name[20]; I guess it's predefined by I²C ID. > + struct device *dev; > + struct software_node swnodes[6]; > + struct property_entry dev_props[3]; > + struct property_entry ep_props[4]; > + struct property_entry cio2_props[3]; I'm now wondering why you can't simply put properties directly to here and do that kcalloc / memcpy() in few functions? I mean to drop those calls and assign properties directly. You even won't need to memset() and stack for them! > + struct fwnode_handle *fwnode; > + struct i2c_driver *old_drv; > + struct i2c_driver new_drv; > +}; ... > +} __attribute__((__packed__)); __packed is enough (it's in compiler.h IIRC). ... > struct cio2_device *cio2; > + struct fwnode_handle *endpoint; > int r; Reverse xmas tree order. ... > + endpoint = fwnode_graph_get_next_endpoint(pci_dev->dev.fwnode, NULL); dev_fwnode() > + if (!endpoint) { > + r = cio2_bridge_build(pci_dev); > + if (r) > + return r; > + } ... > +#ifdef CONFIG_CIO2_BRIDGE IS_ENABLED() > + > +int cio2_bridge_build(struct pci_dev *cio2); > +void cio2_bridge_burn(struct pci_dev *cio2); > + > +#else > + > +int cio2_bridge_build(struct pci_dev *cio2) > +{ > + return 0; > +} One line. > +void cio2_bridge_burn(struct pci_dev *cio2) > +{ > +} One line. > +#endif
On Thu, Oct 01, 2020 at 10:33:26AM +0100, Daniel Scally wrote: ... > This patch is dependent on another (which implements the software node graph > family of functions): > > https://lore.kernel.org/linux-media/20200915232827.3416-1-djrscally@gmail.com/ More thoughts about the (future) series. Can you rather do a series of patches to understand what is going on here. I see something like this: patch 1. Add unregister_nodes_reverse() 2. Use above in test_printf.c 3. Add that graph support patch 4. Add T: to MAINTAINERS for IPU3-CIO2 5. Add this patch
Hi Andy - sorry for delayed reply, busy week On 01/10/2020 18:37, Andy Shevchenko wrote: > On Thu, Oct 01, 2020 at 10:33:26AM +0100, Daniel Scally wrote: > > Awesome work! > My, almost minor, comments below. Thanks as always for your help - great comments. I'll work through and make the changes you suggest to this code and also... >> * Built against media_tree instead of linus's tree - there's no T: entry in >> maintainers for the ipu3-cio2 driver but I see there're recent changes in >> media_tree so thought this was the better option. > Make sense to include T: entry as well (maybe as a separate patch). ...I agree with your other email re. turning this into a series and making the additional changes you suggested, so I'll do that too for the v3. Just a couple comments / queries: >> +// SPDX-License-Identifier: GPL-2.0 > Author line perhaps? You mean literally just like /* Authored By: Dan Scally */ or something? OK, no problem. >> +static const struct ipu3_sensor supported_devices[] = { >> + IPU3_SENSOR("INT33BE", "INT33BE:00"), >> + IPU3_SENSOR("OVTI2680", "OVTI2680:00"), >> + IPU3_SENSOR("OVTI5648", "OVTI5648:00") > In such cases please leave comma at the last item as well. Easier to extend w/o > an additional churn. > > On top of that, please avoid putting *instance* names, i.e. the second > parameters in your macro call. What code should do is to take _HID (first > parameter) and call acpi_dev_match_first_dev() or so. Yeah I was originally using the i2c_client's name field (this comes into play during cio2_bridge_reprobe_sensor()) but the matching refused to work using anything but a string literal. Let me take another look at this then. >> +static struct software_node cio2_hid_node = { CIO2_HID, }; > Here, nevertheless, comma can be removed, since update will need to change > entire line anyway. Trailing commas where lists can be extended, otherwise none - got it. >> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size) >> +{ >> + union acpi_object *obj; >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> + struct acpi_handle *handle = ACPI_HANDLE(dev); >> + acpi_status status; >> + int ret; >> + >> + status = acpi_evaluate_object(handle, id, NULL, &buffer); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + obj = buffer.pointer; >> + if (!obj || obj->type != ACPI_TYPE_BUFFER) { > !obj case doesn't require freeing and I would say the error message can be > amended in such case. Oops - you suggested that in the last version too and I missed it, sorry about that. I'll split those two failure modes out. >> + for (j = 4; j >= 0; j--) >> + software_node_unregister(&sensor->swnodes[j]); > Seems we may need a simple helper for this (test_printf.c has similar case), > so, what about do it here for now and probably then move to somewhere like > swnode.h or whatever holds it. > > static inline software_node_unregister_nodes_reverse(const struct software_node *nodes) > { > unsigned int i = 0; > > while (nodes[i].name) > i++; > while (i--) > software_node_unregister(&nodes[i]); > } Yeah this is a good idea; I see you suggest a new patch for it in your other email; I'll do a series in the future and add this to the swnode source file at the same time as doing the changes that you, Heikki and Sakari suggested for the other patch. >> +#define __NO_VERSION__ > What is this for? Somehow I got on the outdated page for compiling modules spanning multiple files in the linux kernel module programming guide; it's suggested there but not in the newer version. I shall remove it. >> + struct device *dev; >> + struct software_node swnodes[6]; >> + struct property_entry dev_props[3]; >> + struct property_entry ep_props[4]; >> + struct property_entry cio2_props[3]; > I'm now wondering why you can't simply put properties directly to here and do > that kcalloc / memcpy() in few functions? I mean to drop those calls and assign > properties directly. You even won't need to memset() and stack for them! I thought you were hinting that I should use kcalloc in the comments from the last patch to get an array of zero valued entries but I guess I misunderstood - I can just memset these arrays to 0 and assign all but the last entry directly and that seems to work fine, so I'll switch to that. >> struct cio2_device *cio2; >> + struct fwnode_handle *endpoint; >> int r; > Reverse xmas tree order. TIL - thanks, I'll do a pass over the rest of it and make sure that I follow that everywhere else too >> + endpoint = fwnode_graph_get_next_endpoint(pci_dev->dev.fwnode, NULL); > dev_fwnode() I avoided that thinking there might be a case where a CIO2 device has a fwnode but not endpoints defined for some reason, but I'm not familiar enough to judge whether that situation will ever occur - if it's safe to do it that way then I'll switch it over.
On Thu, Oct 08, 2020 at 10:51:46PM +0100, Dan Scally wrote: > On 01/10/2020 18:37, Andy Shevchenko wrote: > > On Thu, Oct 01, 2020 at 10:33:26AM +0100, Daniel Scally wrote: > > Awesome work! > > My, almost minor, comments below. > Thanks as always for your help - great comments. I'll work through and > make the changes you suggest to this code and also... > >> * Built against media_tree instead of linus's tree - there's no T: entry in > >> maintainers for the ipu3-cio2 driver but I see there're recent changes in > >> media_tree so thought this was the better option. > > Make sense to include T: entry as well (maybe as a separate patch). > > ...I agree with your other email re. turning this into a series and > making the additional changes you suggested, so I'll do that too for the v3. I forgot to mention module rename as a separate patch. So, something like 6 (or more) in a series I would expect. ... > >> +// SPDX-License-Identifier: GPL-2.0 > > Author line perhaps? > > You mean literally just like /* Authored By: Dan Scally */ or something? > OK, no problem. * Author: Dan Scally <foo@bar.com> ... > >> +static const struct ipu3_sensor supported_devices[] = { > >> + IPU3_SENSOR("INT33BE", "INT33BE:00"), > >> + IPU3_SENSOR("OVTI2680", "OVTI2680:00"), > >> + IPU3_SENSOR("OVTI5648", "OVTI5648:00") > > In such cases please leave comma at the last item as well. Easier to extend w/o > > an additional churn. > > > > On top of that, please avoid putting *instance* names, i.e. the second > > parameters in your macro call. What code should do is to take _HID (first > > parameter) and call acpi_dev_match_first_dev() or so. > Yeah I was originally using the i2c_client's name field (this comes into > play during cio2_bridge_reprobe_sensor()) but the matching refused to > work using anything but a string literal. Let me take another look at > this then. I meant that you get an instance name from the first found device, like char instance_name[I2C...]; adev = first_match_dev(); if (adev) { snprintf(instance_name, ..., acpi_dev_name(adev)); ... } else { ... } ... > >> +static struct software_node cio2_hid_node = { CIO2_HID, }; > > Here, nevertheless, comma can be removed, since update will need to change > > entire line anyway. > Trailing commas where lists can be extended, otherwise none - got it. Just use a common sense. Simple extrapolate it to the next change, if any, among the same lines. ... > >> + for (j = 4; j >= 0; j--) > >> + software_node_unregister(&sensor->swnodes[j]); > > Seems we may need a simple helper for this (test_printf.c has similar case), > > so, what about do it here for now and probably then move to somewhere like > > swnode.h or whatever holds it. > > > > static inline software_node_unregister_nodes_reverse(const struct software_node *nodes) > > { > > unsigned int i = 0; > > > > while (nodes[i].name) > > i++; > > while (i--) > > software_node_unregister(&nodes[i]); > > } > Yeah this is a good idea; I see you suggest a new patch for it in your > other email; I'll do a series in the future and add this to the swnode > source file at the same time as doing the changes that you, Heikki and > Sakari suggested for the other patch. Yes, thanks! ... > >> + struct software_node swnodes[6]; > >> + struct property_entry dev_props[3]; > >> + struct property_entry ep_props[4]; > >> + struct property_entry cio2_props[3]; > > I'm now wondering why you can't simply put properties directly to here and do > > that kcalloc / memcpy() in few functions? I mean to drop those calls and assign > > properties directly. You even won't need to memset() and stack for them! > I thought you were hinting that I should use kcalloc in the comments > from the last patch to get an array of zero valued entries but I guess I > misunderstood - I can just memset these arrays to 0 and assign all but > the last entry directly and that seems to work fine, so I'll switch to that. The idea behind that any kcalloc() or kzalloc() against a container (whatever data structure that has those property arrays) will do it for you. So, I think neither kcalloc() nor stack is needed. Instantiate properties directly in the arrays of sensor data structure. ... > >> + endpoint = fwnode_graph_get_next_endpoint(pci_dev->dev.fwnode, NULL); > > dev_fwnode() > I avoided that thinking there might be a case where a CIO2 device has a > fwnode but not endpoints defined for some reason, but I'm not familiar > enough to judge whether that situation will ever occur - if it's safe to > do it that way then I'll switch it over. I meant endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&pci_dev->dev), NULL);
On 09/10/2020 13:44, Andy Shevchenko wrote: >> ...I agree with your other email re. turning this into a series and >> making the additional changes you suggested, so I'll do that too for the v3. > I forgot to mention module rename as a separate patch. So, something like 6 > (or more) in a series I would expect. Yeah no problem, I'll do that too. > >>>> +// SPDX-License-Identifier: GPL-2.0 >>> Author line perhaps? >> You mean literally just like /* Authored By: Dan Scally */ or something? >> OK, no problem. > * Author: Dan Scally <foo@bar.com> Okedokey >>>> +static const struct ipu3_sensor supported_devices[] = { >>>> + IPU3_SENSOR("INT33BE", "INT33BE:00"), >>>> + IPU3_SENSOR("OVTI2680", "OVTI2680:00"), >>>> + IPU3_SENSOR("OVTI5648", "OVTI5648:00") >>> In such cases please leave comma at the last item as well. Easier to extend w/o >>> an additional churn. >>> >>> On top of that, please avoid putting *instance* names, i.e. the second >>> parameters in your macro call. What code should do is to take _HID (first >>> parameter) and call acpi_dev_match_first_dev() or so. >> Yeah I was originally using the i2c_client's name field (this comes into >> play during cio2_bridge_reprobe_sensor()) but the matching refused to >> work using anything but a string literal. Let me take another look at >> this then. > I meant that you get an instance name from the first found device, like > > char instance_name[I2C...]; > > adev = first_match_dev(); > if (adev) { > snprintf(instance_name, ..., acpi_dev_name(adev)); > ... > } else { > ... > } Ah, yes, this is better than messing around getting the i2c_client - thanks >> I thought you were hinting that I should use kcalloc in the comments >> from the last patch to get an array of zero valued entries but I guess I >> misunderstood - I can just memset these arrays to 0 and assign all but >> the last entry directly and that seems to work fine, so I'll switch to that. > The idea behind that any kcalloc() or kzalloc() against a container (whatever > data structure that has those property arrays) will do it for you. So, I think > neither kcalloc() nor stack is needed. Instantiate properties directly in the > arrays of sensor data structure. Will do >>>> + endpoint = fwnode_graph_get_next_endpoint(pci_dev->dev.fwnode, NULL); >>> dev_fwnode() >> I avoided that thinking there might be a case where a CIO2 device has a >> fwnode but not endpoints defined for some reason, but I'm not familiar >> enough to judge whether that situation will ever occur - if it's safe to >> do it that way then I'll switch it over. > I meant > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&pci_dev->dev), NULL); Ah! Of course, I'll make that change too, thanks
diff --git a/MAINTAINERS b/MAINTAINERS index 43a025039..0b4481ef2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8848,6 +8848,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER M: Yong Zhi <yong.zhi@intel.com> M: Sakari Ailus <sakari.ailus@linux.intel.com> M: Bingbu Cao <bingbu.cao@intel.com> +M: Dan Scally <djrscally@gmail.com> R: Tianshu Qiu <tian.shu.qiu@intel.com> L: linux-media@vger.kernel.org S: Maintained diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig index 82d7f17e6..04b2d579e 100644 --- a/drivers/media/pci/intel/ipu3/Kconfig +++ b/drivers/media/pci/intel/ipu3/Kconfig @@ -16,3 +16,18 @@ config VIDEO_IPU3_CIO2 Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2 connected camera. The module will be called ipu3-cio2. + +config CIO2_BRIDGE + bool "IPU3 CIO2 Sensors Bridge" + depends on VIDEO_IPU3_CIO2 + help + This extension provides an API for the ipu3-cio2 driver to create + connections to cameras that are hidden in SSDB buffer in ACPI. It + can be used to enable support for cameras in detachable / hybrid + devices that ship with Windows. + + Say y here if your device is a detachable / hybrid laptop that comes + with Windows installed by the OEM, for example the Microsoft Surface + line, Lenovo Miix line or Dell 7285. + + If in doubt, say n here. diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile index 98ddd5bea..4810b34bf 100644 --- a/drivers/media/pci/intel/ipu3/Makefile +++ b/drivers/media/pci/intel/ipu3/Makefile @@ -1,2 +1,8 @@ # SPDX-License-Identifier: GPL-2.0-only -obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o +obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2-driver.o + +ipu3-cio2-driver-objs += ipu3-cio2.o + +ifeq ($(CONFIG_CIO2_BRIDGE), y) +ipu3-cio2-driver-objs += cio2-bridge.o +endif diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c new file mode 100644 index 000000000..14d722dea --- /dev/null +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c @@ -0,0 +1,433 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/fwnode.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/property.h> +#include <media/v4l2-subdev.h> + +#include "cio2-bridge.h" + +/* + * Extend this array with ACPI Hardware ID's of devices known to be + * working + */ + +static const struct ipu3_sensor supported_devices[] = { + IPU3_SENSOR("INT33BE", "INT33BE:00"), + IPU3_SENSOR("OVTI2680", "OVTI2680:00"), + IPU3_SENSOR("OVTI5648", "OVTI5648:00") +}; + +static struct software_node cio2_hid_node = { CIO2_HID, }; + +static struct cio2_bridge bridge; + +static const char * const port_names[] = { + "port0", "port1", "port2", "port3" +}; + +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]), +}; + +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size) +{ + union acpi_object *obj; + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + struct acpi_handle *handle = ACPI_HANDLE(dev); + acpi_status status; + int ret; + + status = acpi_evaluate_object(handle, id, NULL, &buffer); + if (ACPI_FAILURE(status)) + return -ENODEV; + + obj = buffer.pointer; + if (!obj || obj->type != ACPI_TYPE_BUFFER) { + dev_err(dev, "Could't read acpi buffer\n"); + ret = -ENODEV; + goto err_free_buff; + } + + if (obj->buffer.length > size) { + dev_err(dev, "Given buffer is too small\n"); + ret = -ENODEV; + goto err_free_buff; + } + + memcpy(data, obj->buffer.pointer, obj->buffer.length); + kfree(buffer.pointer); + + return obj->buffer.length; +err_free_buff: + kfree(buffer.pointer); + return ret; +} + +static int get_acpi_ssdb_sensor_data(struct device *dev, + struct sensor_bios_data *sensor) +{ + struct sensor_bios_data_packed sensor_data; + int ret; + + ret = read_acpi_block(dev, "SSDB", &sensor_data, sizeof(sensor_data)); + if (ret < 0) + return ret; + + sensor->link = sensor_data.link; + sensor->lanes = sensor_data.lanes; + sensor->mclkspeed = sensor_data.mclkspeed; + sensor->degree = sensor_data.degree; + + return 0; +} + +static int create_fwnode_properties(struct sensor *sensor, + struct sensor_bios_data *ssdb) +{ + struct property_entry *ep_props; + struct property_entry *cio2_props; + struct property_entry *dev_props; + u32 *data_lanes; + int i; + + /* device fwnode properties */ + dev_props = kcalloc(3, sizeof(*dev_props), GFP_KERNEL); + if (!dev_props) + return -ENOMEM; + + dev_props[0] = PROPERTY_ENTRY_U32("clock-frequency", ssdb->mclkspeed); + dev_props[1] = PROPERTY_ENTRY_U8("rotation", ssdb->degree); + + memcpy(sensor->dev_props, dev_props, sizeof(*dev_props) * 3); + kfree(dev_props); + + /* endpoint fwnode properties */ + + data_lanes = devm_kmalloc_array(sensor->dev, ssdb->lanes, sizeof(u32), + GFP_KERNEL); + + if (!data_lanes) + return -ENOMEM; + + for (i = 0; i < ssdb->lanes; i++) + data_lanes[i] = i + 1; + + ep_props = kcalloc(4, sizeof(*ep_props), GFP_KERNEL); + if (!ep_props) + return -ENOMEM; + + ep_props[0] = PROPERTY_ENTRY_U32("bus-type", 5); + ep_props[1] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", + data_lanes, + ssdb->lanes); + ep_props[2] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR]; + + memcpy(sensor->ep_props, ep_props, sizeof(*ep_props) * 4); + kfree(ep_props); + + /* cio2 endpoint props */ + + cio2_props = kcalloc(3, sizeof(*cio2_props), GFP_KERNEL); + if (!cio2_props) + return -ENOMEM; + + cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", + data_lanes, + ssdb->lanes); + cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2]; + + memcpy(sensor->cio2_props, cio2_props, sizeof(*cio2_props) * 3); + kfree(cio2_props); + + return 0; +} + +static int create_connection_swnodes(struct sensor *sensor, + struct sensor_bios_data *ssdb) +{ + struct software_node *nodes; + + nodes = kcalloc(6, sizeof(*nodes), GFP_KERNEL); + if (!nodes) + return -ENOMEM; + + nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name, + sensor->dev_props); + nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0", + &sensor->swnodes[SWNODE_SENSOR_HID]); + nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0", + &sensor->swnodes[SWNODE_SENSOR_PORT], + sensor->ep_props); + nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[ssdb->link], + &cio2_hid_node); + nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0", + &sensor->swnodes[SWNODE_CIO2_PORT], + sensor->cio2_props); + + memcpy(sensor->swnodes, nodes, sizeof(*nodes) * 6); + kfree(nodes); + + return 0; +} + +static void cio2_bridge_unregister_sensors(void) +{ + int i, j; + struct sensor *sensor; + + for (i = 0; i < bridge.n_sensors; i++) { + sensor = &bridge.sensors[i]; + + for (j = 4; j >= 0; j--) + software_node_unregister(&sensor->swnodes[j]); + + /* + * Give the sensor its original fwnode back or the next time + * it's probed will fail, because ACPI matching doesn't work + * when your fwnode doesn't have acpi_device_fwnode_ops. + */ + sensor->dev->fwnode = sensor->fwnode; + fwnode_handle_put(sensor->fwnode); + + device_release_driver(sensor->dev); + i2c_del_driver(&sensor->new_drv); + device_reprobe(sensor->dev); + put_device(sensor->dev); + } +} + +/* + * We have to reprobe the sensor in order for .probe() calls to be able to read + * the fwnode properties we set, but having just overwritten the ACPI fwnode + * the usual matching won't work by default. We need to clone the existing + * driver but add an i2c_device_id so the matching works. + */ +static int cio2_bridge_reprobe_sensor(struct sensor *sensor, int index) +{ + struct i2c_client *client; + int ret; + + client = container_of(sensor->dev, struct i2c_client, dev); + + sensor->old_drv = container_of(sensor->dev->driver, struct i2c_driver, + driver); + + sensor->new_drv.driver.name = supported_devices[index].i2c_id[0].name; + sensor->new_drv.probe_new = sensor->old_drv->probe_new; + sensor->new_drv.remove = sensor->old_drv->remove; + sensor->new_drv.id_table = supported_devices[index].i2c_id; + + device_release_driver(sensor->dev); + + ret = i2c_add_driver(&sensor->new_drv); + if (ret) + return ret; + + ret = device_reprobe(sensor->dev); + if (ret) + goto err_remove_new_drv; + + return 0; + +err_remove_new_drv: + i2c_del_driver(&sensor->new_drv); + + return ret; +} + +static int connect_supported_devices(void) +{ + struct acpi_device *adev; + struct device *dev; + struct sensor_bios_data ssdb; + struct sensor *sensor; + struct fwnode_handle *fwnode; + struct v4l2_subdev *sd; + int i, j, ret; + + ret = 0; + + for (i = 0; i < ARRAY_SIZE(supported_devices); i++) { + adev = acpi_dev_get_first_match_dev(supported_devices[i].hid, + NULL, -1); + if (!adev) + continue; + + dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev); + if (!dev) { + ret = -EPROBE_DEFER; + goto err_rollback; + } + + /* + * We need to clone the driver of any sensors that we connect, + * so if they're probing we need to wait until they're finished + */ + + if (dev->links.status == DL_DEV_PROBING) { + ret = -EPROBE_DEFER; + goto err_free_dev; + } + + /* + * If a sensor has no driver, we want to continue to try and + * link others + */ + sd = dev_get_drvdata(dev); + if (!sd) + goto cont_free_dev; + + sensor = &bridge.sensors[bridge.n_sensors]; + sensor->dev = dev; + + snprintf(sensor->name, 20, "%s", supported_devices[i].hid); + + sensor->fwnode = fwnode_handle_get(dev->fwnode); + if (!sensor->fwnode) + goto err_free_dev; + + ret = get_acpi_ssdb_sensor_data(dev, &ssdb); + if (ret) + goto err_free_fwnode; + + ret = create_fwnode_properties(sensor, &ssdb); + if (ret) + goto err_free_fwnode; + + ret = create_connection_swnodes(sensor, &ssdb); + if (ret) + goto err_free_fwnode; + + ret = software_node_register_nodes(sensor->swnodes); + if (ret) + goto err_free_fwnode; + + fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]); + if (!fwnode) { + ret = -ENODEV; + goto err_free_swnodes; + } + + fwnode->secondary = ERR_PTR(-ENODEV); + dev->fwnode = fwnode; + + ret = cio2_bridge_reprobe_sensor(sensor, i); + if (ret) + goto err_free_swnodes; + + pr_info("cio2-bridge: Found supported device %s\n", + supported_devices[i].hid); + + bridge.n_sensors++; + continue; +cont_free_dev: + put_device(dev); + continue; +err_free_swnodes: + for (j = 4; j >= 0; j--) + software_node_unregister(&sensor->swnodes[j]); +err_free_fwnode: + fwnode_handle_put(sensor->fwnode); +err_free_dev: + put_device(dev); +err_rollback: + /* + * If an iteration of this loop results in -EPROBE_DEFER then + * we need to roll back any sensors that were successfully + * registered. Any other error and we'll skip that step, as + * it seems better to have one successfully connected sensor. + */ + + if (ret == -EPROBE_DEFER) + cio2_bridge_unregister_sensors(); + + break; + } + + return ret; +} + +int cio2_bridge_build(struct pci_dev *cio2) +{ + struct fwnode_handle *fwnode; + int ret; + + pci_dev_get(cio2); + + ret = software_node_register(&cio2_hid_node); + if (ret < 0) { + pr_err("cio2-bridge: Failed to register the CIO2 HID node\n"); + goto err_put_cio2; + } + + ret = connect_supported_devices(); + if (ret == -EPROBE_DEFER) + goto err_unregister_cio2; + + if (bridge.n_sensors == 0) { + ret = -EPROBE_DEFER; + goto err_unregister_cio2; + } + + pr_info("cio2-bridge: Connected %d cameras\n", bridge.n_sensors); + + fwnode = software_node_fwnode(&cio2_hid_node); + if (!fwnode) { + pr_err("Error getting fwnode from cio2 software_node\n"); + ret = -ENODEV; + goto err_unregister_sensors; + } + + /* + * We store the pci_dev's existing fwnode, because in the event we + * want to reload the ipu3-cio2 driver we need to give the device its + * original fwnode back to prevent problems. + */ + + bridge.cio2_fwnode = fwnode_handle_get(cio2->dev.fwnode); + + fwnode->secondary = ERR_PTR(-ENODEV); + cio2->dev.fwnode = fwnode; + + return 0; + +err_unregister_sensors: + cio2_bridge_unregister_sensors(); +err_unregister_cio2: + software_node_unregister(&cio2_hid_node); +err_put_cio2: + pci_dev_put(cio2); + + return ret; +} + +void cio2_bridge_burn(struct pci_dev *cio2) +{ + cio2->dev.fwnode = bridge.cio2_fwnode; + fwnode_handle_put(bridge.cio2_fwnode); + pci_dev_put(cio2); + + cio2_bridge_unregister_sensors(); + + software_node_unregister(&cio2_hid_node); +} diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h new file mode 100644 index 000000000..16ff262f3 --- /dev/null +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h @@ -0,0 +1,110 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __CIO2_BRIDGE_H +#define __CIO2_BRIDGE_H +#define __NO_VERSION__ + +#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_SENSOR(_HID, _PROPS) \ + ((const struct software_node) { \ + .name = _HID, \ + .properties = _PROPS \ + }) + +#define NODE_PORT(_PORT, _SENSOR_NODE) \ + ((const struct software_node) { \ + _PORT, \ + _SENSOR_NODE, \ + }) + +#define NODE_ENDPOINT(_EP, _PORT, _PROPS) \ + ((const struct software_node) { \ + _EP, \ + _PORT, \ + _PROPS, \ + }) + +#define IPU3_SENSOR(_HID, _CLIENT) \ + { \ + .hid = _HID, \ + .i2c_id = { \ + {_CLIENT, 0}, \ + { }, \ + } \ + } + +struct ipu3_sensor { + const char hid[20]; + const struct i2c_device_id i2c_id[2]; +}; + +struct sensor { + char name[20]; + struct device *dev; + struct software_node swnodes[6]; + struct property_entry dev_props[3]; + struct property_entry ep_props[4]; + struct property_entry cio2_props[3]; + struct fwnode_handle *fwnode; + struct i2c_driver *old_drv; + struct i2c_driver new_drv; +}; + +struct cio2_bridge { + int n_sensors; + struct sensor sensors[MAX_CONNECTED_DEVICES]; + struct fwnode_handle *cio2_fwnode; +}; + +/* 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__)); + +/* Fields needed by bridge driver */ +struct sensor_bios_data { + struct device *dev; + u8 link; + u8 lanes; + u8 degree; + u32 mclkspeed; +}; + +#endif diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 4e598e937..ea18d8c81 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1711,8 +1711,26 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, const struct pci_device_id *id) { struct cio2_device *cio2; + struct fwnode_handle *endpoint; int r; + /* + * On some platforms no connections to sensors are defined in firmware, + * if the device has no endpoints then we can try to build those as + * software_nodes parsed from SSDB. + * + * This may EPROBE_DEFER if supported devices are found defined in ACPI + * but not yet ready for use (either not attached to the i2c bus yet, + * or not done probing themselves). + */ + + endpoint = fwnode_graph_get_next_endpoint(pci_dev->dev.fwnode, NULL); + if (!endpoint) { + r = cio2_bridge_build(pci_dev); + if (r) + return r; + } + cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL); if (!cio2) return -ENOMEM; @@ -1820,6 +1838,9 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) { struct cio2_device *cio2 = pci_get_drvdata(pci_dev); + if (is_software_node(pci_dev->dev.fwnode)) + cio2_bridge_burn(pci_dev); + media_device_unregister(&cio2->media_dev); v4l2_async_notifier_unregister(&cio2->notifier); v4l2_async_notifier_cleanup(&cio2->notifier); diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h index 549b08f88..45791208d 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h @@ -436,4 +436,22 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq) return container_of(vq, struct cio2_queue, vbq); } +#ifdef CONFIG_CIO2_BRIDGE + +int cio2_bridge_build(struct pci_dev *cio2); +void cio2_bridge_burn(struct pci_dev *cio2); + +#else + +int cio2_bridge_build(struct pci_dev *cio2) +{ + return 0; +} + +void cio2_bridge_burn(struct pci_dev *cio2) +{ +} + +#endif + #endif
Currently on platforms designed for Windows, connections between CIO2 and sensors are not properly defined in DSDT. This patch extends the ipu3-cio2 driver to compensate by building software_node connections, parsing the connection properties from the sensor's SSDB buffer. Suggested-by: Jordan Hand <jorhand@linux.microsoft.com> Signed-off-by: Daniel Scally <djrscally@gmail.com> --- This patch is dependent on another (which implements the software node graph family of functions): https://lore.kernel.org/linux-media/20200915232827.3416-1-djrscally@gmail.com/ Major changes for the v2 (on top of a myriad of fixes probably too minor to call out - thank you everybody for the comments) * Built against media_tree instead of linus's tree - there's no T: entry in maintainers for the ipu3-cio2 driver but I see there're recent changes in media_tree so thought this was the better option. * Moved from a separate module to integrating the changes into the ipu3-cio2 driver itself. There was some difference of opinion about the desirability of this, but I think that this option won out. I have attempted to address the concerns about bloating everyone's kernel by conditionally compiling the extension in based on config settings; although I don't have an ipu3 device that doesn't need these changes (so haven't been able to test that properly) - it certainly seems to be working as expected. Compiling things in like this did require renaming either the module or the ipu3-cio2.c file, I chose the module to keep patch size down - let me know if that was the wrong choice. * Error handling has been switched so each function cleans itself up properly rather than one single "clean up everything" function. * Indentation fixed for create_endpoint_properties() * Creation of the software_nodes moved to its own function * Clock frequency property moved to the device fwnode instead of the endpoint's * Reprobe of the sensors added at the end of connect_supported_devices(). This is needed to allow probe() calls from the drivers to pick up new properties. This required some unusual behaviour; because we're overwriting the fwnode of the device ACPI device to driver matching will no longer work. To work around that, I'm cloning the driver but adding an i2c_device_id table to it so that fallback i2c matching works. This driver is unregistered if the module is removed. I don't especially like this solution, but I couldn't see a better way to solve this problem. Suggested changes that I didn't make: Sakari - link-frequencies doesn't seem to be available in the SSDB buffer or anywhere else that I can see in DSDT unfortunately, so I didn't add that as a property. Also, I left the iterator as an int until that discussion came to a conclusive answer. MAINTAINERS | 1 + drivers/media/pci/intel/ipu3/Kconfig | 15 + drivers/media/pci/intel/ipu3/Makefile | 8 +- drivers/media/pci/intel/ipu3/cio2-bridge.c | 433 +++++++++++++++++++++ drivers/media/pci/intel/ipu3/cio2-bridge.h | 110 ++++++ drivers/media/pci/intel/ipu3/ipu3-cio2.c | 21 + drivers/media/pci/intel/ipu3/ipu3-cio2.h | 18 + 7 files changed, 605 insertions(+), 1 deletion(-) create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h