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

[Dan Scally <djrscally@gmail.com>]

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.