1
votes

I am attempting to write a driver for an OV2680 camera sensor. I want to turn on some GPIO pins as one of the steps in its ->probe() function. Those GpioIo() pins are declared in the DSDT tables like so (for a device upon which the OV2680 is dependent; see full DSDT table:

        Device (PMI1)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "INT3472")  // _HID: Hardware ID
            Name (_CID, "INT3472")  // _CID: Compatible ID
            Name (_DDN, "INCL-CRDD")  // _DDN: DOS Device Name
            Name (_UID, One)  // _UID: Unique ID
            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (SBUF, ResourceTemplate ()
                {
                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0079
                        }
                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x007A
                        }
                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x008F
                        }
                })
                Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */
            }
        }

        Device (CAM1)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "OVTI2680")  // _HID: Hardware ID
            Name (_CID, "OVTI2680")  // _CID: Compatible ID
            Name (_DDN, "OV2680-CRDD")  // _DDN: DOS Device Name
            Name (_UID, One)  // _UID: Unique ID
            Name (_DEP, Package (0x02)  // _DEP: Dependencies
            {
                PMI1, 
                I2C2
            })
            Name (_PLD, Package (0x01)  // _PLD: Physical Location of Device
            {
                ToPLD (
                    PLD_Revision           = 0x2,
                    PLD_IgnoreColor        = 0x1,
                    PLD_Red                = 0x0,
                    PLD_Green              = 0x0,
                    PLD_Blue               = 0x0,
                    PLD_Width              = 0x0,
                    PLD_Height             = 0x0,
                    PLD_UserVisible        = 0x1,
                    PLD_Dock               = 0x0,
                    PLD_Lid                = 0x0,
                    PLD_Panel              = "FRONT",
                    PLD_VerticalPosition   = "CENTER",
                    PLD_HorizontalPosition = "RIGHT",
                    PLD_Shape              = "VERTICALRECTANGLE",
                    PLD_GroupOrientation   = 0x0,
                    PLD_GroupToken         = 0x0,
                    PLD_GroupPosition      = 0x0,
                    PLD_Bay                = 0x0,
                    PLD_Ejectable          = 0x1,
                    PLD_EjectRequired      = 0x1,
                    PLD_CabinetNumber      = 0x0,
                    PLD_CardCageNumber     = 0x0,
                    PLD_Reference          = 0x0,
                    PLD_Rotation           = 0x0,
                    PLD_Order              = 0x0,
                    PLD_VerticalOffset     = 0xFFFF,
                    PLD_HorizontalOffset   = 0xFFFF)

            })
            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (SBUF, ResourceTemplate ()
                {
                    I2cSerialBusV2 (0x0010, ControllerInitiated, 0x00061A80,
                        AddressingMode7Bit, "\\_SB.PCI0.I2C2",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                })
                Return (SBUF) /* \_SB_.PCI0.CAM1._CRS.SBUF */
            }
        }

Note absence of a _DSD segment, meaning I have to explicitly declare them in the driver code according to the documentation. That's no problem; I have the struct acpi_device for this ACPI device (by scraping the dependents of the OV2680 device that the driver matches to), so I can do that and add them with acpi_dev_add_driver_gpios() as the documentation instructs. My question comes in the Getting GPIO Descriptor stage; the documentation says to use gpiod_get_index(), which function calls for a struct device rather than a struct acpi_device. I've tried to fulfill this by passing the struct acpi_device::dev member, but although I don't receive any error messages upon doing so, nothing actually seems to happen when I set the GPIO pins live, so I don't think that it's working.

Given this is hardware specific I'm not sure a MRE is useful, but here's a driver that should compile and be inserted successfully:

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/i2c.h>
#include <linux/acpi.h>
#include <linux/gpio/consumer.h>
#include <linux/platform_device.h>

static const struct acpi_gpio_params gpio1 = {0, 0, false};
static const struct acpi_gpio_params gpio2 = {1, 0, false};
static const struct acpi_gpio_params gpio3 = {2, 0, false};

static const struct acpi_gpio_mapping int3472_acpi_gpios[] = {
    {"gpio1", &gpio1, 1},
    {"gpio2", &gpio2, 1},
    {"gpio3", &gpio3, 1},
    {}
};

