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

[Sanjay R Mehta <sanmehta@amd.com>]

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@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.
> 
Thanks Andy. Will incorporate the changes in the next version of patch.

> --
> With Best Regards,
> Andy Shevchenko
> 
>