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 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:
> 
> ...
> 
> > > > + * Author: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> > > > + *         Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> > > 
> > > Single or many?
> > 
> > There are 2 authors for this file
> 
> Do you see the issue now?

Yes, I will change 'Author' to 'Authors'

> ...
> 
> > > > +#define SMB_CORE_CTRL_ESO    0x40
> > > > +#define SMB_CORE_CTRL_FW_ACK 0x10
> > > 
> > > Are they bits or numbers?
> > 
> > These are bits
> 
> Use BIT() then. Ditto for the rest of the bits.

Okay

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

> > > Ditto for the rest similar stuff.
> > 
> > There is no direct correlation between the hex value and time. Configuring
> > registers with these values in driver will set the time in device.
> 
> ...
> 
> > > > +#define I2C_DIR_WRITE                0
> > > > +#define I2C_DIR_READ         1
> 
> https://elixir.bootlin.com/linux/v6.0-rc2/source/include/uapi/linux/i2c.h#L24

I2C_M_RD is used in driver. But the purpose of these MACROs is different.
I2C_DIR_WRITE is used inside both pci1xxxx_i2c_write as well as in
pci1xxxx_i2c_read (for sending slave address). Thus these MACROs are required

> > > Namespace collision. Doesn't I²C core provide these?
> > 
> > I am unable to find any existing MACROs for WRITE and READ in I2C core.
> > Kindly
> > let me know the MACROs
> 
> ...
> 
> > > > +#define PCI1XXXX_I2C_TIMEOUT 1000
> > > 
> > > Units? Same to the rest similar cases.
> > 
> > Unit is milliseconds
> 
> So you know what to do, right?

Yes, I will change MACRO to PCI1XXXX_I2C_TIMEOUT_MS

> ...
> 
> > > > +#define SMBUS_PERI_LOCK              BIT(3)
> > > 
> > > BIT() out of a sudden. See above.
> > 
> > Will use hex value for this like in other places to maintain uniformity
> 
> See above.

Okay. 

> ...
> 
> > > > +     pci1xxxx_i2c_shutdown(i2c);
> > > 
> > > Do you really need this in ->remove()? I would expect something in
> > > the ->suspend() / ->shutdown().
> > 
> > pci1xxxx_i2c_shutdown API will reset the registers that are set as part of
> > pci1xxxx_i2c_init. So, this API is present in ->remove() and not in -
> > >suspend()
> > callback
> 
> I understand that, but it doesn't really answer my question.

I will remove this API in ->remove() callback and add in ->suspend() callback


Thanks,
Tharun Kumar P