linux-kernel - Re: [PATCH v5 2/4] mfd: Add driver for Embedded Controller found on Acer Iconia Tab A500

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201116102901.GO3718728@dell>
Date:   Mon, 16 Nov 2020 10:29:01 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Dan Murphy <dmurphy@...com>,
        Sebastian Reichel <sre@...nel.org>, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
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