Re: [PATCH] media: imx335: Fix reset-gpio handling

[Umang Jain <umang.jain@ideasonboard.com>]

Hi Jacopo

On 29/07/24 1:42 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Mon, Jul 29, 2024 at 11:35:35AM GMT, Umang Jain wrote:
>> The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.
> How is this related to this change ? The value to which the GPIO is
> initialized to in probe is the physical level.
>
> What matters is the gpio line active level, which should be described
> in the sensor's datasheet. What's the active level of the reset gpio
> line ?

The XCLR active level is "Low" at the init time. It is set to "high" 
during power-on / normal operation

>
>> However, the reset-gpio logical value is set to 1 in during power-on
>> and to 0 on power-off. This is incorrect as the reset line
>> cannot be high during power-on and low during power-off.
> If the line is physically high or low only depends on how the active
> level is specified in DTS, not by the logical value provided to
> gpiod_set_value[_cansleep]()

True.

AS far as I can see, the DT binding schema specifies 'reset-gpios:' - 
without the active level

The active level is I suppose, intentionally left to the DT implementation ?

>> Rectify the logical value of reset-gpio so that it is set to
>> 0 during power-on and to 1 during power-off.
> This is correct, the reset line should be set to logical 0 (inactive)
> during power on and to logical 1 (active) when powering off. However
> the GPIO active state should have been specified in bindings and as
> this driver has been mainline quite some time, this change will break
> .dtbo already used succesfully with previous kernel releases.
>
> Is this an issue ?

Yes, if the patch is accepted, the Device-tree implementation for IMX335 
will need to be adjusted accordingly. This can be an issue definitely - 
but on the other hand, this attempts to rectify a mistake, no?

>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/media/i2c/imx335.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>> index cd150606a8a9..878d88b5f476 100644
>> --- a/drivers/media/i2c/imx335.c
>> +++ b/drivers/media/i2c/imx335.c
>> @@ -1171,7 +1171,7 @@ static int imx335_power_on(struct device *dev)
>>   	usleep_range(500, 550); /* Tlow */
>>
>>   	/* Set XCLR */
>> -	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
>> +	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
>>
>>   	ret = clk_prepare_enable(imx335->inclk);
>>   	if (ret) {
>> @@ -1184,7 +1184,7 @@ static int imx335_power_on(struct device *dev)
>>   	return 0;
>>
>>   error_reset:
>> -	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
>> +	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
>>   	regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
>>
>>   	return ret;
>> @@ -1201,7 +1201,7 @@ static int imx335_power_off(struct device *dev)
>>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>   	struct imx335 *imx335 = to_imx335(sd);
>>
>> -	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
>> +	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
>>   	clk_disable_unprepare(imx335->inclk);
>>   	regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
>>
>> --
>> 2.45.0
>>
>>