RE: [PATCH 2/7] usb: typec: ucsi: add ccg command framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Heikki
> > > > Used to send command to ccg controller
> > > >
> > > > Signed-off-by: Ajay Gupta <ajayg@xxxxxxxxxx>
> > > > ---
> > > >  drivers/usb/typec/ucsi/ucsi_ccg.c | 252
> > > > ++++++++++++++++++++++++++++--
> > > >  1 file changed, 243 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > index 4d35279ab853..dce9126b6a37 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > @@ -17,6 +17,29 @@
> > > >  #include <asm/unaligned.h>
> > > >  #include "ucsi.h"
> > > >
> > > > +#define CCGX_RAB_DEVICE_MODE			0x0000
> > > > +#define CCGX_RAB_INTR_REG			0x0006
> > > > +#define  DEV_INT				BIT(0)
> > > > +#define  PORT0_INT				BIT(1)
> > > > +#define  PORT1_INT				BIT(2)
> > > > +#define  UCSI_READ_INT				BIT(7)
> > > > +#define CCGX_RAB_READ_ALL_VER			0x0010
> > > > +#define CCGX_RAB_READ_FW2_VER			0x0020
> > > > +#define CCGX_RAB_UCSI_CONTROL			0x0039
> > > > +#define CCGX_RAB_UCSI_CONTROL_START		BIT(0)
> > > > +#define CCGX_RAB_UCSI_CONTROL_STOP		BIT(1)
> > > > +#define CCGX_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) &
> > > 0xff))
> > > > +#define DEV_REG_IDX
> > > 	CCGX_RAB_DEVICE_MODE
> > > > +#define CCGX_RAB_RESPONSE			0x007E
> > > > +#define  ASYNC_EVENT				BIT(7)
> > > > +
> > > > +/* CCGx events & async msg codes */
> > > > +#define RESET_COMPLETE		0x80
> > > > +#define EVENT_INDEX		RESET_COMPLETE
> > > > +#define PORT_CONNECT_DET	0x84
> > > > +#define PORT_DISCONNECT_DET	0x85
> > > > +#define ROLE_SWAP_COMPELETE	0x87
> > > > +
> > > >  struct ccg_dev_info {
> > > >  	u8 fw_mode:2;
> > > >  	u8 two_pd_ports:2;
> > > > @@ -44,23 +67,113 @@ struct version_info {
> > > >  	struct version_format app;
> > > >  };
> > > >
> > > > +/* CCGx response codes */
> > > > +enum ccg_resp_code {
> > > > +	CMD_NO_RESP             = 0x00,
> > > > +	CMD_SUCCESS             = 0x02,
> > > > +	FLASH_DATA_AVAILABLE    = 0x03,
> > > > +	CMD_INVALID             = 0x05,
> > > > +	FLASH_UPDATE_FAIL       = 0x07,
> > > > +	INVALID_FW              = 0x08,
> > > > +	INVALID_ARG             = 0x09,
> > > > +	CMD_NOT_SUPPORT         = 0x0A,
> > > > +	TRANSACTION_FAIL        = 0x0C,
> > > > +	PD_CMD_FAIL             = 0x0D,
> > > > +	UNDEF_ERROR             = 0x0F,
> > > > +	INVALID_RESP		= 0x10,
> > > > +};
> > > > +
> > > > +static const char * const ccg_resp_strs[] = {
> > > > +	/* 0x00 */ "No Response.",
> > > > +	/* 0x01 */ "0x01",
> > > > +	/* 0x02 */ "HPI Command Success.",
> > > > +	/* 0x03 */ "Flash Data Available in data memory.",
> > > > +	/* 0x04 */ "0x04",
> > > > +	/* 0x05 */ "Invalid Command.",
> > > > +	/* 0x06 */ "0x06",
> > > > +	/* 0x07 */ "Flash write operation failed.",
> > > > +	/* 0x08 */ "Firmware validity check failed.",
> > > > +	/* 0x09 */ "Command failed due to invalid arguments.",
> > > > +	/* 0x0A */ "Command not supported in the current mode.",
> > > > +	/* 0x0B */ "0x0B",
> > > > +	/* 0x0C */ "Transaction Failed. GOOD_CRC was not received.",
> > > > +	/* 0x0D */ "PD Command Failed.",
> > > > +	/* 0x0E */ "0x0E",
> > > > +	/* 0x0F */ "Undefined Error",
> > > > +};
> > > > +
> > > > +static const char * const ccg_evt_strs[] = {
> > > > +	/* 0x80 */ "Reset Complete.",
> > > > +	/* 0x81 */ "Message queue overflow detected.",
> > > > +	/* 0x82 */ "Overcurrent Detected",
> > > > +	/* 0x83 */ "Overvoltage Detected",
> > > > +	/* 0x84 */ "Type-C Port Connect Detected",
> > > > +	/* 0x85 */ "Type-C Port Disconnect Detected",
> > > > +	/* 0x86 */ "PD Contract Negotiation Complete",
> > > > +	/* 0x87 */ "SWAP Complete",
> > > > +	/* 0x88 */ "0x88",
> > > > +	/* 0x89 */ "0x89",
> > > > +	/* 0x8A */ "PS_RDY Message Received",
> > > > +	/* 0x8B */ "GotoMin Message Received.",
> > > > +	/* 0x8C */ "Accept Message Received",
> > > > +	/* 0x8D */ "Reject Message Received",
> > > > +	/* 0x8E */ "Wait Message Received",
> > > > +	/* 0x8F */ "Hard Reset Received",
> > > > +	/* 0x90 */ "VDM Received",
> > > > +	/* 0x91 */ "Source Capabilities Message Received",
> > > > +	/* 0x92 */ "Sink Capabilities Message Received",
> > > > +	/* 0x93 */ "Display Port Alternate Mode entered",
> > > > +	/* 0x94 */ "Display Port device connected at UFP_U",
> > > > +	/* 0x95 */ "Display port device not connected at UFP_U",
> > > > +	/* 0x96 */ "Display port SID not found in Discover SID process",
> > > > +	/* 0x97 */ "Multiple SVIDs discovered along with DisplayPort SID",
> > > > +	/* 0x98 */ "DP Functionality not supported by Cable",
> > > > +	/* 0x99 */ "Display Port Configuration not supported by UFP",
> > > > +	/* 0x9A */ "Hard Reset Sent to Port Partner",
> > > > +	/* 0x9B */ "Soft Reset Sent to Port Partner",
> > > > +	/* 0x9C */ "Cable Reset Sent to EMCA",
> > > > +	/* 0x9D */ "Source Disabled State Entered",
> > > > +	/* 0x9E */ "Sender Response Timer Timeout",
> > > > +	/* 0x9F */ "No VDM Response Received",
> > > > +	/* 0xA0 */ "Unexpected Voltage on Vbus",
> > > > +	/* 0xA1 */ "Type-C Error Recovery",
> > > > +	/* 0xA2 */ "0xA2",
> > > > +	/* 0xA3 */ "0xA3",
> > > > +	/* 0xA4 */ "0xA4",
> > > > +	/* 0xA5 */ "0xA5",
> > > > +	/* 0xA6 */ "EMCA Detected",
> > > > +	/* 0xA7 */ "0xA7",
> > > > +	/* 0xA8 */ "0xA8",
> > > > +	/* 0xA9 */ "0xA9",
> > > > +	/* 0xAA */ "Rp Change Detected", };
> > >
> > > Those looks like something that you could use in a tracepoint.
> > These are various events sent from ccg controller in response to a
> > cmd. We use it to display message from any error events.
> >
> > > Can you check how much would it take to implement tracepoints for this
> driver.
> > > They would be more useful compared to the dev_dbg() you are using now.
> > Is it okay to add new tracepoint for ucsi_ccg driver or to reuse
> drivers/usb/typec/ucsi/trace.c, trace.h ?
> 
> I don't know actually what is the best approach.
> 
> Can you skip those debug messages for now. They need to be added in a
> separate patch in any case. Or patches actually.
> 
> I think it would be better that you first introduce the feature without any
> debugging prints. Add the debugging after the feature is in.
> 
> So in your next version, drop all dev_dbg() prints from all patches in the series.
Sure.

Thanks
> nvpublic
> 
> 
> thanks,
> 
> --
> heikki




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux