Re: [PATCH] i2c: add i2c bus driver for AMD NAVI GPU

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Nov 27, 2020 at 01:30:39PM -0600, Sanjay R Mehta wrote:
> From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.Shah@amd.com>
> 
> Latest AMD GPU card has USB Type-C interface. There is a
> Type-C controller which can be accessed over I2C.
> 
> This driver adds I2C bus driver to communicate with Type-C controller.
> I2C client driver will be part of USB Type-C UCSI driver.

...

> +I2C CONTROLLER DRIVER FOR AMD NAVI GPU

>  I2C MUXES

I always thought that NVIDIA should come after AMD...
You still didn't learn to run checkpatch.pl?

...

> +#include <asm/unaligned.h>

Move this after linux/* ones, or explain why should it be first.

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>

...

> +#define DRIVER_DESC "AMD I2C Controller Driver for Navi"
> +#define AMD_UCSI_INTR_REG 0x474
> +#define AMD_UCSI_INTR_EN 0xD
> +#define AMD_MASTERCFG_MASK GENMASK_ULL(15, 0)

linux/bits.h is missing.

May you create a better indentation of the values to make it easier to read?

> +struct amdgpu_i2c_dev {
> +	void __iomem *regs;

DesignWare driver has been converted to use regmap. How comes you are using old
approach?

> +	struct device *dev;
> +	u32 master_cfg;
> +	u32 slave_adr;
> +	u32			tx_fifo_depth;
> +	u32			rx_fifo_depth;
> +	u32			sda_hold_time;
> +	u16			ss_hcnt;
> +	u16			ss_lcnt;
> +	u16			fs_hcnt;
> +	u16			fs_lcnt;
> +	u16			fp_hcnt;
> +	u16			fp_lcnt;
> +	u16			hs_hcnt;
> +	u16			hs_lcnt;
> +	struct i2c_adapter adapter;
> +	struct i2c_board_info *gpu_ccgx_ucsi;
> +	struct i2c_client *ccgx_client;
> +};

...

> +	while (readl(i2cd->regs + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> +		if (timeout <= 0) {
> +			dev_dbg(i2cd->dev, "timeout waiting for bus ready\n");
> +			if (readl(i2cd->regs + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY)
> +				return -ETIMEDOUT;
> +			return 0;
> +		}
> +		timeout--;
> +		usleep_range(1000, 1100);
> +	}

Homework: discover iopoll.h (or regmap.h if we take into account previous
comment). Bonus: try to read newest kernel submission in the area to see what's
new.

I stopped here. I think it's enough to revisit entire patch.
It will look differently for sure when you address all given comments.

-- 
With Best Regards,
Andy Shevchenko