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@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx> 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 -- With Best Regards, Andy Shevchenko