sensor_drivers: better way to tell if system is ACPI-based

[kitakar5525]

Comment from me (kitakar5525)

Hmm, I use is_acpi_node() [1] to tell if the system is ACPI-based or not, in ov7251 and ov8865 (because original driver has DT support, I tried to preserve it). When using with surface_camera driver, is_acpi_node() returns false, resulting in breaking functionality (e.g. here causes null ptr deref).

So, I should either remove support for DT or find another way to tell if system is ACPI-based.

[1] I referred to this commit: torvalds/linux@0c2c7a1 ("media: ov8856: Add devicetree support")

Comment from djrscally

Yeeeeaaaahh it's because we're overwriting the fwnode_handle of the device with that of the software_node we created. Unfortunately the root of that is_acpi_node() function is a check to see if the fwnode_handle's ops are acpi_device_fwnode_ops, and in our case we need them to be software_node_ops or the linking won't work.

I think it's best to be as adaptable as possible, so if you can preserve the DT compatibility that would be preferable imo. You could just use acpi_dev_present() to see if you can match an ACPI device with your sensor's HID and use that as the check?

#define SENSOR_HID "OVTI2680"
bool adev

adev = acpi_dev_present(SENSOR_HID, NULL, -1);

if (adev)
    gpio_crs_ctrl(&sensor->sd, true);

    /* Add some delay. This is required or check_chip_id() will fail. */
    usleep_range(10000, 12000);
}

[kitakar5525]

@djrscally Thanks for the advice!

Because I wanted to avoid defining the new string (SENSOR_HID), I used acpi_match_table instead. (https://github.com/kitakar5525/surface-ipu3-cameras/commit/8cbc76f19f512feb83a3295c20ca73c35de45f5e)

(in probe()):

    if (acpi_match_device(dev->driver->acpi_match_table, dev)) {
        dev_info(dev, "system is acpi-based\n");
        ov7251->is_acpi_based = true;
    } else
        dev_info(dev, "system is not acpi-based\n");

Then, I noticed that, when cio2-bridge (formerly surface_camera) driver is loaded, it somehow also breaks the acpi_match_device() (returns false). So, I added a new member named is_acpi_based and run acpi_match_device() only once in probe().

Do you have any idea why?

By the way, I also want to remove the current cio2-bridge and sensor drivers load order restriction at some point. I guess breaking acpi_match_device() may also mean breaking sensor driver loading when cio2-bridge is loaded before.

[djrscally]

Then, I noticed that, when cio2-bridge (formerly surface_camera) driver is loaded, it somehow also breaks the acpi_match_device() (returns false). So, I added a new member named is_acpi_based and run acpi_match_device() only once in probe().

Do you have any idea why?

Huh, no - that seems pretty weird. Let me look into it for you; might not be till later on though.

By the way, I also want to remove the current cio2-bridge and sensor drivers load order restriction at some point. I guess breaking acpi_match_device() may also mean breaking sensor driver loading when cio2-bridge is loaded before.

The mailing list came back with a lot of changes, part of which was basically to make the code just be exported functions (I.E. have cio2_bridge_probe() just be available to the rest of the kernel with EXPORT_SYMBOL_GPL()), and then call those functions during the cio2_pci_probe() function in ipu3-cio2 driver. So we could check for existing endpoints against CIO2 (for instance on Chromebooks where they're defined properly) and if they don't exist, call the cio2-bridge functions to create them as software nodes. As part of that, the cio2-bridge will have to reprobe the sensors - so there'd be no cio2-bridge module to load any more, and I'm pretty sure you'd be able to just insert/remove the sensor driver modules independently.

[kitakar5525]

Huh, no - that seems pretty weird. Let me look into it for you; might not be till later on though.

Thanks!

As part of that, the cio2-bridge will have to reprobe the sensors - so there'd be no cio2-bridge module to load any more,

OK, I'm glad that the discussion is already taken place.

and I'm pretty sure you'd be able to just insert/remove the sensor driver modules independently.

Does this mean that sensor drivers will be able to read properties from swnode? For example, the upstream ov5670 driver has this clock-frequency value check in probe() (https://github.com/torvalds/linux/blob/98477740630f270aecf648f1d6a9dbc6027d4ff1/drivers/media/i2c/ov5670.c#L2461-L2463)

    device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
    if (input_clk != 19200000)
        return -EINVAL;

[djrscally]

Huh, no - that seems pretty weird. Let me look into it for you; might not be till later on though.

Thanks!

As part of that, the cio2-bridge will have to reprobe the sensors - so there'd be no cio2-bridge module to load any more,

OK, I'm glad that the discussion is already taken place.

and I'm pretty sure you'd be able to just insert/remove the sensor driver modules independently.

Does this mean that sensor drivers will be able to read properties from swnode? For example, the upstream ov5670 driver has this clock-frequency value check in probe() (https://github.com/torvalds/linux/blob/98477740630f270aecf648f1d6a9dbc6027d4ff1/drivers/media/i2c/ov5670.c#L2461-L2463)

  device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
  if (input_clk != 19200000)
      return -EINVAL;

Yes, I think they should be able to, as the call to .probe() that's triggered by the cio2-bridge functions will come after the software_nodes have been registered and assigned to the device as dev->fwnode. Hopefully I'll have that set of changes finished off tonight.

[kitakar5525]

Yes, I think they should be able to, as the call to .probe() that's triggered by the cio2-bridge functions will come after the software_nodes have been registered and assigned to the device as dev->fwnode. Hopefully I'll have that set of changes finished off tonight.

Got it, thanks!

[djrscally]

OK, I got to the bottom of cio2-bridge breaking acpi_match_device() and unfortunately it's a pretty hard problem; the same thing has nuked my plan to force the sensors to reprobe(). Basically, acpi_match_device() includes a call to ACPI_COMPANION(dev), and part of the internals of that macro rely on dev->fwnode->ops being acpi_device_fwnode_ops...and they're not, because we just made them software_node_ops in order for the graph parsing to work.

That means that any properties read during the sensor driver's .probe() would be available. Any read after that point would not, unless I check for properties against the existing fwnode and replicate them in the software node if there are any. I'll investigate that.

[djrscally]

OK; I think I can solve both issues by:

1. deferring cio2-bridge until the sensors have probed.
2. Doing everything we're already doing in cio2-bridge and then...
3. extremely hackishly adding an i2c_device_id to the sensor's driver as part of cio2-bridge's code, so that when the sensor is re-probed, it works.

As I say; seems very hackish, so I'll probably ask on linux-media first.

[kitakar5525]

Basically, acpi_match_device() includes a call to ACPI_COMPANION(dev), and part of the internals of that macro rely on dev->fwnode->ops being acpi_device_fwnode_ops...and they're not, because we just made them software_node_ops in order for the graph parsing to work.

I see. Thanks for the investigation as always!