Re: [RESEND PATCH v7 1/1] platform: Add driver for RAVE Supervisory Processor

[Johan Hovold]

On Fri, Oct 13, 2017 at 08:56:00AM -0700, Andrey Smirnov wrote:
> On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> > On Thu, Oct 12, 2017 at 11:13:21PM -0700, Andrey Smirnov wrote:
> >> Add a driver for RAVE Supervisory Processor, an MCU implementing
> >> varoius bits of housekeeping functionality (watchdoging, backlight
> >> control, LED control, etc) on RAVE family of products by Zodiac
> >> Inflight Innovations.
> >>
> >> This driver implementes core MFD/serdev device as well as
> >> communication subroutines necessary for commanding the device.
> >
> > I only skimmed this, but have a few of comments below.
> >
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Cc: cphealy@xxxxxxxxx
> >> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> >> Cc: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> >> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> Cc: Pavel Machek <pavel@xxxxxx>
> >> Tested-by: Chris Healy <cphealy@xxxxxxxxx>
> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> >> ---
> >>  drivers/platform/Kconfig        |   2 +
> >>  drivers/platform/Makefile       |   1 +
> >>  drivers/platform/rave/Kconfig   |  26 ++
> >>  drivers/platform/rave/Makefile  |   1 +
> >>  drivers/platform/rave/rave-sp.c | 677 ++++++++++++++++++++++++++++++++++++++++
> >
> > First of all, why do these live in drivers/platform and why don't use
> > the mfd subsystem to implement this driver (instead of rolling your own
> > mfd-implementation)?
> >
> 
> Because when I submitted this driver to MFD Lee Jones told me that it
> didn't belong to that subsystem and should be added to the kernel in
> "drivers/platform".
> 
> Could you elaborate more on "instead of rolling your own
> mfd-implementation"? It was my understanding that using "simple-mfd"
> binding and of_platform_default_populate() was the simplest way to
> create a DT-only MFD and that's how the driver was implemented when I
> submitted it for inclusion to MFD as well. Am I re-inventing something
> and is there a simpler way?

AFAICS you're not using "simple-mfd" here. You're implementing an MFD
driver and the corresponding compatible strings were listed in the
binding docs (e.g. "zii,rave-sp-rdu1").

Also note that no drivers under drivers/platforms does anything like
this.

> >> +config RAVE_SP_CORE
> >> +     tristate "RAVE SP MCU core driver"
> >> +     select MFD_CORE
> >
> > And here you select on mfd core even though you currently don't seem to
> > use it at all.
> >
> 
> My bad, for some reason I thought I was using something from
> mfd-core.c, but I don't. Will remove in v8.

> >> +static const struct of_device_id rave_sp_dt_ids[] = {
> >> +     { .compatible = COMPATIBLE_RAVE_SP_NIU,  .data = &rave_sp_legacy },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_legacy },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_ESB,  .data = &rave_sp_legacy },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_rdu1   },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_rdu2   },
> >
> > I think you should use the compatible strings directly here rather than
> > use these defines.
> 
> Those compatible strings are also re-used by cell drivers for this
> device (not a part of this series) to determine which ICD is
> applicable, hence the defines instead of normal strings.

I'd say that is indicative that you're doing something wrong here.
You've already matched your MFD and therefore have all the information
you need. You can pass that data to the child drivers through mfd-cells.

You should submit at least one of your subdrivers along with the core
MFD driver before we merge anything so we can see how things will end up
getting used.

Johan