On Mon, 16 Nov 2020, Dmitry Osipenko wrote:
> ...
> >>>> + while (retries-- > 0) {
> >>>> + ret = i2c_smbus_read_word_data(client, reg);
> >>>> + if (ret >= 0)
> >>>> + break;
> >>>> +
> >>>> + msleep(A500_EC_I2C_ERR_TIMEOUT);
> >>>> + }
> ...
> >>> I'm surprised there isn't a generic function which does this kind of
> >>> read. Seems like pretty common/boilerplate stuff.
> >>
> >> I'm not quite sure that this is a really very common pattern which
> >> deserves a generic helper.
> >
> > What? Read from I2C a few times, then sleep sounds like a pretty
> > common pattern to me.
>
> Maybe the read_poll_timeout() helper could be used for that, but I think
> the open-coded variant is much easier to perceive, don't you agree?
I haven't looked into it. My comment was more general in nature.
As a general rule, helpers are better than open coding, but there are
always exceptions. There may not even be a suitable helper for this
use-case. As I say, the comment was more of a passing remark.
[...]
> >> These are the values which controller's firmware wants to see, otherwise
> >> it will reject command as invalid. I'll add a clarifying comment to the
> >> code.
> >
> > Thanks. Hopefully with a bit more information as to why the firmware
> > expects to see them. Hopefully they're not random.
>
> These are the firmware-defined specific command opcodes, I'll add
> defines for them to make it more clear, thanks.
That'll do.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
