Re: [PATCH 3/3] rsi: Updated boot parameters

[Kalle Valo <kvalo@codeaurora.org>]

Prameela Rani Garnepudi <prameela.j04cs@gmail.com> writes:

> * Switch clock info is divided in to different clock information fields for
>   readability and synchronization with firmware code.
> * Other parameters are added for future use and to make the frame size in sync
>   with latest firmware. Otherwise firmware will discard the frame considering
>   corrupted frame.
>
> Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>

Thanks, this is much easier to review now that the style changes are gone.

>  /* structure to store configs related to UMAC clk programming */
>  struct switch_clk {
> -	__le16 switch_clk_info;
> +	__le16 switch_umac_clk : 1; /* If set rest is valid */
> +	__le16 switch_qspi_clk : 1; /* If set qspi clk will be changed */
> +	__le16 switch_slp_clk_2_32 : 1;
> +	__le16 switch_bbp_lmac_clk_reg : 1;
> +	__le16 switch_mem_ctrl_cfg : 1;
> +	__le16 reserved : 11;

This immediately caugh my eye. Are you sure this works on big endian
machines? I have never seen __le16 mixed with bitfields so I'm skeptical
that this will even work, but I don't have time to really check. Anyone
else know?

Anyway, the original code used the preferred format:

-			.switch_clk_info = cpu_to_le16(BIT(3)),

That is use BIT() or GENMASK()/FIELD_PREP() to create the bitmask in cpu
native format and then convert it with cpu_to_le*() family of functions.

-- 
Kalle Valo