linux-kernel - Re: [PATCH] i2c: add i2c bus driver for amd navi gpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YEeFgZSIY5lb2ubP@smile.fi.intel.com>
Date:   Tue, 9 Mar 2021 16:26:09 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Sanket Goswami <Sanket.Goswami@....com>
Cc:     jarkko.nikula@...ux.intel.com, mika.westerberg@...ux.intel.com,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@....com>
Subject: Re: [PATCH] i2c: add i2c bus driver for amd navi gpu

On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:

i2c: -> i2c: designware:
add i2c bus driver -> add a driver
amd -> AMD
gpu -> GPU

in the subject

> Latest AMDGPU NAVI cards have USB Type-C interface which can be accessed
> over I2C.

I didn't get this. You mean that USB traffic over I²C? This sounds insane
for a least. Maybe something else is there and description is not fully
correct?

> The Type-C controller has integrated designware i2c which is

DesignWare

> exposed as a PCI device to the AMD platform.
> 
> Also there exists couple of notable IP problems that are dealt as a
> workaround:
> - I2C transactions work on a polling mode as IP does not generate
> interrupt.
> 
> - I2C reads commands are sent twice to address the IP issues.

Please, read this article: https://chris.beams.io/posts/git-commit/

...

> +#define AMD_UCSI_INTR_EN	0xD

Why it's capitalized?

...

>  #include "i2c-designware-core.h"

+ Blank line

> +#define AMD_TIMEOUT_MSEC_MIN	10000
> +#define AMD_TIMEOUT_MSEC_MAX	11000

Use unit suffix in the definitions.

...

> +static void i2c_dw_configure_bus(struct dw_i2c_dev *i2cd)

Why all this is customized? Why FIFO can't be autodetected?

...

> +/* Initiate and continue master read/write transaction with polling
> + * based transfer routine and write messages into the tx buffer.
> + */

Wrong multi-line comment style.

...

> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)

Why do you need a custom function for that? Can it be generic and not AMD
specific?

...

> +	/* Enable ucsi interrupt */
> +	if (dev->flags & AMD_NON_INTR_MODE)
> +		regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);

This is looks like a hack. Why is it here?

...

> +	if (dev->flags & AMD_NON_INTR_MODE)
> +		return amd_i2c_dw_master_xfer(adap, msgs, num);

Ditto.

...

> +int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
> +{
> +	struct i2c_adapter *amdap = &amdev->adapter;

> +	int ret;

See below.

> +	if (!(amdev->flags & AMD_NON_INTR_MODE))
> +		return -ENODEV;

> +	return i2c_add_numbered_adapter(amdap);
> +
> +	return ret;

What the second one does?

> +}

...

> +	ret = amd_i2c_adap_quirk(dev);
> +	if (ret != -ENODEV)

This is ugly. Can we run quirk if and only if we have an AMD chip?

> +		return ret;

...

>  #define DRIVER_NAME "i2c-designware-pci"
> +#define AMD_CLK_RATE	100000

Add units.

...

> +/* NAVI-AMD HCNT/LCNT/SDA hold time */
> +static struct dw_scl_sda_cfg navi_amd_config = {
> +	.ss_hcnt = 0x1ae,
> +	.ss_lcnt = 0x23a,
> +	.sda_hold = 0x9,
> +};

(1)

...

> +static int i2c_dw_populate_client(struct dw_i2c_dev *i2cd)
> +{
> +	struct i2c_board_info	*i2c_dw_ccgx_ucsi;
> +	struct i2c_client	*ccgx_client;
> +
> +	i2c_dw_ccgx_ucsi = devm_kzalloc(i2cd->dev, sizeof(*i2c_dw_ccgx_ucsi), GFP_KERNEL);
> +	if (!i2c_dw_ccgx_ucsi)
> +		return -ENOMEM;
> +
> +	strscpy(i2c_dw_ccgx_ucsi->type, "ccgx-ucsi", sizeof(i2c_dw_ccgx_ucsi->type));
> +	i2c_dw_ccgx_ucsi->addr = 0x08;
> +	i2c_dw_ccgx_ucsi->irq = i2cd->irq;
> +
> +	ccgx_client = i2c_new_client_device(&i2cd->adapter, i2c_dw_ccgx_ucsi);
> +	if (!ccgx_client)
> +		return -ENODEV;
> +
> +	return 0;
> +}

This is the same as in nVidia GPU I²C driver. Can you do a preparatory changes
to deduplicate this?

Also why (1) and this can't be instantiated from ACPI / DT?

-- 
With Best Regards,
Andy Shevchenko