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: cphe...@gmail.com
> Cc: Lucas Stach <l.st...@pengutronix.de>
> Cc: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
> Cc: Lee Jones <lee.jo...@linaro.org>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: Pavel Machek <pa...@ucw.cz>
> Tested-by: Chris Healy <cphe...@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
> Signed-off-by: Andrey Smirnov <andrew.smir...@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)?
> 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.
> + 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.
> + 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.
> + { /* 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.
> +}
> +
> +static void rave_sp_remove(struct serdev_device *serdev)
> +{
> + of_platform_depopulate(&serdev->dev);
> + serdev_device_close(serdev);
> +}
> +
> +MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);
> +#define COMPATIBLE_RAVE_SP_NIU "zii,rave-sp-niu"
> +#define COMPATIBLE_RAVE_SP_MEZZ "zii,rave-sp-mezz"
> +#define COMPATIBLE_RAVE_SP_ESB "zii,rave-sp-esb"
> +#define COMPATIBLE_RAVE_SP_RDU1 "zii,rave-sp-rdu1"
> +#define COMPATIBLE_RAVE_SP_RDU2 "zii,rave-sp-rdu2"
> +
> +#endif /* _LINUX_RAVE_SP_H_ */
Johan
