Re: [PATCH v2 1/3] mfd: cros_ec: Add USBPD charger commands and struct definitions.

[Enric Balletbo i Serra]

Hi Lee,

Thanks for the reviews.

On 01/05/18 10:29, Lee Jones wrote:
> On Mon, 30 Apr 2018, Enric Balletbo i Serra wrote:
> 
>> From: Sameer Nanda <snanda@xxxxxxxxxxxx>
>>
>> The USBPD charger driver gets information from the ChromeOS EC, this
>> patch adds the USBPD charger definitions needed by this driver.
>>
>> Signed-off-by: Sameer Nanda <snanda@xxxxxxxxxxxx>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> ---
>>
>> Changes in v2: None
>>
>>  include/linux/mfd/cros_ec_commands.h | 132 ++++++++++++++++++++++++++-
>>  1 file changed, 128 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
>> index f2edd9969b40..94dcf331796c 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
> 
> [...]
> 
>> +struct ec_response_pd_log {
>> +	uint32_t timestamp; /* relative timestamp in milliseconds */
>> +	uint8_t type;       /* event type : see PD_EVENT_xx below */
>> +	uint8_t size_port;  /* [7:5] port number [4:0] payload size in bytes */
>> +	uint16_t data;      /* type-defined data payload */
>> +	uint8_t payload[0]; /* optional additional data payload: 0..16 bytes */
>> +} __packed;
> 
> I think this whole file should be converted to Kerneldoc format.
> 

Ok, is something I can do, let me do this in a separate patchset though.

> [...]
> 
>> +/*
>> + * PD_EVENT_MCU_CHARGE event definition :
>> + * the payload is "struct usb_chg_measures"
>> + * the data field contains the port state flags as defined below :
>> + */
>> +/* Port partner is a dual role device */
>> +#define CHARGE_FLAGS_DUAL_ROLE         (1 << 15)
> 
> BIT()?
> 

Changed in next version, there are also other places (not in this patch) where
the BIT() macro is not used. I'll send separate patchset to change it and to
convert to kerneldoc format.

>> +/* Port is the pending override port */
>> +#define CHARGE_FLAGS_DELAYED_OVERRIDE  (1 << 14)
>> +/* Port is the override port */
>> +#define CHARGE_FLAGS_OVERRIDE          (1 << 13)
> 
> [...]
>