On Fri, Aug 23, 2024 10:13 PM, Andy Shevchenko wrote:
> 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?
I'll add the macro CONFIG_I2C_DESIGNWARE_WX to control it.
> ...
>
> > +#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?
I need to write the register first and then read it in this loop.
It does not seem to apply to the macros in iopoll.h.
> > + }
> > +
> > + 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?
I'll change to add the data in node property.
