On Mon, 16 Nov 2020, Dmitry Osipenko wrote:

...

quoted

quoted

quoted

quoted

+	while (retries-- > 0) {
+		ret = i2c_smbus_read_word_data(client, reg);
+		if (ret >= 0)
+			break;
+
+		msleep(A500_EC_I2C_ERR_TIMEOUT);
+	}

...

quoted

quoted

quoted

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.

[...]

quoted

quoted

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