| Message ID | 20230518153214.194976-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper + gc0310 sensor driver | expand |
On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> wrote: > > gc0310_s_config() used to call camera_sensor_platform_data.csi_cfg() back > when the gc0310 driver was still using the atomisp_gmin_platform code > for power-management. > > Now it is just a weirdly named wrapper around gc0310_detect(), drop > gc0310_s_config() and make probe() call gc0310_detect() directly. ... > - ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); > + ret = pm_runtime_get_sync(&client->dev); > + if (ret >= 0) > + ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); > + pm_runtime_put(&client->dev); > if (ret < 0) { > dev_err(&client->dev, "read sensor_id failed: %d\n", ret); > return -ENODEV; Not sure if it's good to have in this patch, but above can be cleaned up to pm_runtime_get_sync(&client->dev); ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); pm_runtime_put(&client->dev); if (ret < 0) { dev_err(&client->dev, "read sensor_id failed: %d\n", ret); return ret; } But I don't know what will be the response on the I2C bus if the device is powered off.
HI, On 5/19/23 12:32, Andy Shevchenko wrote: > On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> gc0310_s_config() used to call camera_sensor_platform_data.csi_cfg() back >> when the gc0310 driver was still using the atomisp_gmin_platform code >> for power-management. >> >> Now it is just a weirdly named wrapper around gc0310_detect(), drop >> gc0310_s_config() and make probe() call gc0310_detect() directly. > > ... > >> - ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); >> + ret = pm_runtime_get_sync(&client->dev); >> + if (ret >= 0) >> + ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); >> + pm_runtime_put(&client->dev); >> if (ret < 0) { >> dev_err(&client->dev, "read sensor_id failed: %d\n", ret); >> return -ENODEV; > > Not sure if it's good to have in this patch, but above can be cleaned up to > > pm_runtime_get_sync(&client->dev); > ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); > pm_runtime_put(&client->dev); > if (ret < 0) { > dev_err(&client->dev, "read sensor_id failed: %d\n", ret); > return ret; > } > > But I don't know what will be the response on the I2C bus if the > device is powered off. In my experience the i2c bus tends to get stuck when using it with the sensor powered down and unsticking it is tricky (seems to require a full i2c-controller reset). So it is best to not even try if the pm_runtime_get_sync() fails for some reason. Regards, Hans
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c index 35367d6efd2c..486ea7979c57 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c @@ -172,7 +172,10 @@ static int gc0310_detect(struct i2c_client *client) if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) return -ENODEV; - ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); + ret = pm_runtime_get_sync(&client->dev); + if (ret >= 0) + ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H); + pm_runtime_put(&client->dev); if (ret < 0) { dev_err(&client->dev, "read sensor_id failed: %d\n", ret); return -ENODEV; @@ -261,19 +264,6 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable) return ret; } -static int gc0310_s_config(struct v4l2_subdev *sd) -{ - struct i2c_client *client = v4l2_get_subdevdata(sd); - int ret; - - ret = pm_runtime_get_sync(&client->dev); - if (ret >= 0) - ret = gc0310_detect(client); - - pm_runtime_put(&client->dev); - return ret; -} - static int gc0310_g_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *interval) { @@ -403,7 +393,7 @@ static int gc0310_probe(struct i2c_client *client) pm_runtime_set_autosuspend_delay(&client->dev, 1000); pm_runtime_use_autosuspend(&client->dev); - ret = gc0310_s_config(&dev->sd); + ret = gc0310_detect(client); if (ret) { gc0310_remove(client); return ret;
gc0310_s_config() used to call camera_sensor_platform_data.csi_cfg() back when the gc0310 driver was still using the atomisp_gmin_platform code for power-management. Now it is just a weirdly named wrapper around gc0310_detect(), drop gc0310_s_config() and make probe() call gc0310_detect() directly. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../media/atomisp/i2c/atomisp-gc0310.c | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-)