mbox series

[00/28] media: atomisp: Further sensor rework + exotic features removal

Message ID 20230401145926.596216-1-hdegoede@redhat.com
Headers show
Series media: atomisp: Further sensor rework + exotic features removal | expand

Message

Hans de Goede April 1, 2023, 2:58 p.m. UTC
Hi All,

Here is another set of atomisp patches from yours truely.

This basically consists of 2 parts:

1.  Further sensor driver modernization to work towards using standard
    v4l2 sensor drivers instead of atomisp specific copies
1a. For some sensors some GPIOs have different polarity depending on
    which board they are on and the order in which GPIOs are listed in
    in the ACPI resources also is not consistent between boards.
    Fixing this without relying on per board DMI quirks requires parsing
    the results of a special Intel ACPI _DSM. Add a new
    v4l2_get_acpi_sensor_info() helper for this
1b. Convert the gc0310 driver to use ACPI runtime pm instead of relying
    on the direct PMIC poking from atomisp_gmin_platform

2.  Further work on removing various exotic features, specifically
    prep work + remove support for streaming from 2 sensors at once,
    as discussed here:
    https://lore.kernel.org/linux-media/5309d845-063b-6dd9-529d-0f82654290f2@redhat.com/

Regards,

Hans


Hans de Goede (28):
  media: atomisp: Add v4l2_get_acpi_sensor_info() helper
  media: atomisp: ov2680: Use v4l2_get_acpi_sensor_info() for the GPIO
    lookups
  media: atomisp: ov2680: Error handling fixes
  media: atomisp: gc0310: Remove some unused structure definitions
  media: atomisp: gc0310: Remove GC0310_TOK_*
  media: atomisp: gc0310: Simplify gc0310_write_reg_array()
  media: atomisp: gc0310: Remove enum gc0310_tok_type
  media: atomisp: gc0310: Replace custom reg access functions with smbus
    helpers
  media: atomisp: gc0310: Remove non working flip-controls
  media: atomisp: gc0310: Remove read-only exposure control
  media: atomisp: gc0310: Drop custom ATOMISP_IOC_S_EXPOSURE support
  media: atomisp: gc0310: Add exposure and gain controls
  media: atomisp: gc0310: Add error_unlock label to s_stream()
  media: atomisp: gc0310: Modernize and simply set_fmt(), get_fmt(),
    etc.
  media: atomisp: gc0310: Delay power-on till streaming is started
  media: atomisp: gc0310: Add runtime-pm support
  media: atomisp: gc0310: Use devm_kzalloc() for data struct
  media: atomisp: gc0310: Switch over to ACPI powermanagement
  media: atomisp: Remove duplicate atomisp_[start|stop]_streaming
    prototypes
  media: atomisp: Remove continuous mode related code from
    atomisp_set_fmt()
  media: atomisp: Remove custom V4L2_CID_FMT_AUTO control
  media: atomisp: Remove snr_mbus_fmt local var from atomisp_try_fmt()
  media: atomisp: Remove unused ATOM_ISP_MAX_WIDTH_TMP and
    ATOM_ISP_MAX_HEIGHT_TMP
  media: atomisp: Remove atomisp_try_fmt() call from atomisp_set_fmt()
  media: atomisp: Drop support for streaming from 2 sensors at once
  media: atomisp: Remove struct atomisp_sub_device index field
  media: atomisp: gmin_platform: Make DMI quirks take precedence over
    the _DSM table
  media: atomisp: gmin_platform: Add Lenovo Ideapad Miix 310 gmin_vars

 .../media/atomisp/i2c/atomisp-gc0310.c        | 999 ++++--------------
 .../media/atomisp/i2c/atomisp-ov2680.c        |  33 +-
 drivers/staging/media/atomisp/i2c/gc0310.h    | 416 +++-----
 drivers/staging/media/atomisp/i2c/ov2680.h    |   1 -
 .../media/atomisp/include/linux/atomisp.h     |   2 -
 .../atomisp/include/linux/atomisp_platform.h  |   2 +
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 356 ++-----
 .../media/atomisp/pci/atomisp_compat.h        |   4 +-
 .../media/atomisp/pci/atomisp_compat_css20.c  |  89 +-
 .../staging/media/atomisp/pci/atomisp_fops.c  |   9 +-
 .../media/atomisp/pci/atomisp_gmin_platform.c | 289 ++++-
 .../media/atomisp/pci/atomisp_internal.h      |  20 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  64 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.h |   3 -
 .../media/atomisp/pci/atomisp_subdev.c        | 111 +-
 .../media/atomisp/pci/atomisp_subdev.h        |   6 -
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |  36 +-
 17 files changed, 844 insertions(+), 1596 deletions(-)

