Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752332AbdHAUQe (ORCPT ); Tue, 1 Aug 2017 16:16:34 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:33988 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752169AbdHAUQb (ORCPT ); Tue, 1 Aug 2017 16:16:31 -0400 MIME-Version: 1.0 In-Reply-To: <20170801171434.2bc2aa84@bbrezillon> References: <1501518290-5723-1-git-send-email-boris.brezillon@free-electrons.com> <1501518290-5723-3-git-send-email-boris.brezillon@free-electrons.com> <20170731231509.77d1fba4@bbrezillon> <20170801142936.5df48702@bbrezillon> <20170801153414.6ce34ee8@bbrezillon> <20170801155813.4b811a4d@bbrezillon> <20170801171434.2bc2aa84@bbrezillon> From: Arnd Bergmann Date: Tue, 1 Aug 2017 22:16:29 +0200 X-Google-Sender-Auth: 8kRHn9x-ftgyBEwnZ1GgYngPYtY Message-ID: Subject: Re: [RFC 2/5] i3c: Add core I3C infrastructure To: Boris Brezillon Cc: Wolfram Sang , linux-i2c@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Greg Kroah-Hartman , Przemyslaw Sroka , Arkadiusz Golec , Alan Douglas , Bartosz Folta , Damian Kos , Alicja Jurasik-Urbaniak , Jan Kotas , Cyprian Wronka , Alexandre Belloni , Thomas Petazzoni , Nishanth Menon , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , devicetree@vger.kernel.org, Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3116 Lines: 82 On Tue, Aug 1, 2017 at 5:14 PM, Boris Brezillon wrote: > On Tue, 1 Aug 2017 16:22:21 +0200 Arnd Bergmann wrote: >> On Tue, Aug 1, 2017 at 3:58 PM, Boris Brezillon >> wrote: >> > On Tue, 1 Aug 2017 15:34:14 +0200 >> > Boris Brezillon wrote: >> >> On Tue, 1 Aug 2017 15:11:44 +0200 >> >> Arnd Bergmann wrote: >> >> > On Tue, Aug 1, 2017 at 2:29 PM, Boris Brezillon >> >> > wrote: >> > I just realized I forgot to add a "depends on I2C" in the I3C Kconfig >> > entry. Indeed, I'm unconditionally calling functions provided by the >> > I2C framework which have no dummy wrapper when I2C support is disabled. >> > I could of course conditionally compile some portion of the I3C >> > framework so that it still builds when I2C is disabled but I'm not sure >> > it's worth the trouble. >> > >> > This "depends on I2C" should also solve the I2C+I3C driver issue, since >> > I2C is necessarily enabled when I3C is. >> > >> > Am I missing something? >> >> That should solve another part of the problem, as a combined driver then >> just needs 'depends on I3C'. >> >> On top of that, the i3c_driver structure could also contain callback >> pointers for the i2c subsystem, e.g. i2c_probe(), i2c_remove() etc. >> When the i2c_probe() callback exists, the i3c layer could construct >> a 'struct i2c_driver' with those callbacks and register that under the >> cover. This would mean that combined drivers no longer need to >> register two driver objects. > > That should work. Actually, i2c_driver contains a few more hooks, like > ->alert(), ->command() and ->detect(). Of course we could assume that > I3C/I2C drivers do not need them, I was thinking we can add them as they are needed. > but I'm wondering if it's not easier > to just add an i2c_driver pointer inside the i3c_driver struct and let > the driver populate it if it needs to supports both protocols. > > Something like: > > struct i3c_driver { > ... > struct i2c_driver *i2c_compat; > ... > }; > > > and then in I3C/I2C drivers: > > static struct i2c_driver my_i2c_driver = { > ... > }; > > static struct i3c_driver my_i3c_driver = { > ... > .i2c_compat = &my_i2c_driver, > ... > }; > module_i3c_driver(my_i3c_driver); > > > > Of course, you'll have a few fields of ->i2c_compat that would be > filled by the core (like the driver name which can be extracted from > my_i3c_driver->driver.name). Right, that would work too, but it's almost the same as the version you proposed earlier that would use module_i2c_i3c_driver(my_i2c_driver, my_i3c_driver); It's probably a little cleaner this way in the subsystem implementation compared to my suggestion of adding the i2c callback pointers in struct i3c_driver, while that would make the drivers look a little nicer (and save a few lines per driver). Arnd