Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for I2C host controller in multifunction endpoint of pci1xxxx switch
From: Andy Shevchenko
Date: Tue Aug 23 2022 - 13:28:57 EST
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.
> + 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.
...
> obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o
> +obj-$(CONFIG_I2C_PCI1XXXX) += i2c-mchp-pci1xxxx.o
Why unsorted?
...
> + * Author: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
> + * Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
Single or many?
...
> +/*SMB register space*/
Style.
...
> +#define SMB_CORE_CTRL_ESO 0x40
> +#define SMB_CORE_CTRL_FW_ACK 0x10
Are they bits or numbers?
...
> +#define SMB_CORE_CMD_READM 0x10
> +#define SMB_CORE_CMD_STOP 0x04
> +#define SMB_CORE_CMD_START 0x01
Ditto.
...
> +#define SMB_CORE_CMD_M_PROCEED 0x02
> +#define SMB_CORE_CMD_M_RUN 0x01
Ditto.
...
> +#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?
...
> +#define COMPLETION_MDONE 0x40
> +#define COMPLETION_IDLE 0x20
> +#define COMPLETION_MNAKX 0x01
Bits? Same Q for the rest similar stuff.
...
> +#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.
...
> +#define I2C_DIR_WRITE 0
> +#define I2C_DIR_READ 1
Namespace collision. Doesn't I²C core provide these?
...
> +#define PCI1XXXX_I2C_TIMEOUT 1000
Units? Same to the rest similar cases.
...
> +#define SMBUS_PERI_LOCK BIT(3)
BIT() out of a sudden. See above.
...
> +/*
> + * struct pci1xxxx_i2c - private structure for the I2C controller
> + *
Redundant blank line.
> + * @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.
> +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()
...
> + if (remainingbytes <= transferlen && (i2c->flags &
> + I2C_FLAGS_STOP))
Strange indentation.
...
> + /*
> + * 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.
...
> + time_left = wait_for_completion_timeout
> + (&i2c->i2c_xfer_done, msecs_to_jiffies(PCI1XXXX_I2C_TIMEOUT));
Strange indentation.
...
> + i2c_del_adapter(&i2c->adap);
Can't you use devm_ variant?
...
> + pci1xxxx_i2c_shutdown(i2c);
Do you really need this in ->remove()? I would expect something in
the ->suspend() / ->shutdown().
--
With Best Regards,
Andy Shevchenko