Comments

Andy Shevchenko April 2, 2023, 6:17 a.m. UTC | #1
On Sat, Apr 1, 2023 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> ACPI nodes describing sensors on atomisp devices implement a
> "79234640-9e10-4fea-a5c1-b5aa8b19756f" Device Specific Method (DSM)
> to get info about the GPIOs.
>
> Using this method is necessary to figure out which ACPI GPIO resource
> is "reset" and which one is "powerdown" and this is also necessary to
> figure out the correct polarity of the pins.
>
> One example where this is necessary is the GC0310 sensor. The current
> GC0310 code hardcodes reset as being active-low and power-down as being
> active-high. This works on a number of devices such as the mpman
> converter 9. But it is wrong for the Chuwi Vi8 CWI501 where the powerdown
> pin is active-low.
>
> Rather then adding DMI quirks for this, add a helper for this,
> which can be shared between sensor-drivers. This new helper optionally
> also returns a string identifying the exact sensor-module used, which
> might be useful if any module specific behvior is necessary in the sensor
> driver.
>
> This uses the DSM directly on the sensor device's ACPI node. This is
> different from later Intel hardware (IPU3 / IPU6) which has a separate
> INT3472 node (with its own driver) with this info.
>
> Since there is no separate ACPI node to which we can bind to register GPIO
> lookups, this unfortunately means that all sensor drivers which may be used
> on BYT or CHT hw need to call this new helper.
>
> Note for now this function is being added to atomisp_gmin_platform.c,
> but once things are ready to move atomisp over to using generic sensor
> drivers this will need to become a generic v4l2 sensor helper. but this
> will require upstream discussion first.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../atomisp/include/linux/atomisp_platform.h  |   2 +
>  .../media/atomisp/pci/atomisp_gmin_platform.c | 240 ++++++++++++++++++
>  2 files changed, 242 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
> index b77bf814a5a6..e8e965f73fc8 100644
> --- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
> +++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
> @@ -213,6 +213,8 @@ int atomisp_register_sensor_no_gmin(struct v4l2_subdev *subdev, u32 lanes,
>                                     enum atomisp_bayer_order bayer_order);
>  void atomisp_unregister_subdev(struct v4l2_subdev *subdev);
>
> +int v4l2_get_acpi_sensor_info(struct device *dev, char **module_id_str);
> +
>  /* API from old platform_camera.h, new CPUID implementation */
>  #define __IS_SOC(x) (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && \
>                      boot_cpu_data.x86 == 6 &&                       \
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 7fc7dfa56172..f8d8a34d7e7f 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -1447,3 +1447,243 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0f38, isp_pm_cap_fixup);
>
>  MODULE_DESCRIPTION("Ancillary routines for binding ACPI devices");
>  MODULE_LICENSE("GPL");
> +
> +/*
> + * The below helper functions don't really belong here and should eventually be
> + * moved to some place under sys/drivers/media/v4l2-core .

Redundant sys/ and space at the end.

> + */
> +#include <linux/platform_data/x86/soc.h>
> +
> +/*
> + * 79234640-9e10-4fea-a5c1-b5aa8b19756f
> + * This _DSM GUID returns information about the GPIO lines mapped to a sensor.
> + * Function number 1 returns a count of the GPIO lines that are mapped.
> + * Subsequent functions return 32 bit ints encoding information about the GPIO.
> + */
> +static const guid_t intel_sensor_gpio_info_guid =
> +       GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> +                 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
> +
> +/*
> + * 822ace8f-2814-4174-a56b-5f029fe079ee
> + * This _DSM GUID returns a string from the sensor device, which acts as a
> + * module identifier.
> + */
> +static const guid_t intel_sensor_module_guid =
> +       GUID_INIT(0x822ace8f, 0x2814, 0x4174,
> +                 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
> +
> +#define INTEL_DSM_TYPE_SHIFT                           0
> +#define INTEL_DSM_TYPE_MASK                            GENMASK(7, 0)
> +#define INTEL_DSM_PIN_SHIFT                            8
> +#define INTEL_DSM_PIN_MASK                             GENMASK(15, 8)
> +#define INTEL_DSM_SENSOR_ON_VAL_SHIFT                  24
> +#define INTEL_DSM_SENSOR_ON_VAL_MASK                   GENMASK(31, 24)
> +
> +#define INTEL_DSM_TYPE(x) \
> +       (((x) & INTEL_DSM_TYPE_MASK) >> INTEL_DSM_TYPE_SHIFT)
> +#define INTEL_DSM_PIN(x) \
> +       (((x) & INTEL_DSM_PIN_MASK) >> INTEL_DSM_PIN_SHIFT)
> +#define INTEL_DSM_SENSOR_ON_VAL(x) \
> +       (((x) & INTEL_DSM_SENSOR_ON_VAL_MASK) >> INTEL_DSM_SENSOR_ON_VAL_SHIFT)
> +
> +#define V4L2_SENSOR_MAX_ACPI_GPIOS                     2u
> +
> +struct v4l2_acpi_gpio_map {
> +       struct acpi_gpio_params params[V4L2_SENSOR_MAX_ACPI_GPIOS];
> +       struct acpi_gpio_mapping mapping[V4L2_SENSOR_MAX_ACPI_GPIOS + 1];
> +};
> +
> +struct v4l2_acpi_gpio_parsing_data {
> +       struct device *dev;
> +       u32 settings[V4L2_SENSOR_MAX_ACPI_GPIOS];
> +       unsigned int settings_count;
> +       unsigned int res_count;
> +       unsigned int map_count;
> +       struct v4l2_acpi_gpio_map *map;
> +};
> +
> +/* 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;
> +
> +       if (!acpi_gpio_get_io_resource(ares, &agpio))
> +               return 1; /* Not a GPIO, continue the loop */
> +
> +       data->res_count++;
> +
> +       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);
> +               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",
> +                        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

), so

