Re: [PATCH v3 1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock

[Andi Shyti]

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