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

[Johan Hovold]

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)?

>  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