linux-kernel - Re: [PATCH v2 2/2] cros_ec: Move cros_ec

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <bb1bc51a-3ff9-a885-7037-55306d1af564@collabora.com>
Date:   Thu, 21 Jun 2018 10:42:48 +0200
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Lee Jones <lee.jones@...aro.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Guenter Roeck <groeck@...gle.com>,
        lkml <linux-kernel@...r.kernel.org>, thierry.escande@...aro.org,
        Gwendal Grignou <gwendal@...omium.org>
Subject: Re: [PATCH v2 2/2] cros_ec: Move cros_ec_dev module to drivers/mfd

Hi Dmitry,

I think I can answer some of your questions (not all).

cc'ing the new Thierry's address and Gwendal.

On 21/06/18 01:05, Dmitry Torokhov wrote:
> On Mon, Nov 20, 2017 at 8:18 AM Thierry Escande
> <thierry.escande@...labora.com> wrote:
>>
>> The cros_ec_dev module is responsible for registering the MFD devices
>> attached to the ChromeOS EC. This patch moves this module to drivers/mfd
>> so calls to mfd_add_devices() are not done from outside the MFD subtree
>> anymore.
> 
> I am quite a bit late to the party, but what's the rationale for not
> using mfd_add_devices() from outside of MFD tree? We do allow
> registering i2c clients from outside of i2c core, and spi from outside
> of spi core, etc, etc.
> 

What I can say here is that when I tried to use mfd_add_devices outside the mfd
Lee has the preference to not to do this. That's the main reason why we moved
cros_ec_dev to mfd subsystem. The origin of this discussion started here [1] and
iirc after talk with Lee via irc he was not agree to use mfd_add_devices outside
the mfd subsystem (Lee correct me if I am wrong). So, is Lee who can add the
reasoning here.

[1] https://www.spinics.net/lists/kernel/msg2465099.html

> Right now I see cros_ec being split, quite haphazardly, between
> drivers/platform/chrome and drivers/mfd, with some transport drivers
> (i2C, SPI) and some interfaces living in MFD, while LPC transport and
> host of other stuff living in drivers/platform. On top of that we have
> cros_ec_keyb in input, RTC drivers, CEC, and god knows what else
> spread across various subsystems:
> 

Move the transport driver to platform/chrome is on my TODO, I didn't send the
patches to upstream yet, sorry about that.

About having the driver spread across various subsystem I don't have a hard
opinion. I suppose that this is because is the common way for mfd cells,
although, in this case, it is true that the drivers are very ChromeOS specific,
so maybe has sense have all in platform/chrome. On the other side, if you put
all in platform/chrome you will probably lost the advantage of have the
maintainer of the subsytem reviewing the patches (just because he filters the
drivers that doesn't go their subsystem).

> dtor@...r-ws:~/kernel/linus $ find -name 'cros_ec*.c'
> ./drivers/iio/light/cros_ec_light_prox.c
> ./drivers/iio/accel/cros_ec_accel_legacy.c
> ./drivers/iio/pressure/cros_ec_baro.c
> ./drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> ./drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> ./drivers/mfd/cros_ec_spi.c
> ./drivers/mfd/cros_ec.c
> ./drivers/mfd/cros_ec_i2c.c
> ./drivers/mfd/cros_ec_dev.c
> ./drivers/input/keyboard/cros_ec_keyb.c
> ./drivers/platform/chrome/cros_ec_vbc.c
> ./drivers/platform/chrome/cros_ec_debugfs.c
> ./drivers/platform/chrome/cros_ec_lpc.c
> ./drivers/platform/chrome/cros_ec_lightbar.c
> ./drivers/platform/chrome/cros_ec_lpc_reg.c
> ./drivers/platform/chrome/cros_ec_proto.c
> ./drivers/platform/chrome/cros_ec_lpc_mec.c
> ./drivers/platform/chrome/cros_ec_sysfs.c
> 
> The fact that sysfs/debugfs code is in platform but we instantiate it
> from MFD is pure madness (it's driver private data, there is no reason
> why it should be exported to nclude/linux/mfd/cros_ec.h). This all
> creates unnecessary friction and I would love to move most of the code
> into drivers/platform/chrome.
> 

Ack.

> I see the wisdom of having code that could potentially be used in
> several systems in respective subsystems code (pretty much majority of
> drivers/mfd/ drivers are for chips/IP blocks that are used and reused
> by different systems and boards), but I think cros ec is quite
> different in that regard as it is only used by ChromeOS devices and
> has little to no chance to be useful anywhere else.
> 
> Thanks.
> 

Cheers,
 Enric