Re: [PATCH 2/4] media: i2c: Add driver for Sony IMX728

["Sebastian LaVine" <slavine@xxxxxxxxxxxxxx>]

On Wed Feb 12, 2025 at 3:11 PM EST, Krzysztof Kozlowski wrote:
> On 12/02/2025 20:56, Sebastian LaVine wrote:
>>
>> ...
>>
>> +static int imx728_reset(struct imx728 *imx728)
>> +{
>> +
>> +       int ret = 0;
>> +
>> +       // Prefer hardware reset if available.
>> +       if (!IS_ERR_OR_NULL(imx728->reset_gpio)) {
>
> Here can be ERR (although why?) but...
>
>> +               gpiod_set_value_cansleep(imx728->reset_gpio, 1);
>>
>> ...
>>
>> +static int imx728_power_off(struct imx728 *imx728)
>> +{
>> +
>> +       if (imx728->reset_gpio) {
>
> Here cannot.
>
>> +               gpiod_set_value_cansleep(imx728->reset_gpio, 1);
>> +
>> +               usleep_range(1, 10);
>> +       }
>> +       clk_disable_unprepare(imx728->clk);
>> +       return 0;
>> +}
>> +
>>
>> ...
>>
>> +static int imx728_probe(struct i2c_client *client)
>> +{
>> +       struct imx728 *imx728;
>> +       struct v4l2_subdev *sd;
>> +       struct v4l2_ctrl_handler *ctrl_hdr;
>> +       int ret;
>> +
>> +       imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL);
>> +       if (!imx728)
>> +               return -ENOMEM;
>> +
>> +       imx728->dev = &client->dev;
>> +
>> +       imx728->regmap = devm_cci_regmap_init_i2c(client, 16);
>> +       if (IS_ERR(imx728->regmap))
>> +               return PTR_ERR(imx728->regmap);
>> +
>> +       imx728->reset_gpio = devm_gpiod_get_optional(imx728->dev,
>> +                                            "reset", GPIOD_OUT_LOW);
>> +       if (IS_ERR(imx728->reset_gpio))
>> +               return PTR_ERR(imx728->reset_gpio);
>
> So can it be ERR after that point? Looks like not.
>

I see what you mean -- I'll change the check in imx728_reset to a simple
null check in v4. Thanks.

> (Jumping back up to previous inline feedback)
>
> ...
>
>> +
>> +static int imx728_set_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +       struct imx728 *imx728 = to_imx728(sd);
>> +       int ret;
>> +
>> +       mutex_lock(&imx728->lock);
>
> Just use guard. That's actually perfect candidate.
>

Okay -- I'll include this change in v4. I'm not so familiar with using
this interface, so please let me know if this is incorrect:

diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c
index 4a6dfa0a6c58..d7d62e9917a4 100644
--- a/drivers/media/i2c/imx728.c
+++ b/drivers/media/i2c/imx728.c
@@ -9320,17 +9320,16 @@ static int imx728_set_stream(struct v4l2_subdev *sd, int enable)
        struct imx728 *imx728 = to_imx728(sd);
        int ret;

-       mutex_lock(&imx728->lock);
+       guard(mutex)(&imx728->lock);
+
-       if (imx728->streaming == enable) {
+       if (imx728->streaming == enable)
-               mutex_unlock(&imx728->lock);
                return 0;
-       }

        if (enable) {
                ret = pm_runtime_get_sync(imx728->dev);
                if (ret < 0) {
                        pm_runtime_put_noidle(imx728->dev);
-                       goto err_unlock;
+                       goto err;
                }

                ret = imx728_start_stream(imx728);
@@ -9349,15 +9348,12 @@ static int imx728_set_stream(struct v4l2_subdev *sd, int enable)
        __v4l2_ctrl_grab(imx728->ctrl.h_flip, enable);
        __v4l2_ctrl_grab(imx728->ctrl.v_flip, enable);

-       mutex_unlock(&imx728->lock);
-
        return 0;

 err_runtime_put:
        pm_runtime_put(imx728->dev);

-err_unlock:
-       mutex_unlock(&imx728->lock);
+err:
        dev_err(imx728->dev,
                "%s: failed to setup streaming %d\n", __func__, ret);
        return ret;


Thanks,

--
Sebastian

Please be aware that this email includes email addresses outside of the organization.