On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote: > On 17/09/2020 11:33, Sakari Ailus wrote: I will do better review for next version, assuming you will Cc reviewers and TWIMC people. Below is like small part of comments I may give to the code. ... > > The ones I know require PMIC control done in software (not even > > sensors are accessible without that). > So far we've just been getting the sensor drivers themselves to toggle > the gpio pins that turn the PMIC on (those pins are listed against the > PMIC's _CRS, and we've been finding those by evaluating the sensor's > _DEP) - once that's done the cameras show up on i2c and,with the bridge > driver installed, you can use libcamera to take photos. Do I understand correctly that you are able to get pictures from the camera hardware? ... > > a module and not enlarge everyone's kernel, and the initialisation would at > > the same time take place before the rest of what the CIO2 driver does in > > probe. > I thought of that as well, but wasn't sure which was preferable. I can > compress it into the CIO2 driver though sure. Sakari, I tend to agree with Dan and have the board file separated from the driver and even framework. ... > > Cc Andy, too. Thanks! ... > >> I wanted to raise this as an RFC as although I don't think it's ready for > >> integration it has some things that I'd like feedback on, in particular the > >> method I chose to make the module be auto-inserted. A more ideal method would > >> have been to have the driver be an ACPI driver for the INT343E device, but each > > What do you think this device does represent? Devices whose status is > > always zero may exist in the table even if they would not be actually > > present. > > > > CIO2 is a PCI device and it has no ACPI (or PNP) ID, or at least should not > > have one. > This is the ACPI entry I mean: > > Device (CIO2) > { > Method (_STA, 0, NotSerialized) // _STA: Status > { > If ((CIOE == One)) > { > Return (0x0F) > } > Else > { > Return (Zero) > } > } > > Name (_HID, "INT343E") // _HID: Hardware ID > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > { > Name (CBUF, ResourceTemplate () > { > Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, _Y15) > { > 0x00000010, > } > Memory32Fixed (ReadWrite, > 0xFE400000, // Address Base > 0x00010000, // Address Length > ) > }) > CreateDWordField (CBUF, \_SB.PCI0.CIO2._CRS._Y15._INT, CIOV) // _INT: Interrupts > CIOV = CIOI /* \CIOI */ > Return (CBUF) /* \_SB_.PCI0.CIO2._CRS.CBUF */ > } > } Ah, I think you misinterpreted the meaning of above. The above is a switch how camera device appears either as PCI or an ACPI. So, it effectively means you should *not* have any relation for this HID until you find a platform where the device is for real enumerated via ACPI. ... > >> +static int cio2_probe_can_progress(struct pci_dev *pci_dev) > >> +{ > >> + void *sensor; 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( Besides the fact that castings from or to void * are implicit in C, the proper use of list API should have pretty well defined type of lvalue. > >> + &pci_dev->dev.links.consumers, > >> + struct dev_links_info, > >> + consumers); > > Please wrap so it's under 80. > > > Will do > >> + > >> + if (!sensor) > >> + return -EPROBE_DEFER; > >> + } > >> + > >> + return 0; > >> +} ... > >> + if (!IS_ENABLED(CONFIG_ACPI)) { > >> + r = cio2_parse_firmware(cio2); > >> + if (r) > >> + goto fail_clean_notifier; > >> + } How comes? ... > >> \ No newline at end of file ??? Be sure you are using good editor. ... > >> +#include <acpi/acpi_bus.h> Redundant. ACPI headers are designed the way that you are using a single header in Linux kernel for all. It might be different in drivers/acpi stuff, but not in general. > >> +#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> Please, keep them sorted. And since it's for media, the media inclusion may be placed last in a separate group. ... > >> +#define PROPERTY_ENTRY_NULL \ > >> +((const struct property_entry) { }) > > Alignment. Same appears to apply to other macros (please indent). > Yep > > > >> +#define SOFTWARE_NODE_NULL \ > >> +((const struct software_node) { }) Why?! ... > >> +struct software_node cio2_hid_node = { CIO2_HID, }; static ? Same for other global variables. ... > >> +struct cio2_bridge bridge = { 0, }; When define as static the assignment will not be needed. ... > >> +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 *dev_handle = ACPI_HANDLE(dev); Usually we use simple handle if there is no ambiguous reading. > >> + int status; Should be acpi_status > >> + u32 buffer_length; > >> + > >> + status = acpi_evaluate_object(dev_handle, id, NULL, &buffer); > >> + if (!ACPI_SUCCESS(status)) ACPI_FAILURE() > >> + return -ENODEV; > >> + > >> + obj = (union acpi_object *)buffer.pointer; Why explicit casting? > >> + if (!obj || obj->type != ACPI_TYPE_BUFFER) { > >> + dev_err(dev, "Could't read acpi buffer\n"); > >> + status = -ENODEV; Should have different int type variable for that. > >> + goto err; If there is no obj, you may return directly without freeing. > >> + } > >> + > >> + if (obj->buffer.length > size) { > >> + dev_err(dev, "Given buffer is too small\n"); > >> + status = -ENODEV; > >> + goto err; > >> + } > >> + > >> + memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length)); Does type of size and length the same? Otherwise you need min_t(). > >> + buffer_length = obj->buffer.length; > >> + kfree(buffer.pointer); > >> + > >> + return buffer_length; > >> +err: Consider naming labels by what they are about to do. Like err_free: here. > >> + kfree(buffer.pointer); > >> + return status; > >> +} > >> +static int get_acpi_ssdb_sensor_data(struct device *dev, > >> + struct sensor_bios_data *sensor) > >> +{ > >> + struct sensor_bios_data_packed sensor_data; > >> + int ret = read_acpi_block(dev, "SSDB", &sensor_data, > >> + sizeof(sensor_data)); Please, split declaration and assignment especially in the cases where it requires long lines. > >> + if (ret < 0) { > >> + dev_err(dev, "Failed to fetch SSDB data\n"); > >> + return ret; > >> + } > >> + > >> + sensor->link = sensor_data.link; > >> + sensor->lanes = sensor_data.lanes; > >> + sensor->mclkspeed = sensor_data.mclkspeed; > >> + > >> + return 0; > >> +} ... > >> + if (!dev->driver_data) { > >> + pr_info("ACPI match for %s, but it has no driver\n", > >> + supported_devices[i]); > >> + continue; > >> + } else { > >> + pr_info("Found supported device %s\n", > >> + supported_devices[i]); > >> + } Positive conditions are easier to read, but on the other hand 'else' is redundant in such conditionals (where if branch bails out from the flow). -- With Best Regards, Andy Shevchenko _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel