On Fri, Sep 06, 2019 at 06:20:51PM -0400, Nick Crews wrote: > Thanks for the patch Daniel! A few thoughts that I didn't > have on the review on Gerrit, sorry :) After those changes, > No problem, I will take care of them.
> Reviewed-by: Nick Crews <ncr...@chromium.org> > > > u8 sub_cmd; /* Always SUB_CMD_H1_GPIO */ > > This comment should be removed. > I overlooked this. Done. > This comment should be moved to h1_gpio_get(). > Agree > > -static int h1_gpio_get(void *arg, u64 *val) > > +static int write_to_mailbox(struct wilco_ec_device *ec, u8 sub_cmd, u64 > > *val) > > What about send_ec_cmd() or similar? Something that communicates that > we are sometimes telling the EC to do something, and sometimes reading > something > back. Also, since we are adding in another layer in here, we can fix > the signature from > the one required by a debugfs attribute. Use "ret" instead of "val" > and make it a u8*. > send_ec_cmd() sounds good to me but ret is already used. Will change to out_val as discussed offline. > A one line comment as to what test_event does? > > > +static int test_event_set(void *arg, u64 val) > > +{ > > + u64 ret; > > + > > + return write_to_mailbox(arg, SUB_CMD_TEST_EVENT, &ret); > > +} > > + Will do. Thanks again.