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
