On 11/30/2020 4:49 PM, Andy Shevchenko wrote:
> [CAUTION: External Email]
>
> 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.
>
Thanks Andy. Will incorporate the changes in the next version of patch.
> --
> With Best Regards,
> Andy Shevchenko
>
>
