Hi Taniya,
On Mon, Apr 2, 2018 at 3:33 AM Taniya Das <tdas@xxxxxxxxxxxxxx> wrote:
> Hello Evan,
> Thanks for the review comments.
> On 3/30/2018 3:19 AM, Evan Green wrote:
> > Hi Taniya,
> >
> > On Wed, Mar 28, 2018 at 11:19 PM Taniya Das <tdas@xxxxxxxxxxxxxx> wrote:
> >
> >> From: Amit Nischal <anischal@xxxxxxxxxxxxxx>
> >> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
> >> +{
> >> + struct tcs_cmd cmd = { 0 };
> >> + int ret = 0;
> >> +
> >> + cmd.addr = c->res_addr + c->res_en_offset;
> >> +
> >> + if (has_state_changed(c, RPMH_SLEEP_STATE)) {
> >> + cmd.data = (c->aggr_state >> RPMH_SLEEP_STATE) & 1
> >> + ? c->res_on_val : c->res_off_val;
> >
> > I found it quite confusing that you're shifting in the opposite
direction
> > than in has_state_changed. How about this:
> >
> > cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE)) ? c->res_on_val :
> > c->res_off_val;
> >
> It is kept keeping in mind the consistency of the command data for all
> the different states.
> Would it be better if I define a new macro ?
> #define RPMH_CMD_DATA(aggr_state, rpmh_state) \
> ((aggr_state >> rpmh_state) & 1)
> If you agree, I could use the new macro in the next series.
I'm afraid I don't really understand the "consistency of command data for
all the different states". It seems like every other time you refer to a
state mask (eg state, valid_state_mask, and all the *_STATE_MASK defines,
you use BIT(state). The version above seemed like an unnecessarily
confusing variant. In this case, aggr_state is only being used as a
conditional to determine whether to put the res_on or off value, so it's
not like that form is needed because of hardware bitfields or something.
All of the state-like members seem to be purely software masks.
Anyway, I'll leave it to you after this. If you choose to keep the shifts
as written and hide them behind a macro, the name as you've suggested isn't
great, as that macro only gives you the conditional for which data to use,
not the data itself as the macro name might have you believe.
-Evan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
