| Message ID | 20230705213010.390849-18-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | fc0f5b59adf5170f0f4c2dc20c13f558e2baf64a |
| Headers | show |
| Series | media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand |
On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote: > acpi_handle_info() uses the ACPI path to the handle as prefix for messages > e.g. : "\_SB_.I2C2.CAM8". > > This makes it hard for users to figure out which csi2-bridge messages > belong to which sensor since the actual sensor drivers uses the ACPI > device name (typically "HID:00") for logging. > > Extend the acpi_handle_info() (and err and warn) logging to also log > the device name to make it easier to match csi2-bridge messages with > sensor driver log messages. Fine by me, one suggestion below (up to you, guys) Reviewed-by: Andy Shevchenko <andy@kernel.org> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../media/atomisp/pci/atomisp_csi2_bridge.c | 51 ++++++++++++------- > 1 file changed, 34 insertions(+), 17 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > index 551c6fd244fd..8124be486e2e 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > @@ -131,7 +131,8 @@ static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key) > if (!val) > break; > > - acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val); > + acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n", > + dev_name(&adev->dev), key, val); Maybe (maybe!) it's a candidate to have something like v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from thinking about it? > break; > } > } > @@ -156,7 +157,8 @@ static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key > if (strcmp(key, gv->key)) > continue; > > - acpi_handle_info(adev->handle, "Using DMI entry %s=%s\n", key, gv->val); > + acpi_handle_info(adev->handle, "%s: Using DMI entry %s=%s\n", > + dev_name(&adev->dev), key, gv->val); > return kstrdup(gv->val, GFP_KERNEL); > } > > @@ -192,7 +194,8 @@ static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int defau > return int_val; > > out_use_default: > - acpi_handle_info(adev->handle, "Using default %s=%d\n", key, default_val); > + acpi_handle_info(adev->handle, "%s: Using default %s=%d\n", > + dev_name(&adev->dev), key, default_val); > return default_val; > } > > @@ -235,7 +238,8 @@ static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev) > ACPI_FREE(buffer.pointer); > > if (ret < 0) > - acpi_handle_warn(adev->handle, "Could not find PMC clk in _PR0\n"); > + acpi_handle_warn(adev->handle, "%s: Could not find PMC clk in _PR0\n", > + dev_name(&adev->dev)); > > return ret; > } > @@ -254,7 +258,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num > clk = clk_get(NULL, name); > if (IS_ERR(clk)) { > ret = PTR_ERR(clk); > - acpi_handle_err(adev->handle, "Error getting clk %s:%d\n", name, ret); > + acpi_handle_err(adev->handle, "%s: Error getting clk %s: %d\n", > + dev_name(&adev->dev), name, ret); > return ret; > } > > @@ -268,7 +273,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num > if (!ret) > ret = clk_set_rate(clk, PMC_CLK_RATE_19_2MHZ); > if (ret) > - acpi_handle_err(adev->handle, "Error setting clk-rate for %s:%d\n", name, ret); > + acpi_handle_err(adev->handle, "%s: Error setting clk-rate for %s: %d\n", > + dev_name(&adev->dev), name, ret); > > clk_put(clk); > return ret; > @@ -317,7 +323,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_ > > if (i == data->settings_count) { > acpi_handle_warn(data->adev->handle, > - "Could not find DSM GPIO settings for pin %u\n", pin); > + "%s: Could not find DSM GPIO settings for pin %u\n", > + dev_name(&data->adev->dev), pin); > return 1; > } > > @@ -329,7 +336,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_ > name = "powerdown-gpios"; > break; > default: > - acpi_handle_warn(data->adev->handle, "Unknown GPIO type 0x%02lx for pin %u\n", > + acpi_handle_warn(data->adev->handle, "%s: Unknown GPIO type 0x%02lx for pin %u\n", > + dev_name(&data->adev->dev), > INTEL_GPIO_DSM_TYPE(settings), pin); > return 1; > } > @@ -354,7 +362,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_ > data->map->mapping[i].size = 1; > data->map_count++; > > - acpi_handle_info(data->adev->handle, "%s crs %d %s pin %u active-%s\n", name, > + acpi_handle_info(data->adev->handle, "%s: %s crs %d %s pin %u active-%s\n", > + dev_name(&data->adev->dev), name, > data->res_count - 1, agpio->resource_source.string_ptr, > pin, active_low ? "low" : "high"); > > @@ -391,7 +400,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) > obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid, > 0x00, 1, NULL, ACPI_TYPE_STRING); > if (obj) { > - acpi_handle_info(adev->handle, "Sensor module id: '%s'\n", obj->string.pointer); > + acpi_handle_info(adev->handle, "%s: Sensor module id: '%s'\n", > + dev_name(&adev->dev), obj->string.pointer); > ACPI_FREE(obj); > } > > @@ -405,7 +415,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) > &intel_sensor_gpio_info_guid, 0x00, 1, > NULL, ACPI_TYPE_INTEGER); > if (!obj) { > - acpi_handle_err(adev->handle, "No _DSM entry for GPIO pin count\n"); > + acpi_handle_err(adev->handle, "%s: No _DSM entry for GPIO pin count\n", > + dev_name(&adev->dev)); > return -EIO; > } > > @@ -413,7 +424,9 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) > ACPI_FREE(obj); > > if (data.settings_count > CSI2_MAX_ACPI_GPIOS) { > - acpi_handle_err(adev->handle, "Too many GPIOs %u > %u\n", data.settings_count, CSI2_MAX_ACPI_GPIOS); > + acpi_handle_err(adev->handle, "%s: Too many GPIOs %u > %u\n", > + dev_name(&adev->dev), data.settings_count, > + CSI2_MAX_ACPI_GPIOS); > return -EOVERFLOW; > } > > @@ -427,7 +440,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) > 0x00, i + 2, > NULL, ACPI_TYPE_INTEGER); > if (!obj) { > - acpi_handle_err(adev->handle, "No _DSM entry for pin %u\n", i); > + acpi_handle_err(adev->handle, "%s: No _DSM entry for pin %u\n", > + dev_name(&adev->dev), i); > return -EIO; > } > > @@ -442,7 +456,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) > INTEL_GPIO_DSM_PIN(data.settings[j])) > continue; > > - acpi_handle_err(adev->handle, "Duplicate pin number %lu\n", > + acpi_handle_err(adev->handle, "%s: Duplicate pin number %lu\n", > + dev_name(&adev->dev), > INTEL_GPIO_DSM_PIN(data.settings[i])); > return -EIO; > } > @@ -463,12 +478,14 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) > > if (data.map_count != data.settings_count || > data.res_count != data.settings_count) > - acpi_handle_warn(adev->handle, "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); > + acpi_handle_warn(adev->handle, "%s: ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n", > + dev_name(&adev->dev), data.settings_count, > + data.res_count, data.map_count); > > ret = acpi_dev_add_driver_gpios(adev, data.map->mapping); > if (ret) > - acpi_handle_err(adev->handle, "Error adding driver GPIOs: %d\n", ret); > + acpi_handle_err(adev->handle, "%s: Error adding driver GPIOs: %d\n", > + dev_name(&adev->dev), ret); > > return ret; > } > -- > 2.41.0 >
On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote: > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote: ... > > > - acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val); > > > + acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n", > > > + dev_name(&adev->dev), key, val); > > > > Maybe (maybe!) it's a candidate to have something like > > > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from > > thinking about it? > > Or acpi_dev_info() that would take an acpi_device pointer. (which is an equivalent to the below) > Or just just dev_info(&adev->dev) ? The point is to print ACPI handle *and* device name. There are no existing helpers for that.
On Thu, Jul 06, 2023 at 03:23:33PM +0300, Andy Shevchenko wrote: > On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote: > > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote: > > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote: > > ... > > > > > - acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val); > > > > + acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n", > > > > + dev_name(&adev->dev), key, val); > > > > > > Maybe (maybe!) it's a candidate to have something like > > > > > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from > > > thinking about it? > > > > Or acpi_dev_info() that would take an acpi_device pointer. > > (which is an equivalent to the below) > > > Or just just dev_info(&adev->dev) ? > > The point is to print ACPI handle *and* device name. There are no existing > helpers for that. Then a new acpi_dev_info(struct acpi_device *adev, ...) could do that. It shouldn't be V4L2-specific in my opinion.
On Thu, Jul 06, 2023 at 04:07:08PM +0300, Laurent Pinchart wrote: > On Thu, Jul 06, 2023 at 03:23:33PM +0300, Andy Shevchenko wrote: > > On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote: > > > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote: > > > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote: ... > > > > > - acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val); > > > > > + acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n", > > > > > + dev_name(&adev->dev), key, val); > > > > > > > > Maybe (maybe!) it's a candidate to have something like > > > > > > > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from > > > > thinking about it? > > > > > > Or acpi_dev_info() that would take an acpi_device pointer. > > > > (which is an equivalent to the below) > > > > > Or just just dev_info(&adev->dev) ? > > > > The point is to print ACPI handle *and* device name. There are no existing > > helpers for that. > > Then a new acpi_dev_info(struct acpi_device *adev, ...) could do that. > It shouldn't be V4L2-specific in my opinion. But 1) so far seems only v4l2 is interested in this information (I haven't checked for existing users outside of v4l2); 2) the proposed naming doesn't suggest it's also about ACPI handle. :-) Although ACPI seems a good common denominator for these, indeed.
On Thu, Jul 06, 2023 at 04:07:08PM +0300, Laurent Pinchart wrote: > On Thu, Jul 06, 2023 at 03:23:33PM +0300, Andy Shevchenko wrote: > > On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote: > > > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote: > > > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote: > > > > ... > > > > > > > - acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val); > > > > > + acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n", > > > > > + dev_name(&adev->dev), key, val); > > > > > > > > Maybe (maybe!) it's a candidate to have something like > > > > > > > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from > > > > thinking about it? > > > > > > Or acpi_dev_info() that would take an acpi_device pointer. > > > > (which is an equivalent to the below) > > > > > Or just just dev_info(&adev->dev) ? > > > > The point is to print ACPI handle *and* device name. There are no existing > > helpers for that. > > Then a new acpi_dev_info(struct acpi_device *adev, ...) could do that. > It shouldn't be V4L2-specific in my opinion. I certainy have no objections for having a helper for this, but IMO the current code is fine, too.
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c index 551c6fd244fd..8124be486e2e 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c @@ -131,7 +131,8 @@ static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key) if (!val) break; - acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val); + acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n", + dev_name(&adev->dev), key, val); break; } } @@ -156,7 +157,8 @@ static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key if (strcmp(key, gv->key)) continue; - acpi_handle_info(adev->handle, "Using DMI entry %s=%s\n", key, gv->val); + acpi_handle_info(adev->handle, "%s: Using DMI entry %s=%s\n", + dev_name(&adev->dev), key, gv->val); return kstrdup(gv->val, GFP_KERNEL); } @@ -192,7 +194,8 @@ static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int defau return int_val; out_use_default: - acpi_handle_info(adev->handle, "Using default %s=%d\n", key, default_val); + acpi_handle_info(adev->handle, "%s: Using default %s=%d\n", + dev_name(&adev->dev), key, default_val); return default_val; } @@ -235,7 +238,8 @@ static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev) ACPI_FREE(buffer.pointer); if (ret < 0) - acpi_handle_warn(adev->handle, "Could not find PMC clk in _PR0\n"); + acpi_handle_warn(adev->handle, "%s: Could not find PMC clk in _PR0\n", + dev_name(&adev->dev)); return ret; } @@ -254,7 +258,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num clk = clk_get(NULL, name); if (IS_ERR(clk)) { ret = PTR_ERR(clk); - acpi_handle_err(adev->handle, "Error getting clk %s:%d\n", name, ret); + acpi_handle_err(adev->handle, "%s: Error getting clk %s: %d\n", + dev_name(&adev->dev), name, ret); return ret; } @@ -268,7 +273,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num if (!ret) ret = clk_set_rate(clk, PMC_CLK_RATE_19_2MHZ); if (ret) - acpi_handle_err(adev->handle, "Error setting clk-rate for %s:%d\n", name, ret); + acpi_handle_err(adev->handle, "%s: Error setting clk-rate for %s: %d\n", + dev_name(&adev->dev), name, ret); clk_put(clk); return ret; @@ -317,7 +323,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_ if (i == data->settings_count) { acpi_handle_warn(data->adev->handle, - "Could not find DSM GPIO settings for pin %u\n", pin); + "%s: Could not find DSM GPIO settings for pin %u\n", + dev_name(&data->adev->dev), pin); return 1; } @@ -329,7 +336,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_ name = "powerdown-gpios"; break; default: - acpi_handle_warn(data->adev->handle, "Unknown GPIO type 0x%02lx for pin %u\n", + acpi_handle_warn(data->adev->handle, "%s: Unknown GPIO type 0x%02lx for pin %u\n", + dev_name(&data->adev->dev), INTEL_GPIO_DSM_TYPE(settings), pin); return 1; } @@ -354,7 +362,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_ data->map->mapping[i].size = 1; data->map_count++; - acpi_handle_info(data->adev->handle, "%s crs %d %s pin %u active-%s\n", name, + acpi_handle_info(data->adev->handle, "%s: %s crs %d %s pin %u active-%s\n", + dev_name(&data->adev->dev), name, data->res_count - 1, agpio->resource_source.string_ptr, pin, active_low ? "low" : "high"); @@ -391,7 +400,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid, 0x00, 1, NULL, ACPI_TYPE_STRING); if (obj) { - acpi_handle_info(adev->handle, "Sensor module id: '%s'\n", obj->string.pointer); + acpi_handle_info(adev->handle, "%s: Sensor module id: '%s'\n", + dev_name(&adev->dev), obj->string.pointer); ACPI_FREE(obj); } @@ -405,7 +415,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) &intel_sensor_gpio_info_guid, 0x00, 1, NULL, ACPI_TYPE_INTEGER); if (!obj) { - acpi_handle_err(adev->handle, "No _DSM entry for GPIO pin count\n"); + acpi_handle_err(adev->handle, "%s: No _DSM entry for GPIO pin count\n", + dev_name(&adev->dev)); return -EIO; } @@ -413,7 +424,9 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) ACPI_FREE(obj); if (data.settings_count > CSI2_MAX_ACPI_GPIOS) { - acpi_handle_err(adev->handle, "Too many GPIOs %u > %u\n", data.settings_count, CSI2_MAX_ACPI_GPIOS); + acpi_handle_err(adev->handle, "%s: Too many GPIOs %u > %u\n", + dev_name(&adev->dev), data.settings_count, + CSI2_MAX_ACPI_GPIOS); return -EOVERFLOW; } @@ -427,7 +440,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) 0x00, i + 2, NULL, ACPI_TYPE_INTEGER); if (!obj) { - acpi_handle_err(adev->handle, "No _DSM entry for pin %u\n", i); + acpi_handle_err(adev->handle, "%s: No _DSM entry for pin %u\n", + dev_name(&adev->dev), i); return -EIO; } @@ -442,7 +456,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) INTEL_GPIO_DSM_PIN(data.settings[j])) continue; - acpi_handle_err(adev->handle, "Duplicate pin number %lu\n", + acpi_handle_err(adev->handle, "%s: Duplicate pin number %lu\n", + dev_name(&adev->dev), INTEL_GPIO_DSM_PIN(data.settings[i])); return -EIO; } @@ -463,12 +478,14 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev) if (data.map_count != data.settings_count || data.res_count != data.settings_count) - acpi_handle_warn(adev->handle, "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); + acpi_handle_warn(adev->handle, "%s: ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n", + dev_name(&adev->dev), data.settings_count, + data.res_count, data.map_count); ret = acpi_dev_add_driver_gpios(adev, data.map->mapping); if (ret) - acpi_handle_err(adev->handle, "Error adding driver GPIOs: %d\n", ret); + acpi_handle_err(adev->handle, "%s: Error adding driver GPIOs: %d\n", + dev_name(&adev->dev), ret); return ret; }
acpi_handle_info() uses the ACPI path to the handle as prefix for messages e.g. : "\_SB_.I2C2.CAM8". This makes it hard for users to figure out which csi2-bridge messages belong to which sensor since the actual sensor drivers uses the ACPI device name (typically "HID:00") for logging. Extend the acpi_handle_info() (and err and warn) logging to also log the device name to make it easier to match csi2-bridge messages with sensor driver log messages. Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../media/atomisp/pci/atomisp_csi2_bridge.c | 51 ++++++++++++------- 1 file changed, 34 insertions(+), 17 deletions(-)