Messages in this thread | | | Date | Mon, 16 Nov 2020 10:29:01 +0000 | From | Lee Jones <> | Subject | Re: [PATCH v5 2/4] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500 |
| |
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
| |