Received-SPF: pass (google.com: domain of linux-kernel+bounces-16560-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 4 Jan 2024 12:04:51 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: Elad Nachman <enachman@marvell.com>
Cc: gregory.clement@bootlin.com, linux-i2c@vger.kernel.org, 
	linux-kernel@vger.kernel.org, cyuval@marvell.com
Subject: Re: [PATCH v3 1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock
Message-ID: <qki2w47cfu74j6tuwz363wgsleypbkgkejtzbk7zfktjxlbmqp@h3cejf2gnccb>
References: <20231213151312.1165115-1-enachman@marvell.com>
 <20231213151312.1165115-2-enachman@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231213151312.1165115-2-enachman@marvell.com>

Hi Elad,

first of all it's very nice to see code so well documented :-)

Thanks for the effort you put in it.

...

> +	/*
> +	 * Start with arbitration lost soft reset enabled as to false.
> +	 * Try to locate the necessary items in the device tree to
> +	 * make this feature work.
> +	 * Only after we verify that the device tree contains all of
> +	 * the needed information and that it is sound and usable,
> +	 * then we enable this flag.
> +	 * This information should be defined, but the driver maintains
> +	 * backward compatibility with old dts files, so it will not fail
> +	 * the probe in case these are missing.
> +	 */
> +	drv_data->soft_reset = false;
> +	pc = pinctrl_get(&pd->dev);

I'm not against using devm_pinctrl_get(), in my previous comments
I was questioning whether it should be placed in the probe
function (as you did).

Placed there, iirc, it needed explicit release. Here, I don't
think it does if you use the managed version.

> +	if (!IS_ERR(pc)) {
> +		drv_data->pc = pc;
> +		drv_data->i2c_mpp_state =
> +			pinctrl_lookup_state(pc, "default");
> +		drv_data->i2c_gpio_state =
> +			pinctrl_lookup_state(pc, "gpio");
> +		drv_data->scl_gpio =
> +			of_get_named_gpio(pd->dev.of_node, "scl-gpios", 0);
> +		drv_data->sda_gpio =
> +			of_get_named_gpio(pd->dev.of_node, "sda-gpios", 0);

please use devm_gpio_get(...).

> +		if (!IS_ERR(drv_data->i2c_gpio_state) &&
> +		    !IS_ERR(drv_data->i2c_mpp_state) &&
> +		    gpio_is_valid(drv_data->scl_gpio) &&
> +		    gpio_is_valid(drv_data->sda_gpio)) {

...

> +	if (!IS_ERR(drv_data->pc))
> +		pinctrl_put(drv_data->pc);
> +	if (drv_data->soft_reset) {
> +		devm_gpiod_put(drv_data->adapter.dev.parent,
> +			       gpio_to_desc(drv_data->scl_gpio));
> +		devm_gpiod_put(drv_data->adapter.dev.parent,
> +			       gpio_to_desc(drv_data->sda_gpio));

if it's managed it doesn't need to be explicitely removed.

Thanks,
Andi