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;
> +}
--
With Best Regards,
Andy Shevchenko
