Re: [PATCH v7 2/5] media: i2c: imx355: Support devicetree and power management

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/01/2026 08:36, Sakari Ailus wrote:
> On Mon, Jan 19, 2026 at 10:50:41PM -0500, Richard Acayan wrote:
>> On Sat, Jan 17, 2026 at 02:03:02PM +0200, Vladimir Zapolskiy wrote:
>>> On 1/17/26 06:06, Richard Acayan wrote:
>> (snip)
>>>> +static int imx355_power_on(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> +	struct imx355 *imx355 = to_imx355(sd);
>>>> +	int ret;
>>>> +
>>>> +	ret = clk_prepare_enable(imx355->clk);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "failed to enable clocks: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355_supplies),
>>>> +				    imx355->supplies);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
>>>> +		goto error_disable_clocks;
>>>> +	}
>>>> +
>>>> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
>>>> +	usleep_range(1000, 2000);
>>>
>>> The deassert above is not needed IMO, anyway.
>>
>> This assert is for clarity, otherwise it isn't obvious that the GPIO is
>> asserted low when the function is called. It should stay.
> 
> I'd also remove it: it's redundant.
> 

Not only redundant but confusing - function is called power_on, so why
does it try to perform power off/reset first? If it was called reset
then maybe argument of code readability would stay, but here not really,
especially that there is a power off function...


Best regards,
Krzysztof