> +        * when the sensor-on-value, which is the physical pin value is high,

The sub sentence between commas is a bit unreadable.

> +        * then the signal is active-low.
> +        */
> +       active_low = INTEL_DSM_SENSOR_ON_VAL(settings) ? true : false;

Redundant '? true : false" part.

> +       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,
> +                data->res_count - 1, agpio->resource_source.string_ptr,
> +                pin, active_low ? "low" : "high");
> +
> +       return 1;
> +}
> +
> +/*
> + * Helper function to create an ACPI GPIO lookup table for sensor reset and
> + * powerdown signals on Intel Bay Trail (BYT) and Cherry Trail (CHT) devices,
> + * including setting the correct polarity for the GPIO.
> + *
> + * This uses the "79234640-9e10-4fea-a5c1-b5aa8b19756f" DSM method directly
> + * on the sensor device's ACPI node. This is different from later Intel
> + * hardware which has a separate INT3472 with this info. Since there is
> + * no separate firmware-node to which we can bind to register the GPIO lookups
> + * this unfortunately means that all sensor drivers which may be used on
> + * BYT or CHT hw need to call this function. This also means that this function
> + * may only fail when it is actually called on BYT/CHT hw. In all other cases
> + * it must always succeed.
> + *
> + * 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.
> + */
> +int v4l2_get_acpi_sensor_info(struct device *dev, char **module_id_str)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +       struct v4l2_acpi_gpio_parsing_data data = { };
> +       LIST_HEAD(resource_list);
> +       union acpi_object *obj;
> +       unsigned int i, j;
> +       int ret;
> +
> +       if (module_id_str)
> +               *module_id_str = NULL;
> +
> +       if (!adev)
> +               return 0;
> +
> +       obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid,
> +                                     0x00, 0x01, NULL, ACPI_TYPE_STRING);
> +       if (obj) {
> +               dev_info(dev, "Sensor module id: '%s'\n", obj->string.pointer);
> +               if (module_id_str)
> +                       *module_id_str = kstrdup(obj->string.pointer, GFP_KERNEL);
> +
> +               ACPI_FREE(obj);
> +       }

