On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote:
Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the firmware needs to access I2C all the time for some features, the semaphore is used between software and firmware. The driver should set software semaphore before accessing I2C bus and release it when it is finished. Otherwise, there is probability that the correct information on I2C bus will not be obtained.
...
i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-slave.o
i2c-designware-platform-y := i2c-designware-platdrv.o +i2c-designware-platform-y += i2c-designware-wx.o
These lines have TABs/spaces mixture. Please fix at least your entry to avoid this from happening.
...
int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev); #endif
^^^
+int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev);
See below.
...
.probe = i2c_dw_amdpsp_probe_lock_support,}, #endif
^^^
- {
.probe = i2c_dw_txgbe_probe_lock_support,- },
Do we all need this support? Even if the driver is not compiled? Why?
...
+#include <linux/platform_data/i2c-wx.h> +#include <linux/platform_device.h> +#include <linux/i2c.h> +#include <linux/pci.h>
This is a semi-random list. Please, take your time to understand the core you wrote. Follow IWYU principle.
...
+static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev) +{
- void __iomem *req_addr;
- u32 swsm;
- int i;
- req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW;
- for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) {
Retry loops much better in a form of
unsigned int retries = ...; ... do { ... } while (--retries);
BUT... see below.
writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr);/* If we set the bit successfully then we got semaphore. */swsm = readl(req_addr);if (swsm & I2C_DW_TXGBE_MNG_SW_SM)break;udelay(50);
So, can a macro from iopoll.h be utilised here? Why not?
- }
- if (i == I2C_DW_TXGBE_REQ_RETRY_CNT)
return -ETIMEDOUT;- return 0;
+}
+int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev) +{
- struct platform_device *pdev = to_platform_device(dev->dev);
Why do you need this dance? I.o.w. how pdev is being used here?
- struct txgbe_i2c_platform_data *pdata;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata)
return -ENXIO;- dev->ext = pdata->hw_addr;
- if (!dev->ext)
return -ENXIO;- dev->acquire_lock = i2c_dw_txgbe_acquire_lock;
- dev->release_lock = i2c_dw_txgbe_release_lock;
- return 0;
+}