Re: [PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 9/18/2024 15:33, Andy Shevchenko wrote:
> On Wed, Sep 18, 2024 at 12:01:19AM +0530, Shyam Sundar S K wrote:
>> On 9/14/2024 00:49, Andy Shevchenko wrote:
>>> On Fri, Sep 13, 2024 at 05:41:09PM +0530, Shyam Sundar S K wrote:
> 
> ...
> 
>>>> +	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!eoi_addr)
>>>> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
>>>> +
>>>> +	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
>>>> +	if (!asf_dev->eoi_base)
>>>> +		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");
>>>
>>> Home grown copy of devm_platform_ioremap_resource().
>>
>> devm_platform_ioremap_resource() internally calls
>> devm_platform_get_and_ioremap_resource(), performing two main actions:
>>
>> It uses platform_get_resource().
>> It then calls devm_ioremap_resource().
>>
>> However, there's an issue.
>>
>> devm_ioremap_resource() invokes devm_request_mem_region() followed by
>> __devm_ioremap(). In this driver, the resource obtained via ASL might
>> not actually belong to the ASF device address space. Instead, it could
>> be within other IP blocks of the ASIC, which are crucial for
>> generating subsequent interrupts (the main focus of this patch). As a
>> result, devm_request_mem_region() fails, preventing __devm_ioremap()
>> from being executed.
>>
>> TL;DR, it’s more appropriate to call platform_get_resource() and
>> devm_ioremap() separately in this scenario.
> 
> Okay, at bare minimum this must be commented in the code (like the above
> summary). 

Okay, I will leave a comment.

> Ideally it should be done differently as the resource regions
> should not be shared (it's an exceptional case and usually shows bad design
> of the driver). If you can't really split, regmap APIs help with that
> (and they also may provide an adequate serialisation to IO).
> 

Unfortunately, this is the only way to get subsequent interrupts from
the ASF IP block (based on the AMD ASF databook).

Thanks,
Shyam