> +       if (!soc_intel_is_byt() && !soc_intel_is_cht())
> +               return 0;

So, you (might) call the previous _DSM for any SoC, is it intentional?

> +       /*
> +        * First get the GPIO-settings count and then get count GPIO-settings
> +        * values. Note the order of these may differ from the order in which
> +        * the GPIOs are listed on the ACPI resources! So we first store them all
> +        * and then enumerate the ACPI resources and match them up by pin number.
> +        */
> +       obj = acpi_evaluate_dsm_typed(adev->handle,
> +                                     &intel_sensor_gpio_info_guid, 0x00, 1,
> +                                     NULL, ACPI_TYPE_INTEGER);
> +       if (!obj)
> +               return dev_err_probe(dev, -EIO, "No _DSM entry for GPIO pin count\n");
> +
> +       data.settings_count = obj->integer.value;
> +       ACPI_FREE(obj);
> +
> +       if (data.settings_count > V4L2_SENSOR_MAX_ACPI_GPIOS)
> +               return dev_err_probe(dev, -EIO, "Too many GPIOs %u > %u\n",
> +                                    data.settings_count, V4L2_SENSOR_MAX_ACPI_GPIOS);
> +
> +       for (i = 0; i < data.settings_count; i++) {
> +               /*
> +                * i + 2 because the index of this _DSM function is 1-based
> +                * and the first function is just a count.
> +                */
> +               obj = acpi_evaluate_dsm_typed(adev->handle,
> +                                             &intel_sensor_gpio_info_guid,
> +                                             0x00, i + 2,
> +                                             NULL, ACPI_TYPE_INTEGER);
> +               if (!obj)
> +                       return dev_err_probe(dev, -EIO, "No _DSM entry for GPIO pin %u\n", i);
> +
> +               data.settings[i] = obj->integer.value;
> +               ACPI_FREE(obj);
> +       }
> +
> +       /* Since we match up by pin-number the pin-numbers must be unique */
> +       for (i = 0; i < data.settings_count; i++) {
> +               for (j = i + 1; j < data.settings_count; j++) {
> +                       if (INTEL_DSM_PIN(data.settings[i]) !=
> +                           INTEL_DSM_PIN(data.settings[j]))
> +                               continue;
> +
> +                       return dev_err_probe(dev, -EIO, "Duplicate pin number %lu\n",
> +                                            INTEL_DSM_PIN(data.settings[i]));
> +               }
> +       }
> +
> +       /* Use devm_kzalloc() for the mappings + params to auto-free them */
> +       data.map = devm_kzalloc(dev, sizeof(*data.map), GFP_KERNEL);
> +       if (!data.map)
> +               return -ENOMEM;
> +
> +       /* Now parse the ACPI resources and build the lookup table */
> +       data.dev = dev;
> +       ret = acpi_dev_get_resources(adev, &resource_list,
> +                                    v4l2_acpi_handle_gpio_res, &data);
> +       if (ret < 0)
> +               return ret;
> +
> +       acpi_dev_free_resource_list(&resource_list);
> +
> +       if (data.map_count != data.settings_count ||
> +           data.res_count != data.settings_count)
> +               dev_warn(dev, "ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
> +                        data.settings_count, data.res_count, data.map_count);
> +
> +       return devm_acpi_dev_add_driver_gpios(dev, data.map->mapping);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_get_acpi_sensor_info);
> --
> 2.39.1
>
Andy Shevchenko April 2, 2023, 7:13 p.m. UTC | #2
On Sat, Apr 1, 2023 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is another set of atomisp patches from yours truely.
>
> This basically consists of 2 parts:
>
> 1.  Further sensor driver modernization to work towards using standard
>     v4l2 sensor drivers instead of atomisp specific copies
> 1a. For some sensors some GPIOs have different polarity depending on
>     which board they are on and the order in which GPIOs are listed in
>     in the ACPI resources also is not consistent between boards.
>     Fixing this without relying on per board DMI quirks requires parsing
>     the results of a special Intel ACPI _DSM. Add a new
>     v4l2_get_acpi_sensor_info() helper for this
> 1b. Convert the gc0310 driver to use ACPI runtime pm instead of relying
>     on the direct PMIC poking from atomisp_gmin_platform
>
> 2.  Further work on removing various exotic features, specifically
>     prep work + remove support for streaming from 2 sensors at once,
>     as discussed here:
>     https://lore.kernel.org/linux-media/5309d845-063b-6dd9-529d-0f82654290f2@redhat.com/

