linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Benson Leung <bleung@chromium.org>
To: Martin Nordholts <enselic@gmail.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>,
	Olof Johansson <olof@lixom.net>,
	Yufeng Shen <miletus@chromium.org>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/1] Platform: x86: chromeos_laptop - convert to i2c_driver to handle i915 race
Date: Sun, 4 Aug 2013 20:52:26 -0700	[thread overview]
Message-ID: <CANLzEksSsMY2pTYTfGzgntF-i_Kus+WeVPyNzW0U7APcGOv_5g@mail.gmail.com> (raw)
In-Reply-To: <1374163091-4750-1-git-send-email-enselic@gmail.com>

Hi Martin,

Thank you for looking into this problem. Obviously, this is a problem
Olof and our team has been working on as well.

On Thu, Jul 18, 2013 at 8:58 AM,  <enselic@gmail.com> wrote:
> From: Martin Nordholts <enselic@gmail.com>
>
> Hello,
>
> I have looked into solving the "touchpad/touchscreen not working on
> boot"-problem on Chromebook Pixel, see for example Brock Tice's last
> comment on https://plus.google.com/+LinusTorvalds/posts/1iFsBWBqoYY
>
> I have seen Olof's attempt at a fix:
>
>   [PATCH] Platform: x86: chromeos_laptop: defer probing if no i2c busses found
>   https://lkml.org/lkml/2013/4/18/556
>
> At first, I tried Olofs suggestion at the end of the thread, namely to
> do bus_register_notifier() on i2c_bus_type, but it is a bit awkward:
> How do you know that the i2c_adapter you are notified about is ready
> to be interacted with? While investigating this, I noticed that the
> i2c_driver has a .detect() mechanism, which is exactly what we need: A
> callback when a newly added i2c_adapter is initialized and ready to be
> used.

I would be hesitant to use .detect(), as its use will have unintended
side effects. I'll give you a real world example of what can happen
with your driver.

Here I've run the i2cdetect command on an actual Chromebook platform.
The name of the bus is SMBus I801 adapter at 0400, so it will be
triggered by your detect.

localhost ~ # i2cdetect -y 8
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- 35 -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- -- -- -- 4a 4b -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

Your driver will try to instantiate device 0x44, the isl29018 ambient
light sensor, device 0x4a, an Atmel touchscreen, and device 0x4b, an
Atmel touchpad.

The problem is that none of those devices actually exist on this
particular system's SMBus.
These are other i2c clients that just happen to share the same
addresses as devices on other buses on other supported systems. 0x44,
for example, is the Intel PCH SMBus's host Receive Slave Address
Register. It just happens to have the same address as the isl29018.

Your driver would result in three failed probes in 2 different drivers
on my board.

Furthermore, I would recommend reading the documentation on
instantiating i2c devices :
https://www.kernel.org/doc/Documentation/i2c/instantiating-devices

The documentation outlines the four methods that are supported for
instantiating i2c devices. "Detect" is method #3, but it comes with a
stern warning : Once again, method 3 should be avoided wherever
possible. Explicit device instantiation (methods 1 and 2) is much
preferred for it is safer and faster.

>
> Since all chromeos_laptop does is to instantiate i2c devices, I
> figured it would be nice to simply convert chromeos_laptop into a
> i2c_driver. The result is the patch in the next mail (based on
> v3.11-rc1).

That is actually not true. The intention is for chromeos_laptop to
eventually instantiate devices other than i2c devices, actually. The
chromeos-kernel version of this driver actually supports the
Chromebook Pixel's keyboard backlight, which is not an i2c device but
a platform device, so making this driver a platform driver is more
appropriate.

https://gerrit.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.git;a=blob;f=drivers/platform/x86/chromeos_laptop.c;hb=refs/heads/chromeos-3.8

>
> I have gone through the SubmitChecklist and done everything that
> applied. I have however only tested this patch on the Chromebook
> Pixel, as that is what I have. I will need help with verifying the
> patch on other Chromebooks.

As I mentioned before, the patch causes problems on other Chromebook
systems I have tested with devices with colliding addresses on SMBus.

>
> But first, what do you think about my patch?
>
> / Martin
>
> Martin Nordholts (1):
>   Platform: x86: chromeos_laptop - convert to i2c_driver to handle i915
>     race
>
>  drivers/platform/x86/chromeos_laptop.c |  426 +++++++++++++++-----------------
>  1 file changed, 194 insertions(+), 232 deletions(-)
>
> --
> 1.7.10.4
>



--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

  parent reply	other threads:[~2013-08-05  3:52 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-18 15:58 [PATCH 0/1] Platform: x86: chromeos_laptop - convert to i2c_driver to handle i915 race enselic
2013-07-18 15:58 ` [PATCH 1/1] " enselic
2013-08-05  3:52 ` Benson Leung [this message]
2013-08-05  5:32   ` [PATCH 0/1] " Martin Nordholts
2013-08-07 21:55     ` Benson Leung
2013-08-11 19:54       ` Martin Nordholts
2013-08-11 22:44         ` Benson Leung

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANLzEksSsMY2pTYTfGzgntF-i_Kus+WeVPyNzW0U7APcGOv_5g@mail.gmail.com \
    --to=bleung@chromium.org \
    --cc=enselic@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew.garrett@nebula.com \
    --cc=miletus@chromium.org \
    --cc=olof@lixom.net \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).