Re: [PATCH] i2c: add i2c bus driver for amd navi gpu

[Goswami, Sanket]

Hi Andy,

On 25-Mar-21 22:35, Andy Shevchenko wrote:
> [CAUTION: External Email]
> 
> On Mon, Mar 22, 2021 at 10:26:55PM +0530, Goswami, Sanket wrote:
>> On 09-Mar-21 19:56, Andy Shevchenko wrote:
>>> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
> 
> ...
> 
>>>> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
>>>
>>> Why do you need a custom function for that? Can it be generic and not AMD
>>> specific?
>> This function takes care of AMD Specific bus configuration for the transfer and
>> also addresses the IP issue of i2c transactions hence it is kept as custom.
> 
> Can you a bit elaborate this? IT would be nice to have a comment in the code
> explaining what is the difference with a generic approach.

This will be addressed in v3.
> 
> ...
> 
>>>> +     /* Enable ucsi interrupt */
>>>> +     if (dev->flags & AMD_NON_INTR_MODE)
>>>> +             regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
>>>
>>> This is looks like a hack. Why is it here?
>> In order to enable the interrupt for UCSI i.e. AMD NAVI GPU card,
>> it is mandatory to set the right value in specific register
>> (offset:0x474) as per the hardware IP specification.
> 
> Why it can not be done outside of this function?


This will be addressed in v3.
> 
> ...
> 
>>>> +     if (dev->flags & AMD_NON_INTR_MODE)
>>>> +             return amd_i2c_dw_master_xfer(adap, msgs, num);
>>>
>>> Ditto.
>> Initiate I2C message transfer when AMD NAVI GPU card is enabled,
>> As it is polling based transfer mechanism, which does not support
>> interrupt based functionalities of existing DesignWare driver.
> 
> Ditto.
> 
> And I think I already have told you that I prefer to see rather MODEL_ quirk.

I did not find MODEL_ quirk reference in the PCI device tree, It is actually
used in platform device tree which is completely different from our PCI
based configuration, can you please provide some reference of MODEL_ quirk
which will be part of the PCI device tree?
> 
> ...
> 
>>> Also why (1) and this can't be instantiated from ACPI / DT?
>> It is in line with the existing PCIe-based DesignWare driver,
>> A similar approach is used by the various vendors.
> 
> Here is no answer to the question. What prevents you to fix your ACPI tables or
> DT?
> 
> We already got rid of FIFO hard coded values, timings are harder to achieve,
> but we expect that new firmwares will provide values in the ACPI tables.

AMD NAVI GPU card is the PCI initiated driver, not ACPI initiated, and also
It does not contain a corresponding ACPI match table. Moreover, AMD  NAVI GPU
based products are already in the commercial market hence going by this
approach will break the functionalities for the same.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
Thanks,
Sanket