Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

From: Dan Scally
Date: Thu Sep 17 2020 - 10:52:41 EST

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.
>>> 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:

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:
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.