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 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:
> > Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> > Industrial and Automotive applications. This switch has multiple
> > downstream ports. In one of the Switch's Downstream port, there
> > is a multifunction endpoint for peripherals which includes an I2C
> > host controller. The I2C function in the endpoint operates at 100KHz,
> > 400KHz and 1 MHz and has buffer depth of 128 bytes.
> > This patch provides the I2C controller driver for the I2C endpoint
> > of the switch.
> 
> ...
> 
> > @@ -1290,6 +1290,16 @@ config I2C_VIPERBOARD
> >         River Tech's viperboard.h for detailed meaning
> >         of the module parameters.
> > 
> > +config I2C_PCI1XXXX
> 
> Looks unsorted.

Okay, I will sort in alphabetical order

> > +     tristate "PCI1XXXX I2C Host Adapter support"
> > +     depends on PCI
> > +     help
> > +       Say yes here to enable the I2C Host adapter support for the PCI1xxxx
> > card
> > +       This is a PCI to I2C adapter
> > +
> > +       This driver can be built as a module. If so, the module will be
> > +       called as i2c-mchp-pci1xxxx
> 
> English grammar and punctuation while keeping lines shorter (~76) please.

Okay

> 
> >  obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o
> > +obj-$(CONFIG_I2C_PCI1XXXX)   += i2c-mchp-pci1xxxx.o
> 
> Why unsorted?

I will sort in alphabetical order

> 
> > + * Author: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> > + *         Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Single or many?

There are 2 authors for this file

> 
> > +/*SMB register space*/
> 
> Style.

I will take care of styling for comments throughout file

> 
> > +#define SMB_CORE_CTRL_ESO    0x40
> > +#define SMB_CORE_CTRL_FW_ACK 0x10
> 
> Are they bits or numbers?

These are bits

> ...
> 
> > +#define SMB_CORE_CMD_READM           0x10
> > +#define SMB_CORE_CMD_STOP            0x04
> > +#define SMB_CORE_CMD_START           0x01
> 
> Ditto.

These are bits

> ...
> 
> > +#define SMB_CORE_CMD_M_PROCEED       0x02
> > +#define SMB_CORE_CMD_M_RUN           0x01
> 
> Ditto.

These are bits

> ...
> 
> > +#define SR_HOLD_TIME_100KHZ          0x85
> > +#define SR_HOLD_TIME_400KHZ          0x14
> > +#define SR_HOLD_TIME_1000KHZ 0x0B
> 
> These has to be decimal, and why the ACPI / DT does not provide them?
> 
> Also, do they have units or are they proportional coefficients?

There is no direct correlation between the hex value and time. Ex: 0x85
represents 0.5 us. Our device has an OTP region using which user can configure
device to operate at 100KHz, 400KHz and 1MHz. Based on this configuration,
SR_HOLD_TIME, IDLE_SCALING and few other registers will be configured in driver

> ...
> 
> > +#define COMPLETION_MDONE     0x40
> > +#define COMPLETION_IDLE              0x20
> > +#define COMPLETION_MNAKX     0x01
> 
> Bits? Same Q for the rest similar stuff.

Yes. These are bits.

> ...
> 
> > +#define SMB_IDLE_SCALING_100KHZ              0x03E803C9
> > +#define SMB_IDLE_SCALING_400KHZ              0x01F4009D
> > +#define SMB_IDLE_SCALING_1000KHZ     0x01F4009D
> 
> Shouldn't these magics be 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
> 
> 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

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

> ...
> 
> > +/*
> > + * struct pci1xxxx_i2c - private structure for the I2C controller
> 
> > + *
> 
> Redundant blank line.

Will take care of this in upcoming patch

> 
> > + * @adap:    I2C adapter instance
> > + * @dev:     pointer to device struct
> > + * @i2c_base:        pci base address of the I2C ctrler
> > + * @i2c_xfer_done: used for synchronisation between foreground & isr
> > + * @freq:    frequency of I2C transfer
> > + * @flags:   internal flags to store transfer conditions
> > + * @irq:     irq number
> > + */
> 
> > +
> 
> Ditt.

Will take care of this in upcoming patch

> > +struct pci1xxxx_i2c {
> > +     struct completion i2c_xfer_done;
> > +     bool i2c_xfer_in_progress;
> > +     struct i2c_adapter adap;
> > +     void __iomem *i2c_base;
> > +     u32 freq;
> > +     u32 flags;
> > +};
> 
> I have lack of time to finish review, but you already have enough for the next
> version.
> 
> ...
> 
> > +                     transferlen = min((u16)(SMBUS_MAST_BUF_MAX_SIZE - 1),
> > +                                       remainingbytes);
> 
> min_t()

Okay, Will replace min with min_t

> ...
> 
> > +             if (remainingbytes <= transferlen && (i2c->flags &
> > +                                                     I2C_FLAGS_STOP))
> 
> Strange indentation.

Okay, Will take care of indentation in upcoming patch

> ...
> 
> > +             /*
> > +              * wait for the DMA_TERM interrupt and if the timer expires,
> > it means
> > +              * the transaction has failed due to some bus lock as we dint
> > get
> > +              * the interrupt
> > +              */
> 
> You really have to go through all comments and fix grammar, etc.

Okay

> ...
> 
> > +             time_left = wait_for_completion_timeout
> > +                             (&i2c->i2c_xfer_done,
> > msecs_to_jiffies(PCI1XXXX_I2C_TIMEOUT));
> 
> Strange indentation.

Okay, Will take care of indentation

> ...
> 
> > +     i2c_del_adapter(&i2c->adap);
> 
> Can't you use devm_ variant?

Okay, I will use devm_ variant

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


Thanks,
Tharun Kumar P