On Tue, 04 Dec 2018, Guenter Roeck wrote: > On Tue, Dec 4, 2018 at 3:52 AM Enric Balletbo i Serra > <enric.balle...@collabora.com> wrote: > > On 4/12/18 10:21, Lee Jones wrote: > > > On Mon, 03 Dec 2018, Enric Balletbo i Serra wrote: > > >> On 3/12/18 11:36, Lee Jones wrote: > > >>> On Tue, 27 Nov 2018, Enric Balletbo i Serra wrote: > > >>> > > >>>> The entire way how cros sysfs attibutes are created is broken. > > >>>> cros_ec_lightbar should be its own driver and its attributes should be > > >>>> associated with a lightbar driver not the mfd driver. In order to > > >>>> retain > > >>>> the path, the lightbar attributes are attached to the cros_class. > > >>> > > >>> I'm not exactly clear on what a lightbar is, but shouldn't it live in > > >>> the appropriate subsystem. Like LED for example? > > >>> > > >> > > >> The lightbar is a four-color indicator available on some Chromebook, but > > >> the > > >> fact that can you can program this lightbar with different sequences, > > >> including > > >> user defined sequences makes the device a bit special and very chrome > > >> platform > > >> specific. The same happens with the VBC driver. > > > > > > Being Chrome specific doesn't necessarily mean that these drivers > > > shouldn't reside in a proper subsystem. A lot of drivers in the > > > kernel are only relevant to very specific hardware/platforms. > > > > Agree, and we try to put these drivers in their subsystem when we think it > > is > > appropriate (we have in rtc, power, iio, keyboard, etc.) > > > > > IMHO code in drivers/platform should pertain only to the core platform > > > itself. Any drivers for leaf hardware/functionality should be split > > > out into the subsystems, however niche you think they are. > > > > > > > To be honest, I don't have a hard opinion yet on if the drivers/platform > > should > > pertain only to the core platform itself. > > > > The cros_ec_lightbar.c file already exists in drivers/platform, the patch > > just > > converts it to a kernel module (only adds few lines). The main purpose of > > the se > > patches was have cros-ec mfd code and their subdrivers separated instead of > > having crossed calls. > > > > I don't mind to move to another subsystem (I need to discuss with the > > chromium > > guys and I am still not sure if LED fits very well in this case, I can look > > in > > more detail) but shouldn't be this a follow up patch? > > Separate patch, please, if anything.
Agreed. > I would not know which subsystem to move this to, though, and moving > it to misc just for the sake of it would seem odd, since this most > definitely _is_ platform related code. What is platform for if not for > platform specific code ? "platform" has very broad and varying meanings in Computer Science nomenclature. Unfortunately, it can be used to describe quite a lot of different aspects of the (I wanted to say platform then) hardware. All drivers are 'platform' device drivers, that's why we have the platform_device_*/platform_driver_* API. Saying that "platform specific code" should live in */platform reminds me of the arch/*/<platform> days when similar arguments were put forward with respect to jamming all "platform specific code" into arch/*. A great deal of effort was put in to relocate it all into the proper subsystems. What I see here is the backslide into the same mind-set. My guess is that defining what 'platform' means in the context of drivers/platform is not going to be straight forward, however; I have always seen it as a place to put *core* platform code, perhaps an API akin to the one used for the EC in the Chromium context is a good example. Drivers required to control peripherals that, for instance, operate a set of LEDs, I would not consider core platform code. In my mind, *any* and *all* leaf and peripheral drivers should be relocated into a proper subsystem. Even if that is drivers/misc. > > I am also worried on how this could affect the current user interface. Well, > > something to look, right. > > No ABI changes, please. Without seeing the way in which a user controls this stuff, it's difficult to comment, but a change in subsystem usually means a shift in control. Particularly if that control is operated via SYSFS. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog