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@xxxxxxxxxxxxx>
> > + * Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
>
> 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
