Re: [PATCH 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Andrey Konovalov <andrey.konovalov@linaro.org>]

Hi Ezequiel,

On 19.12.2019 18:53, Ezequiel Garcia wrote:
> On Thu, 2019-12-19 at 01:29 +0300, Andrey Konovalov wrote:
>> Hi Ezequiel,
>>
>> Thank you for reviewing the patch!
>>
>>
> [..]
>>>> +/* Stop streaming */
>>>> +static int imx219_stop_streaming(struct imx219 *imx219)
>>>> +{
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>>>> +	int ret;
>>>> +
>>>> +	/* set stream off register */
>>>> +	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
>>>> +			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
>>>> +	if (ret)
>>>> +		dev_err(&client->dev, "%s failed to set stream\n", __func__);
>>>> +
>>>> +	/*
>>>> +	 * Return success even if it was an error, as there is nothing the
>>>> +	 * caller can do about it.
>>>> +	 */
>>>
>>> Just change this function return to void, instead?
>>
>> Maybe something like that (functionally the same, but probably more self-explaining):
>>
>> -----8<-----
>> @@ -798,11 +796,7 @@ static int imx219_stop_streaming(struct imx219 *imx219)
> 
> I don't know if I'm missing something, but why can't we have:
> 
> static void imx219_stop_streaming(struct imx219 *imx219) ?
> 
> Since the return value is not used anywhere, it doesn't make sense
> to return it.

OK. Will fix it in v2.

>>           if (ret)
>>                   dev_err(&client->dev, "%s failed to set stream\n", __func__);
>>
>> -       /*
>> -        * Return success even if it was an error, as there is nothing the
>> -        * caller can do about it.
>> -        */
>> -       return 0;
>> +       return ret;
>>    }
>>
>>    static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>> @@ -832,7 +826,7 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>>                   if (ret)
>>                           goto err_rpm_put;
>>           } else {
>> -               imx219_stop_streaming(imx219);
>> +               (void)imx219_stop_streaming(imx219);
>>                   pm_runtime_put(&client->dev);
>>           }
>> -----8<-----
>>
>> ?
>>
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>>>> +{
>>>> +	struct imx219 *imx219 = to_imx219(sd);
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> +	int ret = 0;
>>>> +
>>>> +	mutex_lock(&imx219->mutex);
>>>> +	if (imx219->streaming == enable) {
>>>> +		mutex_unlock(&imx219->mutex);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	if (enable) {
>>>> +		ret = pm_runtime_get_sync(&client->dev);
>>>> +		if (ret < 0) {
>>>> +			pm_runtime_put_noidle(&client->dev);
>>>> +			goto err_unlock;
>>>> +		}
>>>> +
>>>> +		/*
>>>> +		 * Apply default & customized values
>>>> +		 * and then start streaming.
>>>> +		 */
>>>> +		ret = imx219_start_streaming(imx219);
>>>> +		if (ret)
>>>> +			goto err_rpm_put;
>>>> +	} else {
>>>> +		imx219_stop_streaming(imx219);
>>>> +		pm_runtime_put(&client->dev);
>>>> +	}
>>>> +
>>>> +	imx219->streaming = enable;
>>>> +
>>>> +	/* vflip and hflip cannot change during streaming */
>>>> +	__v4l2_ctrl_grab(imx219->vflip, enable);
>>>> +	__v4l2_ctrl_grab(imx219->hflip, enable);
>>>> +
>>>> +	mutex_unlock(&imx219->mutex);
>>>> +
>>>> +	return ret;
>>>> +
>>>> +err_rpm_put:
>>>> +	pm_runtime_put(&client->dev);
>>>> +err_unlock:
>>>> +	mutex_unlock(&imx219->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +/* Power/clock management functions */
>>>> +static int imx219_power_on(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> +	struct imx219 *imx219 = to_imx219(sd);
>>>> +	int ret;
>>>> +
>>>> +	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
>>>> +				    imx219->supplies);
>>>> +	if (ret) {
>>>> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
>>>> +			__func__);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(imx219->xclk);
>>>> +	if (ret) {
>>>> +		dev_err(&client->dev, "%s: failed to enable clock\n",
>>>> +			__func__);
>>>> +		goto reg_off;
>>>> +	}
>>>> +
>>>> +	gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
>>>> +	msleep(IMX219_XCLR_DELAY_MS);
>>>> +
>>>> +	return 0;
>>>> +reg_off:
>>>> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int imx219_power_off(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> +	struct imx219 *imx219 = to_imx219(sd);
>>>> +
>>>> +	gpiod_set_value_cansleep(imx219->xclr_gpio, 0);
>>>> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
>>>> +	clk_disable_unprepare(imx219->xclk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused imx219_suspend(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> +	struct imx219 *imx219 = to_imx219(sd);
>>>> +
>>>> +	if (imx219->streaming)
>>>> +		imx219_stop_streaming(imx219);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused imx219_resume(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> +	struct imx219 *imx219 = to_imx219(sd);
>>>> +	int ret;
>>>> +
>>>> +	if (imx219->streaming) {
>>>> +		ret = imx219_start_streaming(imx219);
>>>> +		if (ret)
>>>> +			goto error;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +
>>>> +error:
>>>> +	imx219_stop_streaming(imx219);
>>>> +	imx219->streaming = 0;
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int imx219_get_regulators(struct imx219 *imx219)
>>>> +{
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < IMX219_NUM_SUPPLIES; i++)
>>>> +		imx219->supplies[i].supply = imx219_supply_name[i];
>>>> +
>>>> +	return devm_regulator_bulk_get(&client->dev,
>>>> +				       IMX219_NUM_SUPPLIES,
>>>> +				       imx219->supplies);
>>>> +}
>>>> +
>>>> +/* Verify chip ID */
>>>> +static int imx219_identify_module(struct imx219 *imx219)
>>>> +{
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>>>> +	int ret;
>>>> +	u32 val;
>>>> +
>>>> +	ret = imx219_power_on(imx219->dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = imx219_read_reg(imx219, IMX219_REG_CHIP_ID,
>>>> +			      IMX219_REG_VALUE_16BIT, &val);
>>>> +	if (ret) {
>>>> +		dev_err(&client->dev, "failed to read chip id %x\n",
>>>> +			IMX219_CHIP_ID);
>>>> +		goto power_off;
>>>> +	}
>>>> +
>>>> +	if (val != IMX219_CHIP_ID) {
>>>> +		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
>>>> +			IMX219_CHIP_ID, val);
>>>> +		ret = -EIO;
>>>> +	}
>>>> +
>>>
>>> I wonder if this is not a bit obscure: it's not obvious
>>> from a first read that the device is left powered on successful
>>> identification.
>>>
>>> Perhaps you can have:
>>>
>>>       return 0;
>>>
>>> And then goto err_power_off on the error paths.
>>> This way, it's clear that powering off is only
>>> to be done on error.
>>
>> OK. Makes sense. Will fix in v2.
>>
>>> OTOH, why do we need to leave the device powered on probe?
>>
>> Let me try what happens if I leave powering the sensor on and off
>> to dev_pm_ops. (Seems like it *should* work in theory, but maybe
>> there were some hidden problems.)
>> (Also I would only be able to check if the sensor works or not - can't
>> do the electrical signals or power consumption measurements etc.)
>>
>> Anyway, leaving the sensor powered on shouldn't hurt, as the sensor
>> is kept in lower power mode when it is not streaming (MIPI signals
>> are passive - the indication on that is the "clock-noncontinuous"
>> property in the DT bindings; also there is no code in the driver
>> to change that, then the imx219 sensor must always "turn off"
>> MIPI lines when it is not streaming - with the reg setting currently
>> used at least).
>>
> 
> Right, the sensor being in LP state is a good point. IMHO, it's
> totally fine is the sensor needs to be powered. It would be
> nice to have a nice comment, if only for resource tracking reasons.

I've tested this part of the driver, and the driver does power off the
sensor before leaving probe(). And afterwords it powers the sensor on during
streaming only. There is just the error path in probe() which needs to be fixed.
I'll also rearrange the code a little bit, and it should become more straightforward
and easier to follow.

> Thanks,
> Ezequiel

Thanks,
Andrey