Re: [PATCH v3] i2c: hisi: Add I2C bus recovery support

[Jie Zhan]

On 9/17/2026 10:39 AM, yubowen (H) wrote:
[ ... ]
>>> +static int hisi_i2c_recovery_init(struct hisi_i2c_controller *ctlr)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(ctlr->dev);
>>> +	struct i2c_adapter *adapter = &ctlr->adapter;
>>> +	int ret;
>>> +
>>> +	if (acpi_disabled)
>>> +		return -ENODEV;
>> Is #ifdef CONFIG_ACPI still useful if 'acpi_disabled' is checked?
> Thanks for the review, Jie.
> 
> Both guards are needed; they operate at different layers and cannot replace each other.
> 
> - #ifdef CONFIG_ACPI is a compile-time guard to prevent errors caused by functions
> 
>     like acpi_has_method
> 
> - acpi_disabled() is a runtime check, it is 1 by default and set to 0 when the platform
> 
>     boots via acpi.
if not CONFIG_ACPI, acpi_disabled would be 1.
> 
> Other comments will be fixed in v4.
> 
> 
> Thanks
> 
> Bowen
> 
>>> +
>>> +	ret = hisi_i2c_get_bus_recovery_res(ctlr, pdev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ctlr->rinfo = (struct i2c_bus_recovery_info){
>> Neat: (struct i2c_bus_recovery_info) {
>> Good to add a space.
>>> +		.get_scl = hisi_i2c_recovery_get_scl,
>>> +		.get_sda = hisi_i2c_recovery_get_sda,
>>> +		.set_scl = hisi_i2c_recovery_set_scl,
>>> +		.prepare_recovery = hisi_i2c_prepare_recovery,
>>> +		.unprepare_recovery = hisi_i2c_unprepare_recovery,
>>> +		.recover_bus = i2c_generic_scl_recovery,
>>> +	};
>>> +	adapter->bus_recovery_info = &ctlr->rinfo;
>>> +	ctlr->acpi_handle = ACPI_HANDLE(ctlr->dev);
>> Neat: a blank line before return 0.
>>> +	return 0;
>>> +}
>>> +#else
>>> +static int hisi_i2c_recovery_init(struct hisi_i2c_controller *ctlr) { return -ENODEV; }
>>> +#endif /* CONFIG_ACPI */
>>> +
>>>  static int hisi_i2c_start_xfer(struct hisi_i2c_controller *ctlr)
>>>  {
>>>  	struct i2c_msg *msg = ctlr->msgs;
>>> @@ -504,6 +645,10 @@ static int hisi_i2c_probe(struct platform_device *pdev)
>>>  	adapter->dev.parent = dev;
>>>  	i2c_set_adapdata(adapter, ctlr);
>>>  
>>> +	ret = hisi_i2c_recovery_init(ctlr);
>>> +	if (ret)
>>> +		dev_info(ctlr->dev, "I2C bus recovery not available\n");
>>> +
>> Move this before setting 'adapter' and i2c_set_adapdata()?
>>
>> Recovery init should be done before the final preparation of 'adapter'.
>>>  	ret = devm_i2c_add_adapter(dev, adapter);
>>>  	if (ret)
>>>  		return ret;