On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> On x86/ACPI platforms the GPIO resources do not provide information
> about which GPIO resource maps to which connection-id. So e.g.
> gpiod_get(devg, "reset") does not work.
>
> On devices with an Intel IPU3 or newer ISP there is a special ACPI
> INT3472 device describing the GPIOs and instantiating of the i2c_client
> for a sensor is deferred until the INT3472 driver has been bound based
> on the sensor ACPI device having a _DEP on the INT3472 ACPI device.
>
> This allows the INT3472 driver to add the necessary GPIO lookups
> without needing any special ACPI handling in the sensor driver.
>
> Unfortunately this does not work on devices with an atomisp2 ISP,
> there the _DSM describing the GPIOs is part of the sensor ACPI device
> itself, rather then being part of a separate ACPI device.
than
(not the first time I see the same typo in your commit messages :-)
> IOW there is no separate firmware-node to which we can bind to register
> the GPIO lookups (and also no way to defer creating the sensor i2c_client).
>
> This unfortunately means that all sensor drivers which may be used on
> BYT or CHT hw need some code to deal with ACPI integration.
>
> This patch adds a new v4l2_acpi_parse_sensor_gpios() helper function
> for this, which does all the necessary work. This minimizes the
> (unavoidable) change to sensor drivers for ACPI integration to just
> adding a single line calling this void function to probe().
>
> There also is a no-op stub provided for non ACPI platforms so that
> no #ifdef-s are necessary in the driver.
>
> Note v4l2_acpi_parse_sensor_gpios() is basically a copy of
> the atomisp2 v4l2_get_acpi_sensor_info() helper from:
> drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> minus logging the sensor module-name using a second _DSM.
> v4l2_get_acpi_sensor_info() was already reviewed by Andy,
> see the Link tag below.
>
> (this code duplication is removed in the next patch in this series)
I believe the above is not needed to be in the commit message, but
rather in the comments below (after the '---' cutter line).
...
> +/* Note this always returns 1 to continue looping so that res_count is accurate */
> +static int v4l2_acpi_handle_gpio_res(struct acpi_resource *ares, void *_data)
> +{
> + struct v4l2_acpi_gpio_parsing_data *data = _data;
> + struct acpi_resource_gpio *agpio;
> + const char *name;
> + bool active_low;
> + unsigned int i;
> + u32 settings;
> + u8 pin;
Should be u16.
> + pin = agpio->pin_table[0];
> + for (i = 0; i < data->settings_count; i++) {
> + if (INTEL_DSM_PIN(data->settings[i]) == pin) {
> + settings = data->settings[i];
> + break;
> + }
> + }
> +
> + if (i == data->settings_count) {
> + dev_warn(data->dev, "Could not find DSM GPIO settings for pin %d\n", pin);
%u
> + return 1;
> + }
> +
> + switch (INTEL_DSM_TYPE(settings)) {
> + case 0:
> + name = "reset-gpios";
> + break;
> + case 1:
> + name = "powerdown-gpios";
> + break;
> + default:
> + dev_warn(data->dev, "Unknown GPIO type 0x%02lx for pin %d\n",
%u
> + INTEL_DSM_TYPE(settings), pin);
> + return 1;
> + }
> +
> + /*
> + * Both reset and power-down need to be logical false when the sensor
> + * is on (sensor should not be in reset and not be powered-down). So
> + * when the sensor-on-value (which is the physical pin value) is high,
> + * then the signal is active-low.
> + */
> + active_low = INTEL_DSM_SENSOR_ON_VAL(settings) ? true : false;
Ternary part is redundant.
> + i = data->map_count;
> + if (i == V4L2_SENSOR_MAX_ACPI_GPIOS)
> + return 1;
> +
> + /* res_count is already incremented */
> + data->map->params[i].crs_entry_index = data->res_count - 1;
> + data->map->params[i].active_low = active_low;
> + data->map->mapping[i].name = name;
> + data->map->mapping[i].data = &data->map->params[i];
> + data->map->mapping[i].size = 1;
> + data->map_count++;
> +
> + dev_info(data->dev, "%s crs %d %s pin %d active-%s\n", name,
%u (for pin)
> + data->res_count - 1, agpio->resource_source.string_ptr,
> + pin, active_low ? "low" : "high");
> +
> + return 1;
> +}
...
> + * Note this code uses the same DSM GUID as the INT3472 discrete.c code
> + * and there is some overlap, but there are enough differences that it is
> + * difficult to share the code.
Can you add the name of the variable in that file, so likely the
source code indexing tool might add a link?
...
> + struct acpi_device *adev = ACPI_COMPANION(dev);
I would split this assignment and...
> + struct v4l2_acpi_gpio_parsing_data data = { };
> + LIST_HEAD(resource_list);
> + union acpi_object *obj;
> + unsigned int i, j;
> + int ret;
> +
...place it here.
> + if (!adev || (!soc_intel_is_byt() && !soc_intel_is_cht()))
> + return;
...
> + devm_acpi_dev_add_driver_gpios(dev, data.map->mapping);
Won't we print a warning here as well in case of error?
...
> +#ifdef CONFIG_ACPI
> +struct device;
This should be outside of previous ifdeffery as it's used in a stub.
> +/**
> + * v4l2_acpi_parse_sensor_gpios - Parse ACPI info describing sensor GPIOs.
> + *
Dunno the style of v4l2, but this line is redundant.
> + * @dev: Device to parse the ACPI info for
> + *
> + * On x86/ACPI platforms the GPIO resources do not provide information
> + * about which resource maps to which connection-id.
> + *
> + * Sensor drivers can call this function to use platform specific methods
> + * (e.g. the Intel 79234640-9e10-4fea-a5c1-b5aa8b19756f _DSM) to get
> + * information about the pins and add GPIO mappings to make standard gpiod_get()
> + * calls work.
> + *
> + * The registered mappings use devm managed memory and are automatically free-ed
> + * on remove() and on probe() failure.
> + */
Usually the kernel doc is attached to the function implementation.
> +void v4l2_acpi_parse_sensor_gpios(struct device *dev);
> +
> +#else /* ifdef CONFIG_ACPI */
> +static inline void v4l2_acpi_parse_sensor_gpios(struct device *dev) { return 0; }
> +#endif /* ifdef CONFIG_ACPI */
--
With Best Regards,
Andy Shevchenko