Traditionally, for the non-commented ones:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

For the rest depending on the severity of the comment. Feel free to
add my tag where it makes sense in your opinion (no discussion
required).


> Regards,
>
> Hans
>
>
> Hans de Goede (28):
>   media: atomisp: Add v4l2_get_acpi_sensor_info() helper
>   media: atomisp: ov2680: Use v4l2_get_acpi_sensor_info() for the GPIO
>     lookups
>   media: atomisp: ov2680: Error handling fixes
>   media: atomisp: gc0310: Remove some unused structure definitions
>   media: atomisp: gc0310: Remove GC0310_TOK_*
>   media: atomisp: gc0310: Simplify gc0310_write_reg_array()
>   media: atomisp: gc0310: Remove enum gc0310_tok_type
>   media: atomisp: gc0310: Replace custom reg access functions with smbus
>     helpers
>   media: atomisp: gc0310: Remove non working flip-controls
>   media: atomisp: gc0310: Remove read-only exposure control
>   media: atomisp: gc0310: Drop custom ATOMISP_IOC_S_EXPOSURE support
>   media: atomisp: gc0310: Add exposure and gain controls
>   media: atomisp: gc0310: Add error_unlock label to s_stream()
>   media: atomisp: gc0310: Modernize and simply set_fmt(), get_fmt(),
>     etc.
>   media: atomisp: gc0310: Delay power-on till streaming is started
>   media: atomisp: gc0310: Add runtime-pm support
>   media: atomisp: gc0310: Use devm_kzalloc() for data struct
>   media: atomisp: gc0310: Switch over to ACPI powermanagement
>   media: atomisp: Remove duplicate atomisp_[start|stop]_streaming
>     prototypes
>   media: atomisp: Remove continuous mode related code from
>     atomisp_set_fmt()
>   media: atomisp: Remove custom V4L2_CID_FMT_AUTO control
>   media: atomisp: Remove snr_mbus_fmt local var from atomisp_try_fmt()
>   media: atomisp: Remove unused ATOM_ISP_MAX_WIDTH_TMP and
>     ATOM_ISP_MAX_HEIGHT_TMP
>   media: atomisp: Remove atomisp_try_fmt() call from atomisp_set_fmt()
>   media: atomisp: Drop support for streaming from 2 sensors at once
>   media: atomisp: Remove struct atomisp_sub_device index field
>   media: atomisp: gmin_platform: Make DMI quirks take precedence over
>     the _DSM table
>   media: atomisp: gmin_platform: Add Lenovo Ideapad Miix 310 gmin_vars
>
>  .../media/atomisp/i2c/atomisp-gc0310.c        | 999 ++++--------------
>  .../media/atomisp/i2c/atomisp-ov2680.c        |  33 +-
>  drivers/staging/media/atomisp/i2c/gc0310.h    | 416 +++-----
>  drivers/staging/media/atomisp/i2c/ov2680.h    |   1 -
>  .../media/atomisp/include/linux/atomisp.h     |   2 -
>  .../atomisp/include/linux/atomisp_platform.h  |   2 +
>  .../staging/media/atomisp/pci/atomisp_cmd.c   | 356 ++-----
>  .../media/atomisp/pci/atomisp_compat.h        |   4 +-
>  .../media/atomisp/pci/atomisp_compat_css20.c  |  89 +-
>  .../staging/media/atomisp/pci/atomisp_fops.c  |   9 +-
>  .../media/atomisp/pci/atomisp_gmin_platform.c | 289 ++++-
>  .../media/atomisp/pci/atomisp_internal.h      |  20 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.c |  64 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.h |   3 -
>  .../media/atomisp/pci/atomisp_subdev.c        | 111 +-
>  .../media/atomisp/pci/atomisp_subdev.h        |   6 -
>  .../staging/media/atomisp/pci/atomisp_v4l2.c  |  36 +-
>  17 files changed, 844 insertions(+), 1596 deletions(-)
>
> --
> 2.39.1
>
Hans de Goede April 9, 2023, 12:28 p.m. UTC | #3
Hi,

