Hi Andy, thanks for input (as always) On 17/09/2020 13:45, Andy Shevchenko wrote: > 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. TWIMC? >>> 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? Yes, using the libcamera project's qcam program. They're poor quality at the moment, because there's no auto-white-balance / exposure controls in the ipu3 pipeline yet, but we can take images. Example: https://user-images.githubusercontent.com/4592235/91197920-d1d41500-e6f3-11ea-8207-1c27cf24dd45.png A bunch of folks have managed it so far on a couple different platforms (Surface Book 1, Surface Pro something, an Acer A12 and a Lenovo Miix-510) >>>> 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. > Ah, ok. So that was never going to work. Thanks. That does raise another question; we have had some testers report failure, which turns out to be because on their platforms the definition of their cameras in ACPI is never translated into an i2c_client so the cio2-bridge doesn't bind. Those have a similar conditional in the _STA method, see CAM1 in this DSDT for example: https://raw.githubusercontent.com/linux-surface/acpidumps/master/surface_go/dsdt.dsl. Is there anything we can do to enable those cameras to be discovered too? >>>> +static int cio2_probe_can_progress(struct pci_dev *pci_dev) >>>> +{ >>>> + void *sensor; > Why void? > 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. > Yeah, I misunderstood how this worked - after greg pointed out I was doing it wrong I read the code a bit better and got it working assigning to a struct device *sensor; - TIL. >>>> + if (!IS_ENABLED(CONFIG_ACPI)) { >>>> + r = cio2_parse_firmware(cio2); >>>> + if (r) >>>> + goto fail_clean_notifier; >>>> + } > How comes? Me misunderstanding again; it will be removed. > >>>> \ No newline at end of file > ??? > > Be sure you are using good editor. > Yeah haven't managed to track down what's causing this yet. Visual Studio Code maybe. >>>> +#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?! > It felt ugly to have the other definitions be macros and not this one, but I can change it. >>>> + return -ENODEV; >>>> + >>>> + obj = (union acpi_object *)buffer.pointer; > Why explicit casting? > > >>>> + 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). Yeah good point - and much more readable that way. Thanks; I'll stick to that in future. All your other suggestions are great - thank you, I will fix them for the v2.