static int ov2680_probe(struct i2c_client *client)
{    
    /*
     * The driver will match the OV2680 device, but the GPIO
     * pins lie in its dependent INT3472, so we need to walk
     * up the dependencies to find that device.
    */
   struct acpi_device *int3472_device;

   /* get ACPI handle of OV2680 device */
   struct acpi_handle *dev_handle = ACPI_HANDLE(&client->dev);

   /* Get dependent devices */
   struct acpi_handle_list dep_devices;
   acpi_evaluate_reference(dev_handle, "_DEP", NULL, &dep_devices);

   int i;
   for (i=0; i < dep_devices.count; i++) {
       struct acpi_device_info *devinfo;
       acpi_get_object_info(dep_devices.handles[i], &devinfo);

       if (devinfo->valid & ACPI_VALID_HID && !strcmp(devinfo->hardware_id.string, "INT3472")) {
           acpi_bus_get_device(dep_devices.handles[i], &int3472_device);
       }
   }

   int ret;

   ret = acpi_dev_add_driver_gpios(int3472_device, int3472_acpi_gpios);

   struct gpio_desc *gpiod1, *gpiod2, *gpiod3;

   gpiod1 = gpiod_get_index(&int3472_device->dev, NULL, 0, GPIOD_ASIS);
   gpiod2 = gpiod_get_index(&int3472_device->dev, NULL, 1, GPIOD_ASIS);
   gpiod3 = gpiod_get_index(&int3472_device->dev, NULL, 2, GPIOD_ASIS);
   
   gpiod_set_value_cansleep(gpiod1, 1);
   gpiod_set_value_cansleep(gpiod2, 1);
   gpiod_set_value_cansleep(gpiod3, 1);

   return 0;
}

static int ov2680_remove(struct i2c_client *client)
{
    /*
     * Code goes here to get acpi_device, turn off all
     * the GPIO pins, remove them from the ACPI device
     * and whatnot
     */

    return 0;
}

static const struct acpi_device_id ov2680_acpi_match[] = {
    {"OVTI2680", 0},
    { }
};
MODULE_DEVICE_TABLE(acpi, ov2680_acpi_match);

static struct i2c_driver ov2680_driver = {
    .driver = {
        .name = "ov2680",
        .acpi_match_table = ov2680_acpi_match,
    },
    .probe_new = ov2680_probe,
    .remove = ov2680_remove,
};
module_i2c_driver(ov2680_driver);

MODULE_AUTHOR("Dan Scally");
MODULE_DESCRIPTION("A driver for OmniVision 2680 sensors");
MODULE_LICENSE("GPL");

dmesg reports no problems in adding the pins or anything, but the calls to gpiod_set_value_cansleep() throw an error there:

[4840.774633] gpiod_set_value_cansleep: invalid GPIO (errorpointer)

This transpires to be because the calls to gpiod_get_index() have failed, and thus the GPIO descriptors are invalid.

Questions:

  1. Is my use of &int3472->device as a parameter to gpiod_get_index() the correct approach?
  2. If so, what might cause the calls to gpiod_get_index() to fail?

EDITS:

Output of grep -H 15 /sys/bus/acpi/devices/*/status

/sys/bus/acpi/devices/ACPI000C:00/status:15
/sys/bus/acpi/devices/BOSC0200:00/status:15
/sys/bus/acpi/devices/device:16/status:15
/sys/bus/acpi/devices/device:17/status:15
/sys/bus/acpi/devices/device:32/status:15
/sys/bus/acpi/devices/INT33D3:00/status:15
/sys/bus/acpi/devices/INT33D6:00/status:15
/sys/bus/acpi/devices/INT3400:00/status:15
/sys/bus/acpi/devices/INT340E:00/status:15
/sys/bus/acpi/devices/INT344B:00/status:15
/sys/bus/acpi/devices/INT3472:08/status:15
/sys/bus/acpi/devices/INT3472:09/status:15
/sys/bus/acpi/devices/INT3F0D:00/status:15
/sys/bus/acpi/devices/MSFT0001:00/status:15
/sys/bus/acpi/devices/MSFT0101:00/status:15
/sys/bus/acpi/devices/OVTI2680:00/status:15
/sys/bus/acpi/devices/OVTI5648:00/status:15
/sys/bus/acpi/devices/PNP0103:00/status:15
/sys/bus/acpi/devices/PNP0401:01/status:15
/sys/bus/acpi/devices/PNP0A05:04/status:15
/sys/bus/acpi/devices/PNP0C09:00/status:15
/sys/bus/acpi/devices/PNP0C0C:00/status:15
/sys/bus/acpi/devices/PNP0C0D:00/status:15
/sys/bus/acpi/devices/VPC2004:00/status:15
/sys/bus/acpi/devices/WCOM508C:00/status:15
1
In this case, the calls to gpiod_set_value_cansleep() are failing because the calls to gpiod_get_index() are returning an ERR_PTR() value. So the problem lies with the calls to gpiod_get_index(), not with the calls to gpiod_set_value_cansleep().Ian Abbott
@IanAbbot yeeeeeeaaaaas looks like the gpio_descs that returns aren't valid. Turns out I didn't have the GPIO Drivers loaded which might be why, so just rebuilding the kernel with those to see what, if any, different that makes.Dan Scally

1 Answers

2
votes

(Gathering the answer based on comments I have given earlier)

For sake of the clarification I have to say, that from your DSDT we can get the following information. There are 3 groups of PMICs, i.e. DSCx, CLPx and PMIx. I believe they are based on the model, like Desktop, Laptop, 2-in-1. And in each case all PMICs in the same group have different _UID. From the provided output of the grep -H 15 ... we have only 2 out of 10 enumerated with the instances INT3472:08 and INT3472:09 (exactly two last defined in DSDT). And they are PMIx, you may check this by grep -H . /sys/bus/acpi/devices/INT3472:*/path.

Your interest is the PMI1 which consumes three GPIO lines from Intel GPIO driver, i.e. pins 121, 122 and 143 (you may decode them as Community #2, Group #5 or GPP_F, relative to the group pins 1, 2 and 23, this may help you to understand _INI method that touches these lines via other methods in DSDT), and provides 3 + 7 = 10 pins according to its driver.

Now to the code. The _DEP ACPI method is used solely for linking power resources, and Linux kernel has other means how to hijack resources from other device, because what you are trying to do is to use the resource which is not related to the device you are creating driver for.

The method is to find device by ACPI HID:

struct acpi_device *adev;
struct device *phys_dev;
struct gpio_desc *desc;

...

adev = acpi_dev_get_first_match_dev("INT3472", "1", -1);
if (!adev) {
  pr_err("Oops, we didn't find an ACPI device!\n");
  return -ENODEV;
}

phys_dev = get_device(acpi_get_first_physical_node(adev));
acpi_dev_put(adev);

if (!phys_dev) {
  pr_err("Oops, we didn't find a physical device!\n");
  return -ENODEV;
}

desc = gpiod_get_index(phys_dev, NULL, 0, GPIOD_ASIS);
if (IS_ERR(desc)) {
  pr_err("Something went wrong when retrieving GPIO\n");
  put_device(phys_dev);
  return PTR_ERR(desc);
}

...

gpiod_put(desc);
put_device(phys_dev);

Hackish way to simplify this (since you know bus type and exact name of the device instance, but Linux does not guarantee it is kept the same over boots) is:

struct device *phys_dev;
struct gpio_desc *desc;

...

phys_dev = bus_find_device_by_name(&i2c_bus_type, NULL, "i2c-INT3472:09");
if (!phys_dev) {
  pr_err("Oops, we didn't find a physical device!\n");
  return -ENODEV;
}

desc = gpiod_get_index(phys_dev, NULL, 0, GPIOD_ASIS);
if (IS_ERR(desc)) {
  pr_err("Something went wrong when retrieving GPIO\n");
  put_device(phys_dev);
  return PTR_ERR(desc);
}

...

gpiod_put(desc);
put_device(phys_dev);

Side notes:

  1. There is already existing driver for OV2680 camera sensor, it makes sense to extend it instead of creating a specific fork for ACPI case.
  2. The proper method is to use resources where they can be consumed without any hacks, i.e. in the PMIC MFD driver.