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

[Andrey Smirnov]

On Mon, Oct 16, 2017 at 7:14 AM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> 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").
>

The reason I mentioned "simple-mfd" was because I assumed it was
necessary to use of_platform_default_populate(), which, re-reading the
corresponding code, seems to have been a mistake on my part. However,
now I am not sure I understand the point you are/were trying to make.
The way I understand "instead of rolling your own mfd-implementation"
is "You ignored/missed the precedent and didn't use agreed-upon way of
creating an MFD from DT and re-invented the code to do so", in which
case I'd like to know the canonical way I missed.

Form you response, however, I get a feeling that you are trying to
convince me that the driver I submitted is an MFD driver. If so, then
there's no need, we are in agreement here.

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

I am aware of that. As I as I mentioned before, the decision to place
this code into "drivers/platforms" was not mine.

>> >> +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.

I have all the information I need at this layer, however there are ICD
differences that are local to functionality covered by individual cell
which I wanted to keep local to the corresponding drivers.

> You can pass that data to the child drivers through mfd-cells.
>

I don't need "that" data for individual cell drivers, they have their
own local data sets form which to chose based on the "flavor" of the
parent device. Obviously that "flavor" can be passed as custom data to
child drivers as well, but I don't think that could be done with less
code than just using of_match_device(variants_table, dev->parent)
which is why I chose that route.

As you suggested, I'll add a cell driver to v8 and, hopefully, it'll
make it more clear as to why I have those defines.

Thanks,
Andrey Smirnov