Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI

[Dan Scally <djrscally@gmail.com>]

On 01/07/2020 02:16, Jordan Hand wrote:
> On 5/26/20 7:31 AM, Heikki Krogerus wrote:
>> On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote:
>>> Em Thu, 21 May 2020 11:00:19 +0300
>>> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
>>>
>>>> +Cc: Heikki (swnode expert)
>>>>
>>>> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
>>>> <mchehab+huawei@kernel.org> wrote:
>>>>> Em Wed, 20 May 2020 11:26:08 +0300
>>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>>
>>>> ...
>>>>
>>>>> As I said, the problem is not probing the sensor via ACPI, but,
>>>>> instead,
>>>>> to be able receive platform-specific data.
>>>>
>>>> There is no problem with swnodes, except missing parts (*).
>>>> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
>>>> since we have drivers in place with fwnode support, we only need to
>>>> recreate fwnode graph in some board file to compensate the gap in
>>>> ACPI.
>>>>
>>>> *) Missing part is graph support for swnodes. With that done it will
>>>> be feasible to achieve the rest.
>>>> I forgot if we have anything for this already done. Heikki?
>>>
>>> Hmm... I guess I should try this approach. I never heard about swnodes
>>> before. Do you have already some patch with the needed swnodes setup,
>>> and the missing parts to recreate the fwnode graph?
>>
>> Here you go.
>>
>
> For anyone interested, I have taken Heikki's patch and attempted to
> use swnodes to patch the incomplete dsdt on my laptop to use with
> ipu3; the code is currently in a github repo[1].
>
> In particular, patches 1, 2, and 3 setup the software_node
> infrastructure. Patch 5 shows how we might use software nodes where
> ACPI fails.
>
> My sensor driver (in patch 4) doesn't actually work right now which is
> why I haven't brought any of this to the mailing list yet, but that's
> another story :)
>
> I would just submit a patchset, but since my sensor driver doesn't
> work, I can't gully test the rest of it. But if someone has a system
> where the drivers in question are upstream and work, something like
> this could be a good path forward.
>
> - Jordan
>
> [1] https://github.com/jhand2/surface-camera/tree/master/patches
>
>
Hello all

I joined in these efforts [2] to get cameras working on
Microsoft Surface and similar platforms, currently I'm working on
expanding Jordan's module connecting cameras to the cio2
infrastructure (which works - we can use it to take images), aiming to
discover connection information from SSDB in the DSDT, as well as
connect as many supported sensors as are found on the device. I'm just
struggling with a problem I've encountered using the swnode patch that
Heikki linked in this thread; the module's working ok when I only
attempt to connect a single one of my sensors (by preventing the
driver for the other sensor from loading, in which case this new
module ignores the sensor), but when I attempt to connect both cameras
at the same time I get a kernel oops as part of
software_node_get_next_child. The module is creating software_nodes
like this...

/sys/kernel/software_node/INT343E/port0/endpoint0
/sys/kernel/software_node/INT343E/port1/endpoint0
/sys/kernel/software_node/OVTI2680/port0/endpoint0
/sys/kernel/software_node/OVTI5648/port0/endpoint0

And that's the structure that I expect to see, but it seems like the
call to list_next_entry in that function is returning something that
isn't quite a valid swnode. Printing the address of c->fwnode after
that point returns "3", which isn't a valid address of course, and
that's causing the oops when it's either returned (in the version of
the function without the patches) or when the program flow tries to
call the "get" op against that fwnode (in the patched version). I've
been trying to track it down for a while now without success, so I
posted the problem on SO[3] and it was suggested that I mail these
addressees for advice - hope that that is ok.


My copy of Jordan's module is parked in my git repo [4] for now, and
requires the batch of patches from Jordan's repo [5] -
I've been applying those against 5.8.0-rc7. Any other criticism more
than welcome - I'm new to both c and kernel programming so I'm happy
to take all the advice people have the time to give.


On a more general note; Kieran from the libcamera project suggested we
ought to be talking to you guys anyway to get some guidance on design,
and some more expert eye on the things we don't really understand. In
particular; we haven't been able to figure out how sensors that are
intended to work with the cio2 ipu3 stuff have their clock/regulators
supplied - in fact I can strip all the "usual" clock/regulator
functionality out of my camera's driver and it still functions fine,
which seems a bit weird. So far all we're doing as "power management"
of the camera's is turning on the GPIO pins that DSDT tables assign to
the tps68470 PMICs the cameras are theoretically hooked up to...but
given the drivers continue to work without using the PMICs regulator
and clk drivers (which we found in the intel-lts tree on Github),
we're a bit confused exactly how these are connected. Given the
potential for actual hardware damage if we mess something up there
it'd be great if anyone can shed some light on those elements.


Regards

Dan


[2] https://github.com/linux-surface/linux-surface/issues/91

[3] https://stackoverflow.com/questions/63742967/what-is-causing-this-kernel-oops-when-parsing-firmware?

[4] https://github.com/djrscally/miix-510-cameras/blob/master/surface_camera/surface_camera.c

[5] https://github.com/jhand2/surface-camera/tree/master/patches



_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel