On 4/3/24 11:03, Ondřej Jirman wrote: > Hi, > > On Wed, Apr 03, 2024 at 04:28:59PM GMT, Sakari Ailus wrote: >> Hi Luis, >> >> Could you unify the subject prefix for the driver patches, please? E.g. >> "media: imx258: " would be fine. >> >> On Wed, Apr 03, 2024 at 09:03:53AM -0600, git@xxxxxxxxxxxx wrote: >>> From: Luis Garcia <git@xxxxxxxxxxxx> >>> >>> It was documented in DT, but not implemented. >>> >>> Signed-off-by: Ondrej Jirman <megous@xxxxxxxxxx> >>> Signed-off-by: Luis Garcia <git@xxxxxxxxxxxx> >>> --- >>> drivers/media/i2c/imx258.c | 14 +++++++++++++- >>> 1 file changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >>> index 163f04f6f954..4c117c4829f1 100644 >>> --- a/drivers/media/i2c/imx258.c >>> +++ b/drivers/media/i2c/imx258.c >>> @@ -680,6 +680,7 @@ struct imx258 { >>> unsigned int csi2_flags; >>> >>> struct gpio_desc *powerdown_gpio; >>> + struct gpio_desc *reset_gpio; >>> >>> /* >>> * Mutex for serialized access: >>> @@ -1232,7 +1233,11 @@ static int imx258_power_on(struct device *dev) >>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies); >>> } >>> >>> - return ret; >>> + gpiod_set_value_cansleep(imx258->reset_gpio, 0); >>> + >>> + usleep_range(400, 500); >> >> You could mention this at least in the commit message. > > This is T6 in the datasheet: https://megous.com/dl/tmp/92c9223ce877216e.png > > >>> + >>> + return 0; >>> } >>> >>> static int imx258_power_off(struct device *dev) >>> @@ -1243,6 +1248,7 @@ static int imx258_power_off(struct device *dev) >>> clk_disable_unprepare(imx258->clk); >>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies); >>> >>> + gpiod_set_value_cansleep(imx258->reset_gpio, 1); >> >> Same question than on the other GPIO: does this belong here? > > No, this should be before the regulator_bulk_disable. > > See: https://megous.com/dl/tmp/c96180b23d7ce63a.png > > kind regards, > o. > Since I'm supposed to move the reset up should I also move the power up with it to match your downstream driver? >>> gpiod_set_value_cansleep(imx258->powerdown_gpio, 1); >>> >>> return 0; >>> @@ -1554,6 +1560,12 @@ static int imx258_probe(struct i2c_client *client) >>> if (IS_ERR(imx258->powerdown_gpio)) >>> return PTR_ERR(imx258->powerdown_gpio); >>> >>> + /* request optional reset pin */ >>> + imx258->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", >>> + GPIOD_OUT_HIGH); >>> + if (IS_ERR(imx258->reset_gpio)) >>> + return PTR_ERR(imx258->reset_gpio); >>> + >>> /* Initialize subdev */ >>> v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops); >>> >> >> -- >> Regards, >> >> Sakari Ailus