Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220823145603.2606475-1-tharunkumar.pasumarthi@microchip.com>
 <YwTsnCdxM5I6BoJu@smile.fi.intel.com> <80debd4bbf819cbfa16681172919436df476558a.camel@microchip.com>
 <CAHp75VevAifCp_Bz5HN1MmGm7UEJWFR017kngzARfBh+Z8aAAA@mail.gmail.com>
 <787644272b3b260980ff8b9efc317ce3194b0acc.camel@microchip.com>
 <YweFttaSE2GOoW83@smile.fi.intel.com> <8f9e8be7084ad78671198f03313058809038a3f8.camel@microchip.com>
 <a1d8278eb7e94c26de1022eec480a22fced120f0.camel@microchip.com>
In-Reply-To: <a1d8278eb7e94c26de1022eec480a22fced120f0.camel@microchip.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 26 Aug 2022 18:37:06 +0300
Message-ID: <CAHp75VcUKeRhh40dB4Lqh=TE93GhfzaBJ-3uDWrJedDTGLa5vQ@mail.gmail.com>
Subject: Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for
 I2C host controller in multifunction endpoint of pci1xxxx switch
To:     Tharunkumar.Pasumarthi@microchip.com
Cc:     Wolfram Sang <wsa@kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, Sven Peter <sven@svenpeter.dev>,
        Rob Herring <robh@kernel.org>, Jan Dabros <jsd@semihalf.com>,
        Olof Johansson <olof@lixom.net>,
        Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Sam Protsenko <semen.protsenko@linaro.org>,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-i2c <linux-i2c@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Aug 26, 2022 at 4:04 PM <Tharunkumar.Pasumarthi@microchip.com> wrote:
> On Fri, 2022-08-26 at 04:00 +0000, Tharunkumar Pasumarthi - I67821 wrote:
> > On Thu, 2022-08-25 at 17:22 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 25, 2022 at 01:15:42PM +0000,
> > > Tharunkumar.Pasumarthi@microchip.com wrote:
> > > > On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
> > > > > On Wed, Aug 24, 2022 at 6:04 PM <Tharunkumar.Pasumarthi@microchip.com>
> > > > > wrote:
> > > > > > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > > > > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:

...

> > > > > > > > +#define SMB_IDLE_SCALING_100KHZ              0x03E803C9
> > > > > > > > +#define SMB_IDLE_SCALING_400KHZ              0x01F4009D
> > > > > > > > +#define SMB_IDLE_SCALING_1000KHZ     0x01F4009D
> > > > > > >
> > > > > > > Shouldn't these magics be decimals?
> > > > >
> > > > > This Q seems unanswered.
> > > >
> > > > These magic numbers need not be decimals. Configuring registers with these
> > > > values in driver will set the time in device. However, these values do not
> > > > convey any meaning when represented in decimals.
> > >
> > > Hmm... Maybe you don't see this, but I see the following:
> > >
> > > 0x03E803C9 = 65536 (i.e. 2^16) * 1000 + 969
> > > 0x01F4009D = 32768 (i.e. 2^15) * 1000 + 157
> > >
> > > Pretty much sounds like a bit 15 for standard mode and bit 16 for fast modes
> > > shifted by 1000 to have a room for the time in presumably nanoseconds up to
> > > 1
> > > us.
> > >
> > > Please, dig up into the documentation, vendor chat, etc to get more
> > > information
> > > on these values.
> >
>
> I have went through the documentation.
>
> Following is the bit mapping of idle scaling register:
> Reserved [31:28]
> Fair Idle Delay [27:16]
> Reserved [15:12]
> Bus Idle Min [11:0]
>
> 'Bus Idle Min' field will indicate the number of ticks of the baud clock
> required to program 'bus idle period' delay and can have maximum value of 4095.
> 'Fair Idle Min' field will indicate the number of ticks of the baud clock
> required to program 'fair idle' delay and can have maximum value of 4095.
>
> So, either the entire IDLE_SCALING_REG value can be in hex or I could do
> something like below:

No hex.

> #define BUS_IDLE_MIN_TICKS     <VALUE_IN_DECIMAL>
> #define FAIR_IDLE_DELAY_TICKS  <VALUE_IN_DECIMAL>
>
> #define IDLE_SCALING_REG ((FAIR_IDLE_DELAY_TICKS << 16) | BUS_IDLE_MIN_TICKS)
>
> Which of these 2 approaches do you feel is better?

Of course one with the comment explaining the thing and two decimal numbers.
Now, since we know that both values are

1000 and 969
500 and 157

It's easy to see the difference and meaning.

So per each mode you need to have those pairs of decimal numbers in ticks.
and one comment explaining all what you have explained here in this mail.

> > Sure, I will go through the documentation. Thanks.
> > Also, I will modify these magic numbers to decimals.

-- 
With Best Regards,
Andy Shevchenko