linux-kernel - Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver

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: <CAMz4ku+CH5-9Omxr=srJrJM7+_xni1-ZeMTsTJnA+A+ycUarOQ@mail.gmail.com>
Date:   Tue, 29 Aug 2017 09:58:14 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Wolfram Sang <wsa@...-dreams.de>
Cc:     Baolin Wang <baolin.wang@...eadtrum.com>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        andriy.shevchenko@...ux.intel.com, linux-i2c@...r.kernel.org,
        devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver

Hi Wolfram,

On 28 August 2017 at 23:13, Wolfram Sang <wsa@...-dreams.de> wrote:
>
>> >> +     /*
>> >> +      * If we did not get one ACK from slave when writing data, we should
>> >> +      * dump all registers to check I2C status.
>> >
>> > Why? I would say no. NACK from a slave can always happen, e.g. when an
>> > EEPROM is busy erasing a page.
>>
>> For our I2C controller databook, if the master did not get one ACK
>> from slave when writing data to salve, we should send one STOP signal
>> to abort this data transfer or generate one repeated START signal to
>> start one new data transfer cycle. Considering our I2C usage
>
> Yes, so far so good.
>
>> scenarios, we should dump registers to analyze I2C status and notify
>> to user to re-start new data transfer.
>
> I disagree here. You notify the users by returning -EIO. The upper layer
> (e.g. the i2c client driver) will handle it, like the EEPROM driver
> might retry after a while. This all is expected behaviour, so no need to
> print the registers to the logfile.
>
> If you really, really want to keep it, make it debug output. But I think
> the sentence "we should dump all registers" needs to be rephrased.

Make sense. I will remove the registers printing here.

>
>> As I explained before, in our Spreadtrum platform, our regulator
>> driver will depend on I2C driver and the regulator driver uses
>> subsys_initcall() level to initialize. Moreover some other drivers
>> like GPU, they will depend on regulator to set voltage and they also
>> need initialization much earlier.
>>
>> Since it is arch_initcall() level, Andy suggested I should get rid of
>> tristate (use bool) and drop module.h here and all leftovers like
>> MODULE_*() calls including module_exit().
>
> I see. So the driver is really so essential for proper bootup that it is
> not even allowed to be unloaded. I might make an exception here and

Yes.

> allow arch_initcall() then. But I do wonder: did you try deferred
> probing all over the place?

Many modules (like GPU) need set voltage earlier by regulator which
depends on I2C ( or we also need regulate voltage for big-cores ASAP),
if we defer to set voltage for GPU or other modules, maybe will cause
some system problems. Thanks for your comments.

-- 
Baolin.wang
Best Regards