Messages in this thread | | | Date | Wed, 5 Dec 2018 08:09:34 +0000 | From | Lee Jones <> | Subject | Re: [PATCH v3 2/8] mfd / platform: cros_ec: move lightbar attributes to its own driver. |
| |
On Tue, 04 Dec 2018, Guenter Roeck wrote: > On Tue, Dec 4, 2018 at 3:52 AM Enric Balletbo i Serra > <enric.balletbo@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
| |