Hi,
On Fri, Jun 09, 2017 at 07:52:56PM -0700, Guenter Roeck wrote:
> > diff --git a/drivers/usb/typec/ucsi/debug.h b/drivers/usb/typec/ucsi/debug.h
> > new file mode 100644
> > index 000000000000..87d0cd20597a
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/debug.h
> > @@ -0,0 +1,64 @@
> > +#ifndef __UCSI_DEBUG_H
> > +#define __UCSI_DEBUG_H
> > +
> > +#include "ucsi.h"
> > +
> > +static const char * const ucsi_cmd_strs[] = {
> > + [0] = "Unknown command",
> > + [UCSI_PPM_RESET] = "PPM_RESET",
> > + [UCSI_CANCEL] = "CANCEL",
> > + [UCSI_CONNECTOR_RESET] = "CONNECTOR_RESET",
> > + [UCSI_ACK_CC_CI] = "ACK_CC_CI",
> > + [UCSI_SET_NOTIFICATION_ENABLE] = "SET_NOTIFICATION_ENABLE",
> > + [UCSI_GET_CAPABILITY] = "GET_CAPABILITY",
> > + [UCSI_GET_CONNECTOR_CAPABILITY] = "GET_CONNECTOR_CAPABILITY",
> > + [UCSI_SET_UOM] = "SET_UOM",
> > + [UCSI_SET_UOR] = "SET_UOR",
> > + [UCSI_SET_PDM] = "SET_PDM",
> > + [UCSI_SET_PDR] = "SET_PDR",
> > + [UCSI_GET_ALTERNATE_MODES] = "GET_ALTERNATE_MODES",
> > + [UCSI_GET_CAM_SUPPORTED] = "GET_CAM_SUPPORTED",
> > + [UCSI_GET_CURRENT_CAM] = "GET_CURRENT_CAM",
> > + [UCSI_SET_NEW_CAM] = "SET_NEW_CAM",
> > + [UCSI_GET_PDOS] = "GET_PDOS",
> > + [UCSI_GET_CABLE_PROPERTY] = "GET_CABLE_PROPERTY",
> > + [UCSI_GET_CONNECTOR_STATUS] = "GET_CONNECTOR_STATUS",
> > + [UCSI_GET_ERROR_STATUS] = "GET_ERROR_STATUS",
> > +};
> > +
> > +static inline const char *ucsi_cmd_str(u64 raw_cmd)
> > +{
> > + u8 cmd = raw_cmd & GENMASK(7, 0);
> > +
> > + return ucsi_cmd_strs[(cmd > ARRAY_SIZE(ucsi_cmd_strs)) ? 0 : cmd];
>
> I thought I mentioned this before. What if cmd == ARRAY_SIZE(ucsi_cmd_strs) ?
> I may be missing something, but I am quite sure that would point beyond
> the end of the array.
I failed to notice your previous comment about this, sorry. I'll fix
these.
> > +}
> > +
> > +static const char * const ucsi_ack_strs[] = {
> > + [0] = "",
> > + [UCSI_ACK_EVENT] = "event",
> > + [UCSI_ACK_CMD] = "command",
> > +};
> > +
> > +static inline const char *ucsi_ack_str(u8 ack)
> > +{
> > + return ucsi_ack_strs[(ack > ARRAY_SIZE(ucsi_ack_strs)) ? 0 : ack];
>
> Same here.
<snip>
> > +/* Command Status and Connector Change Indication (CCI) data structure */
> > +struct ucsi_cci {
> > + u8:1; /* reserved */
> > + u8 connector_change:7;
> > + u8 data_length;
> > + u16:9; /* reserved */
> > + u16 not_supported:1;
> > + u16 cancel_complete:1;
> > + u16 reset_complete:1;
> > + u16 busy:1;
> > + u16 ack_complete:1;
> > + u16 error:1;
> > + u16 cmd_complete:1;
>
> Wondering: Is this endianness clean ?
> Same whereever you use bit fields. Or is the driver
> only expected to work on little-endian systems ?
The idea was to support only support little endian with the driver
initially, but I'll try to improve the structures and by using
something like __BIG/LITTLE_ENDIAN_BITFIELD.
> > +} __packed;
> > +
> > +/* Default fields in CONTROL data structure */
> > +struct ucsi_command {
> > + u8 cmd;
> > + u8 length;
> > + u64 data:48;
> > +} __packed;
>
> Is ucsi_command supppose to be 64 bit long ? If so, does __packed
> truncate the u64 to 48 bit ?
That was the idea, but now that you mentioned it, __packed does not
seem to be needed for it. I'll drop the __packed from this structure.
<snip>
> > +struct ucsi;
> > +
> > +struct ucsi_data {
> > + u16 version;
> > + u16:16; /* reserved */
>
> The :16 seems unnecessary.
OK. I'll fix it and make it "u16 reserved;"
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
