Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: t+86kMvNcOsyW1owOM/S5oHoVYgrbN4VF6cFc4lrjMaV+RGGLA4nRppnV6llvtad93J9e/LOKh
 nV3fkzVjuLgF//zNQ+/ORoHE9SLdf1VruxA6eXBi2MwXchNSAgrLDbLNPh+iHLEza2kNWKDrfv
 0Ep7bk7oAevNYcNS9NoE3Q7NyWaJIUoJUI/nApbGdKn2ARkPA+z7/l68XjjIUaYWEEXlNwi62J
 wFCNyJTElX04ZbfvUArebCrPSs37W05WGR00NaYjITp1zHyTqh7vtyCcdkSmWjX5+BrBCs2+Pw
 pb8=
From:   <Codrin.Ciubotariu@microchip.com>
To:     <wsa@kernel.org>
CC:     <linux-i2c@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <robh+dt@kernel.org>,
        <Ludovic.Desroches@microchip.com>, <Nicolas.Ferre@microchip.com>,
        <alexandre.belloni@bootlin.com>, <linux@armlinux.org.uk>,
        <kamel.bouhara@bootlin.com>
Subject: Re: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring
 SCL/SDA GPIOs
Thread-Topic: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring
 SCL/SDA GPIOs
Thread-Index: AQHWRkTnXY0butV020yTUidEL/axqqklUZIAgAF4w4A=
Date:   Mon, 3 Aug 2020 15:33:41 +0000
Message-ID: <15a449fa-d649-846a-e6f2-1540f9581846@microchip.com>
References: <20200619141904.910889-1-codrin.ciubotariu@microchip.com>
 <20200619141904.910889-4-codrin.ciubotariu@microchip.com>
 <20200802170500.GB10193@kunai>
In-Reply-To: <20200802170500.GB10193@kunai>
Accept-Language: en-US
Content-Language: en-US
user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
Content-Type: text/plain; charset="Windows-1252"
Content-ID: <35F793B60A5DE54AB90ADAB6C9FCEB5A@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e70a0a5-d500-4095-c229-08d837c29a0e
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Aug 2020 15:33:41.9394
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ior3wEQ6sR4Ssw2yVN4/v1IByjN6KiSeLILS0n5PIi4GFYIzFusjU1Ql3hMdpBIatOw8j/sr8DQ1laxHG2ZG1LWOOInUMz+mo9hvTSgDG24=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4765
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 02.08.2020 20:05, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
>> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
>> -EPROBE_DEFER, so we should at least treat that. This ends up with
>> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> ---
>>   drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 4ee29fec4e93..f8d9f2048ca8 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapt=
er *adap)
>>   	return i2c_gpio_init_generic_recovery(adap);
>>   }
>>  =20
>> -static void i2c_init_recovery(struct i2c_adapter *adap)
>> +static int i2c_init_recovery(struct i2c_adapter *adap)
>>   {
>>   	struct i2c_bus_recovery_info *bri =3D adap->bus_recovery_info;
>>   	char *err_str;
>>  =20
>>   	if (!bri)
>> -		return;
>> +		return 0;
>>  =20
>> -	i2c_gpio_init_recovery(adap);
>> +	if (i2c_gpio_init_recovery(adap) =3D=3D -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>>  =20
>>   	if (!bri->recover_bus) {
>>   		err_str =3D "no recover_bus() found";
>> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *ad=
ap)
>>   			if (gpiod_get_direction(bri->sda_gpiod) =3D=3D 0)
>>   				bri->set_sda =3D set_sda_gpio_value;
>>   		}
>> -		return;
>> +		return 0;
>=20
> This is correct but I think the code flow is/was confusing. Can you drop
> this 'return' and use 'else if' for the next code block? I think this is
> more readable.

Ok, it makes sense. Should I make a separate patch for this only?
One more question, should we keep:
if (!bri->set_sda && !bri->get_sda) {
	err_str =3D "either get_sda() or set_sda() needed";
	goto err;
}
?
Without {get/set}_sda we won't be able to generate stop commands and=20
possibly check if the bus is free, but we can still generate the SCL=20
clock pulses.

>=20
>>   	}
>>  =20
>>   	if (bri->recover_bus =3D=3D i2c_generic_scl_recovery) {
>> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *=
adap)
>>   		}
>>   	}
>>  =20
>> -	return;
>> +	return 0;
>>    err:
>>   	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
>>   	adap->bus_recovery_info =3D NULL;
>> +
>> +	return 0;
>=20
> 'return -EINVAL;' I'd suggest.

OK

>=20
>>   }
>>  =20
>>   static int i2c_smbus_host_notify_to_irq(const struct i2c_client *clien=
t)
>> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter=
 *adap)
>>   			 "Failed to create compatibility class link\n");
>>   #endif
>>  =20
>> -	i2c_init_recovery(adap);
>> +	res =3D i2c_init_recovery(adap);
>> +	if (res =3D=3D -EPROBE_DEFER)
>> +		goto out_link;
>=20
> Please move 'i2c_init_recovery' above the class-link creation. It
> shouldn't make a difference but we can skip the extra label and the
> ifdeffery.

Ok. Perhaps I should also move the debug print with the registered=20
adapter after calling i2c_init_recovery().

>=20
>>  =20
>>   	/* create pre-declared device nodes */
>>   	of_i2c_register_devices(adap);
>> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapte=
r *adap)
>>  =20
>>   	return 0;
>>  =20
>> +out_link:
>> +#ifdef CONFIG_I2C_COMPAT
>> +	class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
>> +				 adap->dev.parent);
>> +#endif
>>   out_reg:
>>   	init_completion(&adap->dev_released);
>>   	device_unregister(&adap->dev);
>> --=20
>> 2.25.1
>>

Do you want me to integrate this patch in the previous one?

Best regards,
Codrin