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

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold <johan@kernel.org> 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@vger.kernel.org
>> Cc: cphealy@gmail.com
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> Tested-by: Chris Healy <cphealy@gmail.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  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?

>>  include/linux/rave-sp.h         |  54 ++++
>>  6 files changed, 761 insertions(+)
>>  create mode 100644 drivers/platform/rave/Kconfig
>>  create mode 100644 drivers/platform/rave/Makefile
>>  create mode 100644 drivers/platform/rave/rave-sp.c
>>  create mode 100644 include/linux/rave-sp.h
>>
>> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
>> index c11db8bceea1..e6db685bb895 100644
>> --- a/drivers/platform/Kconfig
>> +++ b/drivers/platform/Kconfig
>> @@ -8,3 +8,5 @@ endif
>>  source "drivers/platform/goldfish/Kconfig"
>>
>>  source "drivers/platform/chrome/Kconfig"
>> +
>> +source "drivers/platform/rave/Kconfig"
>> diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
>> index ca2692510733..17bdec5ece0c 100644
>> --- a/drivers/platform/Makefile
>> +++ b/drivers/platform/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_MIPS)            += mips/
>>  obj-$(CONFIG_OLPC)           += olpc/
>>  obj-$(CONFIG_GOLDFISH)               += goldfish/
>>  obj-$(CONFIG_CHROME_PLATFORMS)       += chrome/
>> +obj-y += rave/
>> diff --git a/drivers/platform/rave/Kconfig b/drivers/platform/rave/Kconfig
>> new file mode 100644
>> index 000000000000..6fc50ade3871
>> --- /dev/null
>> +++ b/drivers/platform/rave/Kconfig
>> @@ -0,0 +1,26 @@
>> +#
>> +# Platform support for Zodiac RAVE hardware
>> +#
>> +
>> +menuconfig RAVE_PLATFORMS
>> +     bool "Platform support for Zodiac RAVE hardware"
>> +     depends on SERIAL_DEV_BUS && SERIAL_DEV_CTRL_TTYPORT
>
> You don't strictly depend on SERIAL_DEV_CTRL_TTYPORT even if I
> understand why you added it (that controller will default to Y when
> serdev is enabled soon).
>
> Also this is not the entry that depends on serdev, RAVE_SP_CORE is the
> driver that depends on it.
>
> I think this one can just be removed, and like for normal mfd drivers,
> have the "child drivers" depend on the mfd driver (e.g. RAVE_SP_CORE)
> below.
>

Sure, I can change that.

>> +     help
>> +       Say Y here to get to see options for platform support for
>> +       various devices present in RAVE hardware. This option alone
>> +       does not add any kernel code.
>> +
>> +       If you say N, all options in this submenu will be skipped
>> +       and disabled.
>> +
>> +if RAVE_PLATFORMS
>> +
>> +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.

>> +     select CRC_CCITT
>> +     help
>> +       Select this to get support for the Supervisory Processor
>> +       device found on several devices in RAVE line of hardware.
>> +
>> +endif
>> diff --git a/drivers/platform/rave/Makefile b/drivers/platform/rave/Makefile
>> new file mode 100644
>> index 000000000000..e4c21ab2d2f5
>> --- /dev/null
>> +++ b/drivers/platform/rave/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
>> diff --git a/drivers/platform/rave/rave-sp.c b/drivers/platform/rave/rave-sp.c
>> new file mode 100644
>> index 000000000000..d2660d0f8e7d
>> --- /dev/null
>> +++ b/drivers/platform/rave/rave-sp.c
>
> [ ignoring the driver implementation ]
>
>> +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.

>> +     { /* sentinel */ }
>> +};
>
>> +static int rave_sp_probe(struct serdev_device *serdev)
>> +{
>> +     struct device *dev = &serdev->dev;
>> +     struct rave_sp *sp;
>> +     u32 baud;
>> +     int ret;
>> +
>> +     if (of_property_read_u32(dev->of_node, "current-speed", &baud)) {
>> +             dev_err(dev,
>> +                     "'current-speed' is not specified in device node\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
>> +     if (!sp)
>> +             return -ENOMEM;
>> +
>> +     sp->serdev = serdev;
>> +     dev_set_drvdata(dev, sp);
>> +
>> +     sp->variant = of_device_get_match_data(dev);
>> +     if (!sp->variant)
>> +             return -ENODEV;
>> +
>> +     mutex_init(&sp->bus_lock);
>> +     mutex_init(&sp->reply_lock);
>> +     BLOCKING_INIT_NOTIFIER_HEAD(&sp->event_notifier_list);
>> +
>> +     serdev_device_set_client_ops(serdev, &rave_sp_serdev_device_ops);
>> +     ret = serdev_device_open(serdev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     serdev_device_set_baudrate(serdev, baud);
>> +
>> +     return of_platform_default_populate(dev->of_node, NULL, dev);
>
> You must close the serdev before returning on errors.
>

Oops, missed this one, thanks. Will fix in v8.

Thanks,
Andrey Smirnov