[v5,2/2] usb: typec: ucsi: add support for Cypress CCGx

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Aug 31, 2018 at 01:22:21PM -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.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
> Changes from v1 -> v2
> 	Fixed identation in drivers/usb/typec/ucsi/Kconfig
> Changes from v2 -> v3
> 	Fixed most of comments from Heikki
> 	Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
> Changes from v3 -> v4
> 	Fixed comments from Andy
> Changes from v4 -> v5
> 	Fixed comments from Andy
> 
>  drivers/usb/typec/ucsi/Kconfig    |  10 +
>  drivers/usb/typec/ucsi/Makefile   |   2 +
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 390 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 402 insertions(+)
>  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..7811888 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_CCG
> +	tristate "UCSI Interface Driver for Cypress CCGx"
> +	depends on I2C
> +	help
> +	  This driver enables UCSI support on platforms 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_ccg.
> +
>  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..2f4900b 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_CCG)		+= ucsi_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> new file mode 100644
> index 0000000..1e7c3b2
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI driver for Cypress CCGx Type-C controller
> + *
> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta <ajayg@nvidia.com>
> + *
> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> + */
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/unaligned.h>
> +#include "ucsi.h"
> +
> +struct ucsi_ccg {
> +	struct device *dev;
> +	struct ucsi *ucsi;
> +	struct ucsi_ppm ppm;
> +	struct i2c_client *client;
> +	int irq;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE			0x00
> +#define CCGX_I2C_RAB_READ_SILICON_ID			0x2
> +#define CCGX_I2C_RAB_INTR_REG				0x06
> +#define CCGX_I2C_RAB_FW1_VERSION			0x28
> +#define CCGX_I2C_RAB_FW2_VERSION			0x20
> +#define CCGX_I2C_RAB_UCSI_CONTROL			0x39
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START			BIT(0)
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP			BIT(1)
> +#define CCGX_I2C_RAB_RESPONSE_REG			0x7E
> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK			0xf000
> +
> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> +	struct device *dev = uc->dev;
> +	struct i2c_client *client = uc->client;
> +	unsigned char buf[2];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0x0,
> +			.len	= 0x2,
> +			.buf	= buf,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags  = I2C_M_RD,
> +			.buf	= data,
> +		},
> +	};

Are you sure you are allowed to do i2c messages off of the stack like
this?  Will that work on all platforms?


> +	u32 rlen, rem_len = len;
> +	int err;
> +
> +	while (rem_len > 0) {
> +		msgs[1].buf = &data[len - rem_len];
> +		rlen = min_t(u16, rem_len, 4);
> +		msgs[1].len = rlen;
> +		put_unaligned_le16(rab, buf);
> +		err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +		if (err < 0) {
> +			dev_err(dev, "i2c_transfer failed, err %d\n", err);

You are printing out an error here, no need to print out another
error every time you call this function and it fails, right?  Only do it
in one place, otherwise it is extra noisy when things fail.

> +			return err;
> +		}
> +		rab += rlen;
> +		rem_len -= rlen;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> +	struct device *dev = uc->dev;
> +	struct i2c_client *client = uc->client;
> +	unsigned char buf[2];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0x0,
> +			.len	= 0x2,
> +			.buf	= buf,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0x0,
> +			.buf	= data,
> +			.len	= len,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags  = I2C_M_STOP,
> +		},
> +	};
> +	int err;

Same question here, i2c message off of the stack?

> +
> +	put_unaligned_le16(rab, buf);
> +	err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (err < 0) {
> +		dev_err(dev, "i2c_transfer failed, err %d\n", err);
> +		return err;

And again, only print an error in one place please.

This can be simplified to just:
	return i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
right?


> +	}
> +
> +	return 0;
> +}
> +
> +static int ucsi_ccg_init(struct ucsi_ccg *uc)
> +{
> +	struct device *dev = uc->dev;
> +	unsigned int count = 10;
> +	u8 data[64];
> +	int err;
> +
> +	/*
> +	 * Selectively issue device reset
> +	 * - if RESPONSE register is RESET_COMPLETE, do not issue device reset
> +	 *   (will cause usb device disconnect / reconnect)
> +	 * - if RESPONSE register is not RESET_COMPLETE, issue device reset
> +	 *   (causes PPC to resync device connect state by re-issuing
> +	 *   set mux command)
> +	 */
> +	data[0] = 0x00;
> +	data[1] = 0x00;

Again, it's ok to do messages off of the stack?

thanks,

greg k-h