On 4/2/23 08:17, Andy Shevchenko wrote:
> On Sat, Apr 1, 2023 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>> +int v4l2_get_acpi_sensor_info(struct device *dev, char **module_id_str)
>> +{
>> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> +       struct v4l2_acpi_gpio_parsing_data data = { };
>> +       LIST_HEAD(resource_list);
>> +       union acpi_object *obj;
>> +       unsigned int i, j;
>> +       int ret;
>> +
>> +       if (module_id_str)
>> +               *module_id_str = NULL;
>> +
>> +       if (!adev)
>> +               return 0;
>> +
>> +       obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid,
>> +                                     0x00, 0x01, NULL, ACPI_TYPE_STRING);
>> +       if (obj) {
>> +               dev_info(dev, "Sensor module id: '%s'\n", obj->string.pointer);
>> +               if (module_id_str)
>> +                       *module_id_str = kstrdup(obj->string.pointer, GFP_KERNEL);
>> +
>> +               ACPI_FREE(obj);
>> +       }
> 
>> +       if (!soc_intel_is_byt() && !soc_intel_is_cht())
>> +               return 0;
> 
> So, you (might) call the previous _DSM for any SoC, is it intentional?

Yes the previous _DSM which gives a sensor-module-id string is also used on IPU3 and IPU6 so we want to at least try it on all x86/ACPI platforms. If the _DSM with that GUID is not supported then the call should be harmless.

Regards,

Hans
Hans de Goede April 9, 2023, 1:09 p.m. UTC | #4
Hi Andy,

On 4/2/23 21:13, Andy Shevchenko wrote:
> On Sat, Apr 1, 2023 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is another set of atomisp patches from yours truely.
>>
>> This basically consists of 2 parts:
>>
>> 1.  Further sensor driver modernization to work towards using standard
>>     v4l2 sensor drivers instead of atomisp specific copies
>> 1a. For some sensors some GPIOs have different polarity depending on
>>     which board they are on and the order in which GPIOs are listed in
>>     in the ACPI resources also is not consistent between boards.
>>     Fixing this without relying on per board DMI quirks requires parsing
>>     the results of a special Intel ACPI _DSM. Add a new
>>     v4l2_get_acpi_sensor_info() helper for this
>> 1b. Convert the gc0310 driver to use ACPI runtime pm instead of relying
>>     on the direct PMIC poking from atomisp_gmin_platform
>>
>> 2.  Further work on removing various exotic features, specifically
>>     prep work + remove support for streaming from 2 sensors at once,
>>     as discussed here:
>>     https://lore.kernel.org/linux-media/5309d845-063b-6dd9-529d-0f82654290f2@redhat.com/
> 
> Traditionally, for the non-commented ones:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> For the rest depending on the severity of the comment. Feel free to
> add my tag where it makes sense in your opinion (no discussion
> required).

Thank you for all the reviews. I've pushed a new version
addressing all your comments and adding your Reviewd-by to:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

I'll send out a pull-req to Mauro for this, wrapping up
the atomisp changes for this cycle.

Regards,

Hans



