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

From: Tharunkumar.Pasumarthi
Date: Wed Aug 24 2022 - 10:38:42 EST


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