Hi, On 10/23/23 08:23, Andreas Helbech Kleist wrote: > Hi, > > On Fri, 2023-10-20 at 16:39 +0200, Hans de Goede wrote: >> Hi, >> >> On 10/20/23 12:47, Andreas Helbech Kleist wrote: >>> I've only tried it on my WIP IPU4 hack of this driver >> >> Oh a IPU4 version of this driver would be very interesting, >> I was actually thinking about looking into this myself >> because I have a Chuwi Hi13 tablet which uses the IPU4 and >> it would be nice to get the camera working there (and on >> other IPU4 using devices). >> >> Can you give some more info on your IPU4 work, e.g. : >> >> 1. Does it work? > > Not yet. The intel-ipu4 module is working to the extent I'm able to > test it without the isys-module. Buttress authentication succeeds, and > I have very similar mmiotraces between the new driver and the old 4.19 > based driver I'm comparing it to. > > The isys-module is very much work in progress. I'm learning the v4l2 > along the way, so there is a steep learning curve. > >> 2. Which sensor are you using it with ? > > A custom tc358748 driver that pretends to be a sensor that delivers RGB > data. I tried with the tc358746 driver in-tree, but figured it would be > simpler to start out with a custom driver, since we have an old driver > that has a list of the exact register writes needed to setup the > device. > >> 3. Which device are you testing on ? > > An endoscopic system from Ambu A/S (the company I work for). See my > colleague Claus' description of the device here: > https://lore.kernel.org/all/471df7ffdf34b73d186c429a366cfee62963015f.camel@xxxxxxxxx/ > >> 4. Do you have a public git branch with this somewhere ? > > Not yet, but I will share when I have something working. I could push > the main driver part if anyone is interested. The isys driver is not > worth sharing yet. Sharing the code once you have something working is fine. >> I'm afraid that I won't have time to look into this >> myself anytime soon, but I'm very interested in this. > > It would be great to have somebody else testing this out when I get a > bit further. Ok, I cannot promise I'll be able to find the time but I'm definitely interested in this. If you can send me a link to a git repo with the patches once you have something working, then I will try to make the time to test this. >>> but I think it >>> is correct for IPU6 as well. The reason is that isys_driver is an >>> auxiliary_driver, so I don't think >>> >>> MODULE_DEVICE_TABLE(pci, isys_pci_tbl); >>> >>> has any effect. The PCI probe happens in ipu6_pci_probe in ipu6.c >>> (because it has a pci_device_id table as well), and the isys_driver >>> is >>> probed indirectly by ipu6-bus.c. >> >> So the MODULE_DEVICE_TABLE(pci, isys_pci_tbl) indeed does not >> belong in this auxbus driver, instead it should use some sort >> of auxbus MODULE_DEVICE_TABLE() to autoload based on its >> auxbus modalias. >> >> But it does have an effect, modprobe will load both the main >> ipu6 driver registering the aux devices as well as this driver >> based on the modalias of the PCI device because with this >> MODULE_DEVICE_TABLE(pci, isys_pci_tbl); statement both drivers >> match that PCI modalias. > > All right. But since the main driver contains the same table, I don't > think there's any need to have it here? > >> But the correct thing to do here would be to switch to >> an auxbus based MODULE_DEVICE_TABLE() for the isys driver. > > The isys_driver already has an auxiliary_device_id table. I'm not sure > if that's what you mean? > > From the bottom of ipu6-isys.c in PATCH 10/15: > > +static const struct auxiliary_device_id ipu6_isys_id_table[] = { > + { > + .name = "intel_ipu6.isys", > + .driver_data = (kernel_ulong_t)&ipu6_isys_auxdrv_data, > + }, > +}; Right, so this needs a: MODULE_DEVICE_TABLE(auxiliary, ipu6_isys_id_table); And then the: MODULE_DEVICE_TABLE(pci, isys_pci_tbl) and any other mention of isys_pci_tbl can be dropped. Regards, Hans
