Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Dmitry Osipenko <digetx@gmail.com>]

29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:

Change title of the patch to:

"i2c: busses: Add driver for PCI1XXXX adapter"

...
> +#define SMB_CORE_CTRL_ESO		(0x40)
> +#define SMB_CORE_CTRL_FW_ACK		(0x10)

Parentheses are not needed here.


> +#define PCI1XXXX_I2C_TIMEOUT	(msecs_to_jiffies(1000))

And here.

...
> +static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
> +{
> +	struct pci1xxxx_i2c *i2c = dev;
> +	bool intr_handled = false;
> +	unsigned long flags;
> +	u16 regval;
> +	u8 regval1;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);

Interrupt is disabled in ISR, this lock does nothing.

> +	/* Mask the interrupt */
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> +	regval |= (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK);
> +	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));

writew(regval, (parentheses not needed here));

Similar for all other places in the code.

> +	/*
> +	 * Read the SMBus interrupt status register to see if the
> +	 * DMA_TERM interrupt has caused this callback
> +	 */
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
> +
> +	if (regval & I2C_BUF_MSTR_INTR_MASK) {
> +		regval1 = readb(i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
> +		if (regval1 & INTR_STAT_DMA_TERM) {
> +			complete(&i2c->i2c_xfer_done);
> +			intr_handled = true;
> +			writeb(INTR_STAT_DMA_TERM,
> +			       (i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF));
> +		}
> +		/* ACK the high level interrupt */
> +		writew(I2C_BUF_MSTR_INTR_MASK,
> +		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> +	}
> +
> +	if (regval & SMBALERT_INTR_MASK) {
> +		intr_handled = true;
> +		/* ACK the high level interrupt */
> +		writew(SMBALERT_INTR_MASK,
> +		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> +	}
> +
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> +	/* UnMask the interrupt */
> +	regval &= ~(I2C_BUF_MSTR_INTR_MASK | SMBALERT_INTR_MASK);
> +	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	if (intr_handled)
> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;

return intr_handled ? IRQ_HANDLED : IRQ_NONE;

Or turn intr_handled into "irqreturn_t ret" and return it directly.

...
> +static void pci1xxxx_i2c_configure_core_reg(struct pci1xxxx_i2c *i2c, bool enable)
> +{
> +	u8 regval;
> +	u8 regval1;

u8 reg1, reg3;

...> +static void pci1xxxx_i2c_set_freq(struct pci1xxxx_i2c *i2c)
> +{
> +	/*
> +	 * The SMB core needs specific values to be set in the BUS_CLK register
> +	 * for the corresponding frequency
> +	 */
> +	switch (i2c->freq) {

Why i2c->freq is fixed to I2C_MAX_FAST_MODE_FREQ in pci1xxxx_i2c_init()?

...
> +static void pci1xxxx_i2c_init(struct pci1xxxx_i2c *i2c)
> +{
> +	i2c->freq = I2C_MAX_FAST_MODE_FREQ;
...

> +static u32 pci1xxxx_i2c_get_funcs(struct i2c_adapter *adap)
> +{
> +	return (I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING
> +		| I2C_FUNC_SMBUS_BLOCK_PROC_CALL
> +		| I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE
> +		| I2C_FUNC_SMBUS_READ_BYTE_DATA
> +		| I2C_FUNC_SMBUS_WRITE_BYTE_DATA
> +		| I2C_FUNC_SMBUS_READ_WORD_DATA
> +		| I2C_FUNC_SMBUS_WRITE_WORD_DATA
> +		| I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_READ_BLOCK_DATA
> +		| I2C_FUNC_SMBUS_WRITE_BLOCK_DATA);
> +}
The 'or' should be on the right side and parentheses are not needed.

...
> +static int pci1xxxx_i2c_resume(struct device *dev)> +{
> +	struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	u32 regval;
> +
> +	i2c_mark_adapter_resumed(&i2c->adap);
> +
> +	regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> +	regval &= ~PERI_SMBUS_D3_RESET_DIS;
> +	writel(regval, i2c->i2c_base + SMBUS_RESET_REG);

Adapter is resumed after register is written.

> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
> +			 pci1xxxx_i2c_resume);
> +
> +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> +				  const struct pci_device_id *ent)
> +{
...
> +	pci1xxxx_i2c_init(i2c);
> +
> +	i2c->adap = pci1xxxx_i2c_ops;
> +
> +	i2c->adap.class = I2C_CLASS_SPD;
> +	i2c->adap.dev.parent = &pdev->dev;
> +
> +	snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> +		 "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
> +
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +
> +	ret = i2c_add_adapter(&i2c->adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> +		goto err_free_region;

pci1xxxx_i2c_shutdown()?

> +	}
> +
> +	return 0;
> +
> +err_free_region:
> +	pci_release_regions(pdev);
> +	return ret;
> +}
> +
> +static void pci1xxxx_i2c_shutdown(struct pci1xxxx_i2c *i2c)
> +{
> +	pci1xxxx_i2c_config_padctrl(i2c, false);
> +	pci1xxxx_i2c_configure_core_reg(i2c, false);
> +}
> +
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> +	struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> +
> +	pci1xxxx_i2c_shutdown(i2c);
> +	i2c_del_adapter(&i2c->adap);
> +}
> +
> +static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI12000_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11010_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11101_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11400_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11414_I2C_PID) },
> +	{0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);
> +
> +static struct pci_driver pci1xxxx_i2c_pci_driver = {
> +	.name		= DRV_NAME,

Assign name directly, DRV_NAME unneeded.