From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756606AbdJMH1c (ORCPT ); Fri, 13 Oct 2017 03:27:32 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:46823 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751551AbdJMH1b (ORCPT ); Fri, 13 Oct 2017 03:27:31 -0400 X-Google-Smtp-Source: ABhQp+QTQeKZSqohvHV6xiZQkERiimtcW81fKwlgQlbVzyw5NC6QTuXDjphBEJf2imRnaTrUP2hw+A== Date: Fri, 13 Oct 2017 09:27:31 +0200 From: Johan Hovold To: Andrey Smirnov Cc: linux-kernel@vger.kernel.org, cphealy@gmail.com, Lucas Stach , Nikita Yushchenko , Lee Jones , Greg Kroah-Hartman , Pavel Machek Subject: Re: [RESEND PATCH v7 1/1] platform: Add driver for RAVE Supervisory Processor Message-ID: <20171013072731.GC29243@localhost> References: <20171013061321.31252-1-andrew.smirnov@gmail.com> <20171013061321.31252-2-andrew.smirnov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171013061321.31252-2-andrew.smirnov@gmail.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Nikita Yushchenko > Cc: Lee Jones > Cc: Greg Kroah-Hartman > Cc: Pavel Machek > Tested-by: Chris Healy > Reviewed-by: Andy Shevchenko > Signed-off-by: Andrey Smirnov > --- > 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