RE: [PATCH v2 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Heikki,
> On Fri, Aug 24, 2018 at 02:33:36PM -0700, Ajay Gupta wrote:
> > Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> > over I2C interface.
> >
> > This UCSI I2C driver uses I2C bus driver interface for communicating
> > with Type-C controller.
>
> Cool. The patch looks fairly good to me, but I put a few comments
> below.
Thanks for review.
>
> > Signed-off-by: Ajay Gupta
> > ---
> > Changes from v1 -> v2:
> > Fixed identation in drivers/usb/typec/ucsi/Kconfig
> >
> > drivers/usb/typec/ucsi/Kconfig| 10 +
> > drivers/usb/typec/ucsi/Makefile | 2 +
> > drivers/usb/typec/ucsi/ucsi_i2c_ccg.c | 591
> ++
>
> CCGx controllers support also SPI and UART AFAIK. Though, I'm not sure
> how commonly they are used (I would expect I2C to be the most common
> with these controllers), the driver should ultimately work with both
> of those busses as well.
>
> To avoid confusion, and potential driver duplicates in the future,
> just name the driver ucsi_ccg.c for now, and also s/i2c_ccg/ccg/
> everything in the driver (except the i2c_driver structure of course).
Sure.
> > 3 files changed, 603 insertions(+)
> > create mode 100644 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> >
> > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > index e36d6c7..0ce9d48 100644
> > --- a/drivers/usb/typec/ucsi/Kconfig
> > +++ b/drivers/usb/typec/ucsi/Kconfig
> > @@ -23,6 +23,16 @@ config TYPEC_UCSI
> >
> > if TYPEC_UCSI
> >
> > +config UCSI_I2C_CCG
> > + tristate "UCSI I2C Interface Driver for Cypress CCGx"
> > + depends on I2C_GPU
>
> Why does it need to depend only on your I2C controller?
>
> I think that should be just "depends on I2C".
ok
> > + help
> > + This driver enables UCSI support on NVIDIA GPUs that expose a
> > + Cypress CCGx Type-C controller over I2C interface.
> > +
> > + To compile the driver as a module, choose M here: the module will
> be
> > + called ucsi_i2c_ccg.ko.
> > +
> > config UCSI_ACPI
> > tristate "UCSI ACPI Interface Driver"
> > depends on ACPI
> > diff --git a/drivers/usb/typec/ucsi/Makefile
> b/drivers/usb/typec/ucsi/Makefile
> > index 7afbea5..4439482 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -8,3 +8,5 @@ typec_ucsi-y:= ucsi.o
> > typec_ucsi-$(CONFIG_TRACING) += trace.o
> >
> > obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
> > +
> > +obj-$(CONFIG_UCSI_I2C_CCG) += ucsi_i2c_ccg.o
> > diff --git a/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> > new file mode 100644
> > index 000..587e3f8
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> > @@ -0,0 +1,591 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * UCSI I2C driver for Cypress CCGx Type-C controller
> > + *
> > + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> > + * Author: Ajay Gupta
> > + *
> > + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "ucsi.h"
> > +
> > +struct ucsi_i2c_ccg {
> > + struct device *dev;
> > + struct ucsi *ucsi;
> > + struct ucsi_ppm ppm;
> > + struct i2c_client *client;
> > + int irq;
> > + bool wake_enabled;
> > + unsigned char ver;
> > +};
> > +
> > +#define CCGX_I2C_RAB_DEVICE_MODE 0xU
> > +#define CCGX_I2C_RAB_BOOT_MODE_REASON
> 0x0001U
> > +#define CCGX_I2C_RAB_READ_SILICON_ID 0x0002U
> > +#define CCGX_I2C_RAB_INTR_REG 0x0006U
> > +#define CCGX_I2C_RAB_RESET 0x0008U
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION 0x0010U
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x00)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_BASE \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER
> + 0)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_FW \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER
> + 4)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x08)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_BASE \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 0)
> > +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_FW \
> > + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 4)
> > +#define CCGX_I2C_RAB_FW2_VERSION 0x0020U
> > +#define CCGX_I2C_RAB_PDPORT_ENABLE 0x002CU
> > +#define CCGX_I2C_RAB_UCSI_STATUS 0x0038U
> > +#define CCGX_I2C_RAB_UCSI_CONTROL 0x0039U
> > +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP 0x2U
> > +#define CCGX_I2C_RAB_UCSI_CONTROL_START0x1U
> > +#define
Re: [PATCH v2 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Ajay,
I love your patch! Yet something to improve:
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.19-rc1 next-20180827]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Ajay-Gupta/i2c-buses-add-i2c-bus-driver-for-NVIDIA-GPU/20180827-093218
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
i2c/for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/usb/typec/ucsi/ucsi_i2c_ccg.c: In function 'ucsi_i2c_ccg_resume':
>> drivers/usb/typec/ucsi/ucsi_i2c_ccg.c:559:8: error: implicit declaration of
>> function 'ucsi_run_command'; did you mean 'ucsi_i2c_ccg_cmd'?
>> [-Werror=implicit-function-declaration]
ret = ucsi_run_command(ui->ucsi, , NULL, 0);
^~~~
ucsi_i2c_ccg_cmd
cc1: some warnings being treated as errors
vim +559 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
544
545 static int ucsi_i2c_ccg_resume(struct device *dev)
546 {
547 struct i2c_client *client = to_i2c_client(dev);
548 struct ucsi_i2c_ccg *ui = i2c_get_clientdata(client);
549 struct ucsi_control c;
550 int ret;
551
552 if (device_may_wakeup(dev) && ui->wake_enabled) {
553 disable_irq_wake(ui->irq);
554 ui->wake_enabled = false;
555 }
556
557 /* restore UCSI notification enable mask */
558 UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
> 559 ret = ucsi_run_command(ui->ucsi, , NULL, 0);
560 if (ret) {
561 dev_err(ui->dev, "%s: failed to set notification enable
- %d\n",
562 __func__, ret);
563 }
564
565 return 0;
566 }
567
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH v2 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Ajay,
On Fri, Aug 24, 2018 at 02:33:36PM -0700, Ajay Gupta wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
>
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
Cool. The patch looks fairly good to me, but I put a few comments
below.
> Signed-off-by: Ajay Gupta
> ---
> Changes from v1 -> v2:
> Fixed identation in drivers/usb/typec/ucsi/Kconfig
>
> drivers/usb/typec/ucsi/Kconfig| 10 +
> drivers/usb/typec/ucsi/Makefile | 2 +
> drivers/usb/typec/ucsi/ucsi_i2c_ccg.c | 591
> ++
CCGx controllers support also SPI and UART AFAIK. Though, I'm not sure
how commonly they are used (I would expect I2C to be the most common
with these controllers), the driver should ultimately work with both
of those busses as well.
To avoid confusion, and potential driver duplicates in the future,
just name the driver ucsi_ccg.c for now, and also s/i2c_ccg/ccg/
everything in the driver (except the i2c_driver structure of course).
> 3 files changed, 603 insertions(+)
> create mode 100644 drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
>
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..0ce9d48 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>
> if TYPEC_UCSI
>
> +config UCSI_I2C_CCG
> + tristate "UCSI I2C Interface Driver for Cypress CCGx"
> + depends on I2C_GPU
Why does it need to depend only on your I2C controller?
I think that should be just "depends on I2C".
> + help
> + This driver enables UCSI support on NVIDIA GPUs that expose a
> + Cypress CCGx Type-C controller over I2C interface.
> +
> + To compile the driver as a module, choose M here: the module will be
> + called ucsi_i2c_ccg.ko.
> +
> config UCSI_ACPI
> tristate "UCSI ACPI Interface Driver"
> depends on ACPI
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index 7afbea5..4439482 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -8,3 +8,5 @@ typec_ucsi-y := ucsi.o
> typec_ucsi-$(CONFIG_TRACING) += trace.o
>
> obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o
> +
> +obj-$(CONFIG_UCSI_I2C_CCG) += ucsi_i2c_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> new file mode 100644
> index 000..587e3f8
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_i2c_ccg.c
> @@ -0,0 +1,591 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI I2C driver for Cypress CCGx Type-C controller
> + *
> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta
> + *
> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "ucsi.h"
> +
> +struct ucsi_i2c_ccg {
> + struct device *dev;
> + struct ucsi *ucsi;
> + struct ucsi_ppm ppm;
> + struct i2c_client *client;
> + int irq;
> + bool wake_enabled;
> + unsigned char ver;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE 0xU
> +#define CCGX_I2C_RAB_BOOT_MODE_REASON0x0001U
> +#define CCGX_I2C_RAB_READ_SILICON_ID 0x0002U
> +#define CCGX_I2C_RAB_INTR_REG0x0006U
> +#define CCGX_I2C_RAB_RESET 0x0008U
> +#define CCGX_I2C_RAB_READ_ALL_VERSION0x0010U
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER \
> + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x00)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_BASE \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER + 0)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER_FW \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_BOOTLOADER + 4)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP \
> + (CCGX_I2C_RAB_READ_ALL_VERSION + 0x08)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_BASE \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 0)
> +#define CCGX_I2C_RAB_READ_ALL_VERSION_APP_FW \
> + (CCGX_I2C_RAB_READ_ALL_VERSION_APP + 4)
> +#define CCGX_I2C_RAB_FW2_VERSION 0x0020U
> +#define CCGX_I2C_RAB_PDPORT_ENABLE 0x002CU
> +#define CCGX_I2C_RAB_UCSI_STATUS 0x0038U
> +#define CCGX_I2C_RAB_UCSI_CONTROL0x0039U
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP 0x2U
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START 0x1U
> +#define CCGX_I2C_RAB_HPI_VERSION 0x003CU
> +#define CCGX_I2C_RAB_RESPONSE_REG0x007EU
> +#define CCGX_I2C_RAB_DM_CONTROL_10x1000U
> +#define CCGX_I2C_RAB_WRITE_DATA_MEMORY_1
Re: [PATCH v2 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Sat, Aug 25, 2018 at 07:29:24PM +0300, Andy Shevchenko wrote: > > --- > > This email message is for the sole use of the intended recipient(s) and may > > contain > > confidential information. Any unauthorized review, use, disclosure or > > distribution > > is prohibited. If you are not the intended recipient, please contact the > > sender by > > reply email and destroy all copies of the original message. > > --- > > You have an issue here. Not an issue if you want your emails to just be deleted automatically, like I do :)
Re: [PATCH v2 2/2] usb: typec: ucsi: add support for Cypress CCGx
> --- > This email message is for the sole use of the intended recipient(s) and may > contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact the > sender by > reply email and destroy all copies of the original message. > --- You have an issue here. -- With Best Regards, Andy Shevchenko
