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: H1hyMxyU12hAkVcY5UikepL7X11tUvG5PlKPNE1+gOSlx74Os4dB72dCwuUrCX57ehnmq6vg+j
 u03e0kKJWnZQ==
IronPort-SDR: 0zVs3Ixl4PvLhsM1rLl1jNqPu0t1LvpjEpw89hXqedJG7yo8e2ebDIuA2SlnbQspll9Zm8Mo4+
 AZpL8EYBXtnQ==
Date:   Thu, 25 Mar 2021 19:05:27 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     "Goswami, Sanket" <Sanket.Goswami@amd.com>
Cc:     jarkko.nikula@linux.intel.com, mika.westerberg@linux.intel.com,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        Shyam Sundar S K <Shyam-sundar.S-k@amd.com>,
        Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
Subject: Re: [PATCH] i2c: add i2c bus driver for amd navi gpu
Message-ID: <YFzC19IiGZdmLCOR@smile.fi.intel.com>
References: <20210309133147.1042775-1-Sanket.Goswami@amd.com>
 <YEeFgZSIY5lb2ubP@smile.fi.intel.com>
 <fa1a59fb-a7fa-44bb-1629-5e726f164b94@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fa1a59fb-a7fa-44bb-1629-5e726f164b94@amd.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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.

...

> >> +     /* 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?

...

> >> +     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.

...

> > 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.

-- 
With Best Regards,
Andy Shevchenko