>> Hans de Goede (28):
>>   media: atomisp: Add v4l2_get_acpi_sensor_info() helper
>>   media: atomisp: ov2680: Use v4l2_get_acpi_sensor_info() for the GPIO
>>     lookups
>>   media: atomisp: ov2680: Error handling fixes
>>   media: atomisp: gc0310: Remove some unused structure definitions
>>   media: atomisp: gc0310: Remove GC0310_TOK_*
>>   media: atomisp: gc0310: Simplify gc0310_write_reg_array()
>>   media: atomisp: gc0310: Remove enum gc0310_tok_type
>>   media: atomisp: gc0310: Replace custom reg access functions with smbus
>>     helpers
>>   media: atomisp: gc0310: Remove non working flip-controls
>>   media: atomisp: gc0310: Remove read-only exposure control
>>   media: atomisp: gc0310: Drop custom ATOMISP_IOC_S_EXPOSURE support
>>   media: atomisp: gc0310: Add exposure and gain controls
>>   media: atomisp: gc0310: Add error_unlock label to s_stream()
>>   media: atomisp: gc0310: Modernize and simply set_fmt(), get_fmt(),
>>     etc.
>>   media: atomisp: gc0310: Delay power-on till streaming is started
>>   media: atomisp: gc0310: Add runtime-pm support
>>   media: atomisp: gc0310: Use devm_kzalloc() for data struct
>>   media: atomisp: gc0310: Switch over to ACPI powermanagement
>>   media: atomisp: Remove duplicate atomisp_[start|stop]_streaming
>>     prototypes
>>   media: atomisp: Remove continuous mode related code from
>>     atomisp_set_fmt()
>>   media: atomisp: Remove custom V4L2_CID_FMT_AUTO control
>>   media: atomisp: Remove snr_mbus_fmt local var from atomisp_try_fmt()
>>   media: atomisp: Remove unused ATOM_ISP_MAX_WIDTH_TMP and
>>     ATOM_ISP_MAX_HEIGHT_TMP
>>   media: atomisp: Remove atomisp_try_fmt() call from atomisp_set_fmt()
>>   media: atomisp: Drop support for streaming from 2 sensors at once
>>   media: atomisp: Remove struct atomisp_sub_device index field
>>   media: atomisp: gmin_platform: Make DMI quirks take precedence over
>>     the _DSM table
>>   media: atomisp: gmin_platform: Add Lenovo Ideapad Miix 310 gmin_vars
>>
>>  .../media/atomisp/i2c/atomisp-gc0310.c        | 999 ++++--------------
>>  .../media/atomisp/i2c/atomisp-ov2680.c        |  33 +-
>>  drivers/staging/media/atomisp/i2c/gc0310.h    | 416 +++-----
>>  drivers/staging/media/atomisp/i2c/ov2680.h    |   1 -
>>  .../media/atomisp/include/linux/atomisp.h     |   2 -
>>  .../atomisp/include/linux/atomisp_platform.h  |   2 +
>>  .../staging/media/atomisp/pci/atomisp_cmd.c   | 356 ++-----
>>  .../media/atomisp/pci/atomisp_compat.h        |   4 +-
>>  .../media/atomisp/pci/atomisp_compat_css20.c  |  89 +-
>>  .../staging/media/atomisp/pci/atomisp_fops.c  |   9 +-
>>  .../media/atomisp/pci/atomisp_gmin_platform.c | 289 ++++-
>>  .../media/atomisp/pci/atomisp_internal.h      |  20 +-
>>  .../staging/media/atomisp/pci/atomisp_ioctl.c |  64 +-
>>  .../staging/media/atomisp/pci/atomisp_ioctl.h |   3 -
>>  .../media/atomisp/pci/atomisp_subdev.c        | 111 +-
>>  .../media/atomisp/pci/atomisp_subdev.h        |   6 -
>>  .../staging/media/atomisp/pci/atomisp_v4l2.c  |  36 +-
>>  17 files changed, 844 insertions(+), 1596 deletions(-)
>>
>> --
>> 2.39.1
>>
> 
>
Andy Shevchenko April 10, 2023, 9:39 a.m. UTC | #5
On Sun, Apr 9, 2023 at 4:09 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 4/2/23 21:13, Andy Shevchenko wrote:

...

> Thank you for all the reviews. I've pushed a new version
> addressing all your comments and adding your Reviewd-by to:

Reviewed-by?

> https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

This is what answers my question in the reply to patch 28 I think.
I'll refer to this branch for AtomISP latest code.


> I'll send out a pull-req to Mauro for this, wrapping up
> the atomisp changes for this cycle.

ACK.