On Wed, Jun 14, 2017 at 2:34 PM, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote:
> Amitkumar Karwar <amitkarwar@xxxxxxxxx> writes:
>
>> From: Prameela Rani Garnepudi <prameela.j04cs@xxxxxxxxx>
>>
>> After successful loading of firmware, a CARD READY indication is
>> received by host. Common device configuration parameters are sent
>> to the device after this. It includes information like device
>> operating mode (Wi-Fi alone or BT coex), power save related
>> parameters, GPIO information etc. As device supports BT coex,
>> this frame is send in COEX queue initially. Based on the operating
>> mode, CARD READY indication is received from each protocol module
>> in firmware i.e. WLAN, BT.
>>
>> Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@xxxxxxxxx>
>> Signed-off-by: Amitkumar Karwar <amit.karwar@xxxxxxxxxxxxxxxxxx>
>
> [...]
>
>> +struct rsi_ulp_gpio_vals {
>> +#ifdef __LITTLE_ENDIAN
>> + u8 motion_sensor_gpio_ulp_wakeup:1;
>> + u8 sleep_ind_from_device:1;
>> + u8 ulp_gpio_2:1;
>> + u8 push_button_ulp_wakeup:1;
>> + u8 reserved:4;
>> +#else
>> + u8 reserved:4;
>> + u8 push_button_ulp_wakeup:1;
>> + u8 ulp_gpio_2:1;
>> + u8 sleep_ind_from_device:1;
>> + u8 motion_sensor_gpio_ulp_wakeup:1;
>> +#endif
>> +} __packed;
>
> This is something I'm not exactly thrilled to see (bitfields are not
> really liked in upstream community) but I'm not going to reject that
> either as we have other offenders, rtl8xxxx being one. But it's much
> better that to __le_to_cpu(), FIELD_GET() & co. That you don't need to
> duplicate the fields and is less error prone.
Got it. I will get rid of bitfields in structure and __LITTLE_ENDIAN
macro usage.
>
>> +struct rsi_config_vals {
>> +#ifdef __LITTLE_ENDIAN
>> + u16 len:12;
>> + u16 q_no:4;
>> +#else
>> + u16 q_no:4;
>> + u16 len:12;
>> +#endif
>> + u8 pkt_type;
>> + u8 misc_flags;
>> + __le16 reserved1[6];
>> + u8 lp_ps_handshake;
>> + u8 ulp_ps_handshake;
>> + u8 sleep_config_params; /* 0 for no handshake,
>> + * 1 for GPIO based handshake,
>> + * 2 packet handshake
>> + */
>> + u8 unused_ulp_gpio;
>> + u32 unused_soc_gpio_bitmap;
>> + u8 ext_pa_or_bt_coex_en;
>> + u8 opermode;
>> + u8 wlan_rf_pwr_mode;
>> + u8 bt_rf_pwr_mode;
>> + u8 zigbee_rf_pwr_mode;
>> + u8 driver_mode;
>> + u8 region_code;
>> + u8 antenna_sel_val;
>> + u8 reserved2[16];
>> +} __packed;
>
> But here you are even mixing use of __LITTLE_ENDIAN and __le16, that's
> just weird. And shouldn't unused_soc_gpio_bitmap be __le32?
>
You are right. unused_soc_gpio_bitmap should have been __le32.
I will correct this and get rid of __LITTLE_ENDIAN in updated version
Regards,
Amitkumar
