| Message ID | 20230705213010.390849-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand |
On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: > Some ACPI glue code (1) may want to do an acpi_device_id match while > it only has a struct acpi_device available because the first physical > node may not have been instantiated yet. > > Add a new acpi_match_acpi_device() helper for this, which takes > a "struct acpi_device *" as argument rather then the "struct device *" > which acpi_match_device() takes. > > 1) E.g. code which parses ACPI tables to transforms them > into more standard kernel data structures like fwnodes Looks like it's v1 of my original patch, anyway this is now in Linux Next as 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").
Hi Andy, On Thu, Jul 06, 2023 at 12:14:27PM +0300, Andy Shevchenko wrote: > On Thu, Jul 06, 2023 at 07:47:38AM +0000, Sakari Ailus wrote: > > On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote: > > ... > > > > +static int dw9719_init_controls(struct dw9719_device *dw9719) > > > +{ > > > + const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops; > > > + int ret; > > > + > > > + ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1); > > > + if (ret) > > > + return ret; > > > > This check can be dropped. > > The obvious question why that API returns int instead of void? I guess it's for historical reasons. The control handler initialisation functions won't do anything if the handler is in error state. I guess this could be changed. Cc Hans. > > > > + dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops, > > > + V4L2_CID_FOCUS_ABSOLUTE, 0, > > > + DW9719_MAX_FOCUS_POS, 1, 0); > > > + > > > + if (dw9719->ctrls.handler.error) { > > > + dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n"); > > > + ret = dw9719->ctrls.handler.error; > > > + goto err_free_handler; > > > + } > > > + > > > + dw9719->sd.ctrl_handler = &dw9719->ctrls.handler; > > > > + return ret; > > return 0; > > ? Makes sense. > > > > +err_free_handler: > > > + v4l2_ctrl_handler_free(&dw9719->ctrls.handler); > > > + return ret; > > > +} >
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 >
Hi Andy, On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote: ... > > +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val) > > +{ > > + struct i2c_msg msg[2]; > > + u8 buf[2] = { reg }; > > + int ret; > > + > > + msg[0].addr = client->addr; > > + msg[0].flags = 0; > > > + msg[0].len = 1; > > + msg[0].buf = buf; > > sizeof(buf[0]) > &buf[0] > > looks more explicit. The original seems fine to me. Note that this code will disappear soon. Same for the other comments regarding register access functions (apart from the return one). > > > + msg[1].addr = client->addr; > > + msg[1].flags = I2C_M_RD; > > + msg[1].len = 1; > > + msg[1].buf = &buf[1]; > > Ditto. > > > + *val = 0; > > + > > + ret = i2c_transfer(client->adapter, msg, 2); > > ARRAY_SIZE() > > > + if (ret < 0) > > + return ret; > > + > > + *val = buf[1]; > > + > > + return 0; > > +} > > But as Sakari said this perhaps could go into CCI library. > > ... > > > + ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val); > > + if (ret < 0) > > + return ret; > > + > > + if (val != DW9719_ID) { > > + dev_err(dw9719->dev, "Failed to detect correct id\n"); > > + ret = -ENXIO; > > return -ENXIO; > > > + } > > + > > + return 0; > > ... > > > + /* Need 100us to transit from SHUTDOWN to STANDBY*/ > > Missing space. > > > + usleep_range(100, 1000); > > Perhaps fsleep() would be better, but I'm fine with either here. > > ... > > > +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value) > > +{ > > + int ret; > > Redundant? > > > + value = clamp(value, 0, DW9719_MAX_FOCUS_POS); This is redundant, too, btw., the control framework already handles this. > > > + ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > return _wr16(...); > > or can it return positive values? > > > +} > > ... > > > +static int __maybe_unused dw9719_suspend(struct device *dev) > > Can we use new PM macros instead of __maybe_unused? > > > +{ > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > + struct dw9719_device *dw9719 = to_dw9719_device(sd); > > + int ret; > > + int val; > > + > > + for (val = dw9719->ctrls.focus->val; val >= 0; > > + val -= DW9719_CTRL_STEPS) { > > + ret = dw9719_t_focus_abs(dw9719, val); > > + if (ret) > > + return ret; > > > + usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10); > > fsleep() ? > > > + } > > + > > + return dw9719_power_down(dw9719); > > +} > > > +static int __maybe_unused dw9719_resume(struct device *dev) > > +{ > > As per above function. > > ... > > > +err_power_down: > > In one functions you use err_ in another fail_, be consistent. > > > + dw9719_power_down(dw9719); > > + return ret; > > +} > > ... > > > + dw9719->regulator = devm_regulator_get(&client->dev, "vdd"); > > + if (IS_ERR(dw9719->regulator)) > > + return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator), > > With > > struct device *dev = &client->dev; > > code may look neater. I prefer it as-is. > > > + "getting regulator\n"); >
On Thu, Jul 06, 2023 at 10:27:55AM +0000, Sakari Ailus wrote: > On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote: ... > > > + dw9719->regulator = devm_regulator_get(&client->dev, "vdd"); > > > + if (IS_ERR(dw9719->regulator)) > > > + return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator), > > > > With > > > > struct device *dev = &client->dev; > > > > code may look neater. > > I prefer it as-is. May I ask what the technical rationale behind your preferences? Esp. taking into account how picky you are for 80 character limit. > > > + "getting regulator\n");
Hi Andy, On Thu, Jul 06, 2023 at 01:48:25PM +0300, Andy Shevchenko wrote: > On Thu, Jul 06, 2023 at 10:27:55AM +0000, Sakari Ailus wrote: > > On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote: > > ... > > > > > + dw9719->regulator = devm_regulator_get(&client->dev, "vdd"); > > > > + if (IS_ERR(dw9719->regulator)) > > > > + return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator), > > > > > > With > > > > > > struct device *dev = &client->dev; > > > > > > code may look neater. > > > > I prefer it as-is. > > May I ask what the technical rationale behind your preferences? Esp. taking > into account how picky you are for 80 character limit. The device is already available via &client->dev. Sure, you do remove a few characters per line but also introduce a new variable to be aware of. In this case there's no line break that would be affected. That being said, I certainly don't have a strong opinion on this. If Hans prefers to have a local variable for this I'm totally fine with that. I just think there's no need to.
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() 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? Or acpi_dev_info() that would take an acpi_device pointer. Or just just dev_info(&adev->dev) ? > > 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; > > }
Hi Hans On Wed, 5 Jul 2023 at 22:33, Hans de Goede <hdegoede@redhat.com> wrote: > > From: Daniel Scally <djrscally@gmail.com> > > Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice > and registers a control to set the desired focus. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v3 (Hans de Goede) > - New patch in v3 of this series based on Dan Scally's initial > DW9719 upstream submission: > https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/ > - Drop hack to enable "vsio" regulator, this is no longer necessary > now that there is a device-link making the VCM a runtime-pm consumer > of the sensor > - Add checking of device-properties for sac-mode and vcm-freq, > as requested by Sakari, this is done similar to the dw9768: > Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml > Note no devicetree binding doc is added since currently only > i2c_device_id enumeration (instantiated by IPU bridge) is > supported > --- > MAINTAINERS | 7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 446 insertions(+) > create mode 100644 drivers/media/i2c/dw9719.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 494682dd437f..cf8e799f6ea2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6266,6 +6266,13 @@ T: git git://linuxtv.org/media_tree.git > F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml > F: drivers/media/i2c/dw9714.c > > +DONGWOON DW9719 LENS VOICE COIL DRIVER > +M: Daniel Scally <djrscally@gmail.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: drivers/media/i2c/dw9719.c > + > DONGWOON DW9768 LENS VOICE COIL DRIVER > L: linux-media@vger.kernel.org > S: Orphan > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 26dc365365d8..4864f1df3c7a 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -875,6 +875,17 @@ config VIDEO_DW9714 > capability. This is designed for linear control of > voice coil motors, controlled via I2C serial interface. > > +config VIDEO_DW9719 > + tristate "DW9719 lens voice coil support" > + depends on I2C && VIDEO_DEV > + select MEDIA_CONTROLLER > + select VIDEO_V4L2_SUBDEV_API > + select V4L2_ASYNC > + help > + This is a driver for the DW9719 camera lens voice coil. > + This is designed for linear control of voice coil motors, > + controlled via I2C serial interface. > + > config VIDEO_DW9768 > tristate "DW9768 lens voice coil support" > depends on I2C && VIDEO_DEV > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index d175a2e2fb19..745f8d07e649 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o > obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o > obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o > obj-$(CONFIG_VIDEO_DW9714) += dw9714.o > +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o > obj-$(CONFIG_VIDEO_DW9768) += dw9768.o > obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o > obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ > diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c > new file mode 100644 > index 000000000000..7b83ae102131 > --- /dev/null > +++ b/drivers/media/i2c/dw9719.c > @@ -0,0 +1,427 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2012 Intel Corporation > + > +/* > + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo: > + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5 > + */ > + > +#include <asm/unaligned.h> > + > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > +#include <linux/types.h> > + > +#include <media/v4l2-common.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-subdev.h> > + > +#define DW9719_MAX_FOCUS_POS 1023 > +#define DW9719_CTRL_STEPS 16 > +#define DW9719_CTRL_DELAY_US 1000 > +#define DELAY_MAX_PER_STEP_NS (1000000 * 1023) > + > +#define DW9719_INFO 0 > +#define DW9719_ID 0xF1 > +#define DW9719_CONTROL 2 > +#define DW9719_VCM_CURRENT 3 > + > +#define DW9719_MODE 6 > +#define DW9719_VCM_FREQ 7 > + > +#define DW9719_MODE_SAC_SHIFT 4 > +#define DW9719_MODE_SAC3 4 > + > +#define DW9719_DEFAULT_VCM_FREQ 0x60 > + > +#define DW9719_ENABLE_RINGING 0x02 This register setup and the ramping up/down code is nearly identical to the existing dw9807-vcm driver[1]. Admittedly that doesn't expose SAC (Smart Actuator Control) for damping the movement, but dw9807 does support it. The only really quirky bit here is the "Jiggle SCL pin to wake up device", but I can't find a datasheet to know anything more about that. The other apparent distinction would be whether DW9719 has the VBUSY bit in the status register that dw9807 is abiding by, whilst this driver doesn't. Should this be a new driver, or a variant of dw9807-vcm? Cheers Dave [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/dw9807-vcm.c > + > +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd) > + > +struct dw9719_device { > + struct device *dev; > + struct i2c_client *client; > + struct regulator *regulator; > + struct v4l2_subdev sd; > + u32 sac_mode; > + u32 vcm_freq; > + > + struct dw9719_v4l2_ctrls { > + struct v4l2_ctrl_handler handler; > + struct v4l2_ctrl *focus; > + } ctrls; > +}; > + > +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val) > +{ > + struct i2c_msg msg[2]; > + u8 buf[2] = { reg }; > + int ret; > + > + msg[0].addr = client->addr; > + msg[0].flags = 0; > + msg[0].len = 1; > + msg[0].buf = buf; > + > + msg[1].addr = client->addr; > + msg[1].flags = I2C_M_RD; > + msg[1].len = 1; > + msg[1].buf = &buf[1]; > + *val = 0; > + > + ret = i2c_transfer(client->adapter, msg, 2); > + if (ret < 0) > + return ret; > + > + *val = buf[1]; > + > + return 0; > +} > + > +static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val) > +{ > + struct i2c_msg msg; > + int ret; > + > + u8 buf[2] = { reg, val }; > + > + msg.addr = client->addr; > + msg.flags = 0; > + msg.len = sizeof(buf); > + msg.buf = buf; > + > + ret = i2c_transfer(client->adapter, &msg, 1); > + > + return ret < 0 ? ret : 0; > +} > + > +static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val) > +{ > + struct i2c_msg msg; > + u8 buf[3] = { reg }; > + int ret; > + > + put_unaligned_be16(val, buf + 1); > + > + msg.addr = client->addr; > + msg.flags = 0; > + msg.len = sizeof(buf); > + msg.buf = buf; > + > + ret = i2c_transfer(client->adapter, &msg, 1); > + > + return ret < 0 ? ret : 0; > +} > + > +static int dw9719_detect(struct dw9719_device *dw9719) > +{ > + int ret; > + u8 val; > + > + ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val); > + if (ret < 0) > + return ret; > + > + if (val != DW9719_ID) { > + dev_err(dw9719->dev, "Failed to detect correct id\n"); > + ret = -ENXIO; > + } > + > + return 0; > +} > + > +static int dw9719_power_down(struct dw9719_device *dw9719) > +{ > + return regulator_disable(dw9719->regulator); > +} > + > +static int dw9719_power_up(struct dw9719_device *dw9719) > +{ > + int ret; > + > + ret = regulator_enable(dw9719->regulator); > + if (ret) > + return ret; > + > + /* Jiggle SCL pin to wake up device */ > + ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1); > + > + /* Need 100us to transit from SHUTDOWN to STANDBY*/ > + usleep_range(100, 1000); > + > + ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, > + DW9719_ENABLE_RINGING); > + if (ret < 0) > + goto fail_powerdown; > + > + ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE, > + dw9719->sac_mode << DW9719_MODE_SAC_SHIFT); > + if (ret < 0) > + goto fail_powerdown; > + > + ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ, dw9719->vcm_freq); > + if (ret < 0) > + goto fail_powerdown; > + > + return 0; > + > +fail_powerdown: > + dw9719_power_down(dw9719); > + return ret; > +} > + > +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value) > +{ > + int ret; > + > + value = clamp(value, 0, DW9719_MAX_FOCUS_POS); > + ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct dw9719_device *dw9719 = container_of(ctrl->handler, > + struct dw9719_device, > + ctrls.handler); > + int ret; > + > + /* Only apply changes to the controls if the device is powered up */ > + if (!pm_runtime_get_if_in_use(dw9719->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_FOCUS_ABSOLUTE: > + ret = dw9719_t_focus_abs(dw9719, ctrl->val); > + break; > + default: > + ret = -EINVAL; > + } > + > + pm_runtime_put(dw9719->dev); > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops dw9719_ctrl_ops = { > + .s_ctrl = dw9719_set_ctrl, > +}; > + > +static int __maybe_unused dw9719_suspend(struct device *dev) > +{ > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct dw9719_device *dw9719 = to_dw9719_device(sd); > + int ret; > + int val; > + > + for (val = dw9719->ctrls.focus->val; val >= 0; > + val -= DW9719_CTRL_STEPS) { > + ret = dw9719_t_focus_abs(dw9719, val); > + if (ret) > + return ret; > + > + usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10); > + } > + > + return dw9719_power_down(dw9719); > +} > + > +static int __maybe_unused dw9719_resume(struct device *dev) > +{ > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct dw9719_device *dw9719 = to_dw9719_device(sd); > + int current_focus = dw9719->ctrls.focus->val; > + int ret; > + int val; > + > + ret = dw9719_power_up(dw9719); > + if (ret) > + return ret; > + > + for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus; > + val += DW9719_CTRL_STEPS) { > + ret = dw9719_t_focus_abs(dw9719, val); > + if (ret) > + goto err_power_down; > + > + usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10); > + } > + > + return 0; > + > +err_power_down: > + dw9719_power_down(dw9719); > + return ret; > +} > + > +static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + return pm_runtime_resume_and_get(sd->dev); > +} > + > +static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + pm_runtime_put(sd->dev); > + > + return 0; > +} > + > +static const struct v4l2_subdev_internal_ops dw9719_internal_ops = { > + .open = dw9719_open, > + .close = dw9719_close, > +}; > + > +static int dw9719_init_controls(struct dw9719_device *dw9719) > +{ > + const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops; > + int ret; > + > + ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1); > + if (ret) > + return ret; > + > + dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops, > + V4L2_CID_FOCUS_ABSOLUTE, 0, > + DW9719_MAX_FOCUS_POS, 1, 0); > + > + if (dw9719->ctrls.handler.error) { > + dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n"); > + ret = dw9719->ctrls.handler.error; > + goto err_free_handler; > + } > + > + dw9719->sd.ctrl_handler = &dw9719->ctrls.handler; > + > + return ret; > + > +err_free_handler: > + v4l2_ctrl_handler_free(&dw9719->ctrls.handler); > + return ret; > +} > + > +static const struct v4l2_subdev_ops dw9719_ops = { }; > + > +static int dw9719_probe(struct i2c_client *client) > +{ > + struct dw9719_device *dw9719; > + int ret; > + > + dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL); > + if (!dw9719) > + return -ENOMEM; > + > + dw9719->client = client; > + dw9719->dev = &client->dev; > + > + dw9719->sac_mode = DW9719_MODE_SAC3; > + dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ; > + > + /* Optional indication of SAC mode select */ > + device_property_read_u32(&client->dev, "dongwoon,sac-mode", > + &dw9719->sac_mode); > + > + /* Optional indication of VCM frequency */ > + device_property_read_u32(&client->dev, "dongwoon,vcm-freq", > + &dw9719->vcm_freq); > + > + dw9719->regulator = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(dw9719->regulator)) > + return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator), > + "getting regulator\n"); > + > + v4l2_i2c_subdev_init(&dw9719->sd, client, &dw9719_ops); > + dw9719->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + dw9719->sd.internal_ops = &dw9719_internal_ops; > + > + ret = dw9719_init_controls(dw9719); > + if (ret) > + return ret; > + > + ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL); > + if (ret < 0) > + goto err_free_ctrl_handler; > + > + dw9719->sd.entity.function = MEDIA_ENT_F_LENS; > + > + /* > + * We need the driver to work in the event that pm runtime is disable in > + * the kernel, so power up and verify the chip now. In the event that > + * runtime pm is disabled this will leave the chip on, so that the lens > + * will work. > + */ > + > + ret = dw9719_power_up(dw9719); > + if (ret) > + goto err_cleanup_media; > + > + ret = dw9719_detect(dw9719); > + if (ret) > + goto err_powerdown; > + > + pm_runtime_set_active(&client->dev); > + pm_runtime_get_noresume(&client->dev); > + pm_runtime_enable(&client->dev); > + > + ret = v4l2_async_register_subdev(&dw9719->sd); > + if (ret < 0) > + goto err_pm_runtime; > + > + pm_runtime_set_autosuspend_delay(&client->dev, 1000); > + pm_runtime_use_autosuspend(&client->dev); > + pm_runtime_put_autosuspend(&client->dev); > + > + return ret; > + > +err_pm_runtime: > + pm_runtime_disable(&client->dev); > + pm_runtime_put_noidle(&client->dev); > +err_powerdown: > + dw9719_power_down(dw9719); > +err_cleanup_media: > + media_entity_cleanup(&dw9719->sd.entity); > +err_free_ctrl_handler: > + v4l2_ctrl_handler_free(&dw9719->ctrls.handler); > + > + return ret; > +} > + > +static void dw9719_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device, sd); > + > + pm_runtime_disable(&client->dev); > + v4l2_async_unregister_subdev(sd); > + v4l2_ctrl_handler_free(&dw9719->ctrls.handler); > + media_entity_cleanup(&dw9719->sd.entity); > +} > + > +static const struct i2c_device_id dw9719_id_table[] = { > + { "dw9719" }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, dw9719_id_table); > + > +static const struct dev_pm_ops dw9719_pm_ops = { > + SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL) > +}; > + > +static struct i2c_driver dw9719_i2c_driver = { > + .driver = { > + .name = "dw9719", > + .pm = &dw9719_pm_ops, > + }, > + .probe_new = dw9719_probe, > + .remove = dw9719_remove, > + .id_table = dw9719_id_table, > +}; > +module_i2c_driver(dw9719_i2c_driver); > + > +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > +MODULE_DESCRIPTION("DW9719 VCM Driver"); > +MODULE_LICENSE("GPL"); > -- > 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.
Hi, On 7/6/23 11:19, Andy Shevchenko wrote: > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: >> Some ACPI glue code (1) may want to do an acpi_device_id match while >> it only has a struct acpi_device available because the first physical >> node may not have been instantiated yet. >> >> Add a new acpi_match_acpi_device() helper for this, which takes >> a "struct acpi_device *" as argument rather then the "struct device *" >> which acpi_match_device() takes. >> >> 1) E.g. code which parses ACPI tables to transforms them >> into more standard kernel data structures like fwnodes > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). Ah interesting, it does indeed look a lot like your version. but it was developed independently. Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp changes in this series which rely on this are intended for 6.6-rc1 too. So we still need to figure out how to merge this. Regards, Hans
Hi Dave, On 7/6/23 13:18, Dave Stevenson wrote: > Hi Hans > > On Wed, 5 Jul 2023 at 22:33, Hans de Goede <hdegoede@redhat.com> wrote: >> >> From: Daniel Scally <djrscally@gmail.com> >> >> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice >> and registers a control to set the desired focus. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3 (Hans de Goede) >> - New patch in v3 of this series based on Dan Scally's initial >> DW9719 upstream submission: >> https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/ >> - Drop hack to enable "vsio" regulator, this is no longer necessary >> now that there is a device-link making the VCM a runtime-pm consumer >> of the sensor >> - Add checking of device-properties for sac-mode and vcm-freq, >> as requested by Sakari, this is done similar to the dw9768: >> Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml >> Note no devicetree binding doc is added since currently only >> i2c_device_id enumeration (instantiated by IPU bridge) is >> supported >> --- >> MAINTAINERS | 7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile | 1 + >> drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++ >> 4 files changed, 446 insertions(+) >> create mode 100644 drivers/media/i2c/dw9719.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 494682dd437f..cf8e799f6ea2 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -6266,6 +6266,13 @@ T: git git://linuxtv.org/media_tree.git >> F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml >> F: drivers/media/i2c/dw9714.c >> >> +DONGWOON DW9719 LENS VOICE COIL DRIVER >> +M: Daniel Scally <djrscally@gmail.com> >> +L: linux-media@vger.kernel.org >> +S: Maintained >> +T: git git://linuxtv.org/media_tree.git >> +F: drivers/media/i2c/dw9719.c >> + >> DONGWOON DW9768 LENS VOICE COIL DRIVER >> L: linux-media@vger.kernel.org >> S: Orphan >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 26dc365365d8..4864f1df3c7a 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -875,6 +875,17 @@ config VIDEO_DW9714 >> capability. This is designed for linear control of >> voice coil motors, controlled via I2C serial interface. >> >> +config VIDEO_DW9719 >> + tristate "DW9719 lens voice coil support" >> + depends on I2C && VIDEO_DEV >> + select MEDIA_CONTROLLER >> + select VIDEO_V4L2_SUBDEV_API >> + select V4L2_ASYNC >> + help >> + This is a driver for the DW9719 camera lens voice coil. >> + This is designed for linear control of voice coil motors, >> + controlled via I2C serial interface. >> + >> config VIDEO_DW9768 >> tristate "DW9768 lens voice coil support" >> depends on I2C && VIDEO_DEV >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index d175a2e2fb19..745f8d07e649 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o >> obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o >> obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o >> obj-$(CONFIG_VIDEO_DW9714) += dw9714.o >> +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o >> obj-$(CONFIG_VIDEO_DW9768) += dw9768.o >> obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o >> obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ >> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c >> new file mode 100644 >> index 000000000000..7b83ae102131 >> --- /dev/null >> +++ b/drivers/media/i2c/dw9719.c >> @@ -0,0 +1,427 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2012 Intel Corporation >> + >> +/* >> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo: >> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5 >> + */ >> + >> +#include <asm/unaligned.h> >> + >> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/types.h> >> + >> +#include <media/v4l2-common.h> >> +#include <media/v4l2-ctrls.h> >> +#include <media/v4l2-subdev.h> >> + >> +#define DW9719_MAX_FOCUS_POS 1023 >> +#define DW9719_CTRL_STEPS 16 >> +#define DW9719_CTRL_DELAY_US 1000 >> +#define DELAY_MAX_PER_STEP_NS (1000000 * 1023) >> + >> +#define DW9719_INFO 0 >> +#define DW9719_ID 0xF1 >> +#define DW9719_CONTROL 2 >> +#define DW9719_VCM_CURRENT 3 >> + >> +#define DW9719_MODE 6 >> +#define DW9719_VCM_FREQ 7 >> + >> +#define DW9719_MODE_SAC_SHIFT 4 >> +#define DW9719_MODE_SAC3 4 >> + >> +#define DW9719_DEFAULT_VCM_FREQ 0x60 >> + >> +#define DW9719_ENABLE_RINGING 0x02 > > This register setup and the ramping up/down code is nearly identical > to the existing dw9807-vcm driver[1]. Admittedly that doesn't expose > SAC (Smart Actuator Control) for damping the movement, but dw9807 does > support it. > > The only really quirky bit here is the "Jiggle SCL pin to wake up > device", but I can't find a datasheet to know anything more about > that. The other apparent distinction would be whether DW9719 has the > VBUSY bit in the status register that dw9807 is abiding by, whilst > this driver doesn't. > > Should this be a new driver, or a variant of dw9807-vcm? That is a good question and there also have been lots of other remarks on this driver / patch. So lets continue with this series without this patch (it is not necessary for the rest of the series) and then I'll submit a new version of the patch as a new standalone series/patch when I have some time to work on this more. I'll then also check to see if instead of this driver a patch to modify dw9807-vcm makes more sense. Regards, Hans
On Thu, Jul 06, 2023 at 02:29:35PM +0200, Hans de Goede wrote: > On 7/6/23 11:19, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: ... > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). > > Ah interesting, it does indeed look a lot like your version. > but it was developed independently. Very interesting! It's a second patch of me that collides with someone's else work. > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp > changes in this series which rely on this are intended for 6.6-rc1 too. > > So we still need to figure out how to merge this. Standard way? I.e. ask Rafael for immutable tag/branch?
Hi Dave, On 7/6/23 13:18, Dave Stevenson wrote: > Hi Hans > > On Wed, 5 Jul 2023 at 22:33, Hans de Goede <hdegoede@redhat.com> wrote: >> >> From: Daniel Scally <djrscally@gmail.com> >> >> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice >> and registers a control to set the desired focus. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3 (Hans de Goede) >> - New patch in v3 of this series based on Dan Scally's initial >> DW9719 upstream submission: >> https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/ >> - Drop hack to enable "vsio" regulator, this is no longer necessary >> now that there is a device-link making the VCM a runtime-pm consumer >> of the sensor >> - Add checking of device-properties for sac-mode and vcm-freq, >> as requested by Sakari, this is done similar to the dw9768: >> Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml >> Note no devicetree binding doc is added since currently only >> i2c_device_id enumeration (instantiated by IPU bridge) is >> supported >> --- >> MAINTAINERS | 7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile | 1 + >> drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++ >> 4 files changed, 446 insertions(+) >> create mode 100644 drivers/media/i2c/dw9719.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 494682dd437f..cf8e799f6ea2 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -6266,6 +6266,13 @@ T: git git://linuxtv.org/media_tree.git >> F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml >> F: drivers/media/i2c/dw9714.c >> >> +DONGWOON DW9719 LENS VOICE COIL DRIVER >> +M: Daniel Scally <djrscally@gmail.com> >> +L: linux-media@vger.kernel.org >> +S: Maintained >> +T: git git://linuxtv.org/media_tree.git >> +F: drivers/media/i2c/dw9719.c >> + >> DONGWOON DW9768 LENS VOICE COIL DRIVER >> L: linux-media@vger.kernel.org >> S: Orphan >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 26dc365365d8..4864f1df3c7a 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -875,6 +875,17 @@ config VIDEO_DW9714 >> capability. This is designed for linear control of >> voice coil motors, controlled via I2C serial interface. >> >> +config VIDEO_DW9719 >> + tristate "DW9719 lens voice coil support" >> + depends on I2C && VIDEO_DEV >> + select MEDIA_CONTROLLER >> + select VIDEO_V4L2_SUBDEV_API >> + select V4L2_ASYNC >> + help >> + This is a driver for the DW9719 camera lens voice coil. >> + This is designed for linear control of voice coil motors, >> + controlled via I2C serial interface. >> + >> config VIDEO_DW9768 >> tristate "DW9768 lens voice coil support" >> depends on I2C && VIDEO_DEV >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index d175a2e2fb19..745f8d07e649 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o >> obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o >> obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o >> obj-$(CONFIG_VIDEO_DW9714) += dw9714.o >> +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o >> obj-$(CONFIG_VIDEO_DW9768) += dw9768.o >> obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o >> obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/ >> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c >> new file mode 100644 >> index 000000000000..7b83ae102131 >> --- /dev/null >> +++ b/drivers/media/i2c/dw9719.c >> @@ -0,0 +1,427 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2012 Intel Corporation >> + >> +/* >> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo: >> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5 >> + */ >> + >> +#include <asm/unaligned.h> >> + >> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/types.h> >> + >> +#include <media/v4l2-common.h> >> +#include <media/v4l2-ctrls.h> >> +#include <media/v4l2-subdev.h> >> + >> +#define DW9719_MAX_FOCUS_POS 1023 >> +#define DW9719_CTRL_STEPS 16 >> +#define DW9719_CTRL_DELAY_US 1000 >> +#define DELAY_MAX_PER_STEP_NS (1000000 * 1023) >> + >> +#define DW9719_INFO 0 >> +#define DW9719_ID 0xF1 >> +#define DW9719_CONTROL 2 >> +#define DW9719_VCM_CURRENT 3 >> + >> +#define DW9719_MODE 6 >> +#define DW9719_VCM_FREQ 7 >> + >> +#define DW9719_MODE_SAC_SHIFT 4 >> +#define DW9719_MODE_SAC3 4 >> + >> +#define DW9719_DEFAULT_VCM_FREQ 0x60 >> + >> +#define DW9719_ENABLE_RINGING 0x02 > > This register setup and the ramping up/down code is nearly identical > to the existing dw9807-vcm driver[1]. Admittedly that doesn't expose > SAC (Smart Actuator Control) for damping the movement, but dw9807 does > support it. > > The only really quirky bit here is the "Jiggle SCL pin to wake up > device", but I can't find a datasheet to know anything more about > that. The other apparent distinction would be whether DW9719 has the > VBUSY bit in the status register that dw9807 is abiding by, whilst > this driver doesn't. > > Should this be a new driver, or a variant of dw9807-vcm? So I did a quick check and there are some interesting differences, like the dw9719 code writing 1 to the CTRL register on resume / powerup while as the dw9807 code writes 0 on resume / powerup. And I have been unable to find a datasheet for either model. This means that both drivers have some black-box aspects, like the powerup differences, to them (both come from Android source dumps I think). This + the small size of these drivers, makes me think that it would be best to just keep this as a separate driver. So for the next standalone (not as part of this series) submission I plan to stick with having a separate driver and I'll try to address all the other review remarks. Regards, Hans
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.
Hi Hans On 05/07/2023 22:29, Hans de Goede wrote: > When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run > sensor->adev is not set yet. > > So if either of the dev_warn() calls about unknown values are hit this > will lead to a NULL pointer deref. > > Set sensor->adev earlier, with a borrowed ref to avoid making unrolling > on errors harder, to fix this. > > Fixes: 485aa3df0dff ("media: ipu3-cio2: Parse sensor orientation and rotation") > Cc: Fabian Wüthrich <me@fabwu.ch> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> And same for the corresponding 09/18 > drivers/media/pci/intel/ipu-bridge.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c > index 62daa8c1f6b1..f0927f80184d 100644 > --- a/drivers/media/pci/intel/ipu-bridge.c > +++ b/drivers/media/pci/intel/ipu-bridge.c > @@ -308,6 +308,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg, > } > > sensor = &bridge->sensors[bridge->n_sensors]; > + /* > + * Borrow our adev ref to the sensor for now, on success > + * acpi_dev_get(adev) is done further below. > + */ > + sensor->adev = adev; > > ret = ipu_bridge_read_acpi_buffer(adev, "SSDB", > &sensor->ssdb,
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 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 7/6/23 11:19, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: > >> Some ACPI glue code (1) may want to do an acpi_device_id match while > >> it only has a struct acpi_device available because the first physical > >> node may not have been instantiated yet. > >> > >> Add a new acpi_match_acpi_device() helper for this, which takes > >> a "struct acpi_device *" as argument rather then the "struct device *" > >> which acpi_match_device() takes. > >> > >> 1) E.g. code which parses ACPI tables to transforms them > >> into more standard kernel data structures like fwnodes > > > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). > > Ah interesting, it does indeed look a lot like your version. > but it was developed independently. > > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp > changes in this series which rely on this are intended for 6.6-rc1 too. No, the material Andy is talking about will be pushed for 6.5-rc1 (probably even today), because it is part of a fix for systems that are broken in the field. > So we still need to figure out how to merge this. This shouldn't be a problem.
Hi, On 7/6/23 15:26, Rafael J. Wysocki wrote: > On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 7/6/23 11:19, Andy Shevchenko wrote: >>> On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: >>>> Some ACPI glue code (1) may want to do an acpi_device_id match while >>>> it only has a struct acpi_device available because the first physical >>>> node may not have been instantiated yet. >>>> >>>> Add a new acpi_match_acpi_device() helper for this, which takes >>>> a "struct acpi_device *" as argument rather then the "struct device *" >>>> which acpi_match_device() takes. >>>> >>>> 1) E.g. code which parses ACPI tables to transforms them >>>> into more standard kernel data structures like fwnodes >>> >>> Looks like it's v1 of my original patch, anyway this is now in Linux Next as >>> 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). >> >> Ah interesting, it does indeed look a lot like your version. >> but it was developed independently. >> >> Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp >> changes in this series which rely on this are intended for 6.6-rc1 too. > > No, the material Andy is talking about will be pushed for 6.5-rc1 > (probably even today), because it is part of a fix for systems that > are broken in the field. > >> So we still need to figure out how to merge this. > > This shouldn't be a problem. Great, thank you. Regards, Hans
On Thu, Jul 06, 2023 at 03:26:20PM +0200, Rafael J. Wysocki wrote: > On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 7/6/23 11:19, Andy Shevchenko wrote: > > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: ... > > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). > > > > Ah interesting, it does indeed look a lot like your version. > > but it was developed independently. > > > > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp > > changes in this series which rely on this are intended for 6.6-rc1 too. > > No, the material Andy is talking about will be pushed for 6.5-rc1 > (probably even today), because it is part of a fix for systems that > are broken in the field. Oh, totally forgot about that. > > So we still need to figure out how to merge this. > > This shouldn't be a problem. True, and thank you!
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.
On Thu, Jul 06, 2023 at 04:34:41PM +0200, Hans de Goede wrote: > On 7/6/23 12:06, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote: ... > >> + usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10); > > > > fsleep() ? > > fsleep would expand to: > > usleep_range(DW9719_CTRL_DELAY_US, 2 * DW9719_CTRL_DELAY_US); > > making the loop take up to twice as long. Is it a problem here? If so, perhaps one line to explain that? > >> + }
Hi All, Here is v3 of my patch-series to make the atomisp code share the ACPI -> sensor fwnode bridge code with the IPU3 (and IPU6 code). This series also rework VCM instantiation, which was my initial reason for unifying / sharing the code. Rafael, this v3 now includes a small ACPI patch: ACPI: bus: Introduce acpi_match_acpi_device() function It is probably easiest if you can give an ack for merging this through the media tree. May we have your ack for this? Sakari, I know that you have other pending patches depending on this, patches 1-14 can be merged independent of the new ACPI patch. Only the atomisp changes require that and those can also be merged later. Changes in v3: - New patches: media: i2c: Add driver for DW9719 VCM ACPI: bus: Introduce acpi_match_acpi_device() function media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation - media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init(): - Add a table with per sensor-HID atomisp_sensor_config settings for sensors which have lanes != 1 or which may have a VCM (VCM support is added in a follow-up patch) - Switch to acpi_handle_err() for logging errors - Set orientation based on CSI link/port This set now consists of the following parts: Patches 1-4 Bugfixes for recent changes Patches 5-12 Cleanup / preparation patches Patch 13 Rework how VCM client instantiation is done so that a device-link can be added from VCM to sensor to fix issues with the VCM power-state being tied to the sensor power state Patch 14 Resubmit DW9719 VCM driver upstream now that Vsio hack is gone Patch 15 New ACPI helper needed by atomisp bridge code Patch 16 Drop ipu-bridge code copy from atomisp, switching to the shared ipu-bridge module Patch 17-18 Further atomisp bridge code improvements Changes in v2: - Rebase on top of f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3") (rebase on top of sailus/media_tree.git/for-6.6-1.4-signed) - Share the ipu_supported_sensors[] array between atomisp and IPU3/IPU6 (leave it in ipu-bridge.c instead of giving each consumer its own copy) - 2 new bugfixes: media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings media: ipu-bridge: Allow building as module Original cover-letter: While working on adding (proper) VCM support to the atomisp code I found myself copying yet more code from drivers/media/pci/intel/ipu3/cio2-bridge.c into the atomisp code. So I decided that it really was time to factor out the common code (most of the code) from intel/ipu3/cio2-bridge.c into its own helper library and then share it between the atomisp and IPU3 code. This will hopefully also be useful for the ongoing work to upstream IPU6 input system support which also needs this functionality and currently contains a 3th copy of this code in the out of tree driver. Regards, Hans Daniel Scally (1): media: i2c: Add driver for DW9719 VCM Hans de Goede (17): media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings media: ipu-bridge: Do not use on stack memory for software_node.name field media: ipu-bridge: Move initialization of node_names.vcm to ipu_bridge_init_swnode_names() media: ipu-bridge: Allow building as module media: ipu-bridge: Make ipu_bridge_init() take a regular struct device as argument media: ipu-bridge: Store dev pointer in struct ipu_bridge media: ipu-bridge: Only keep PLD around while parsing media: ipu-bridge: Add a ipu_bridge_parse_ssdb() helper function media: ipu-bridge: Drop early setting of sensor->adev media: ipu-bridge: Add a parse_sensor_fwnode callback to ipu_bridge_init() media: ipu-bridge: Move ipu-bridge.h to include/media/ media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor ACPI: bus: Introduce acpi_match_acpi_device() function media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init() media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation MAINTAINERS | 7 + drivers/acpi/bus.c | 22 +- drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/dw9719.c | 427 ++++++++++++++++++ drivers/media/pci/intel/Kconfig | 18 +- drivers/media/pci/intel/ipu-bridge.c | 333 ++++++++------ drivers/media/pci/intel/ipu3/Kconfig | 20 + drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +- drivers/staging/media/atomisp/Kconfig | 3 + .../staging/media/atomisp/pci/atomisp_csi2.h | 67 --- .../media/atomisp/pci/atomisp_csi2_bridge.c | 424 ++++++----------- .../staging/media/atomisp/pci/atomisp_v4l2.c | 1 + include/acpi/acpi_bus.h | 2 + .../pci/intel => include/media}/ipu-bridge.h | 27 +- 15 files changed, 851 insertions(+), 522 deletions(-) create mode 100644 drivers/media/i2c/dw9719.c rename {drivers/media/pci/intel => include/media}/ipu-bridge.h (80%)