Re: [PATCH v2 2/2] cros_ec: Move cros_ec_dev module to drivers/mfd

[Lee Jones]

On Wed, 20 Jun 2018, Dmitry Torokhov wrote:
> On Mon, Nov 20, 2017 at 8:18 AM Thierry Escande
> <thierry.escande@xxxxxxxxxxxxx> 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.

The rationale for not using the MFD API outside of drivers/mfd is the
avoidance of (mild) chaos, since the aforementioned API lends itself to
abuse if not diligently monitored - preferably by someone who knows
about MFDs.  Contributors regularly attempt to (ab)use the MFD API to
hack around various problems relating to the registration of devices.
Most of which receive a "this is not an MFD" review comment and sent
on their way!

MFD is not like anything else in the kernel, so comparing it bus types
such as SPI and I2C does not carry much weight.  The MFD Subsystem's
job is simple; slice multi-function ICs into separate functional areas
and register the resultant sub-devices whist managing shared
resources.  The parent drivers should all reside in drivers/mfd.

> 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:

AFAICS, there aren't any compelling reasons for the CROS drivers to
continue to reside in drivers/mfd or their use of the MFD API.  It
would be better if they moved into drivers/platform and dropped the
use of the MFD API entirely.  Simply use platform_device_add() in its
place.

> dtor@dtor-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.
> 
> 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.
> 

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog