On Fri, Nov 27, 2020 at 01:30:39PM -0600, Sanjay R Mehta wrote:
> From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.Shah@xxxxxxx>
>
> 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
