Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for I2C host controller in multifunction endpoint of pci1xxxx switch

[<Tharunkumar.Pasumarthi@microchip.com>]

On Fri, 2022-08-26 at 18:37 +0300, Andy Shevchenko wrote:
> 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.

Okay. I will follow this approach.


Thanks,
Tharun Kumar P