From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751122AbdJMP4F (ORCPT ); Fri, 13 Oct 2017 11:56:05 -0400 Received: from mail-qk0-f195.google.com ([209.85.220.195]:53707 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750716AbdJMP4C (ORCPT ); Fri, 13 Oct 2017 11:56:02 -0400 X-Google-Smtp-Source: ABhQp+T6yQtmQLauVPmWHIjedYChYoP9Xm4+yFg8NSIH2UotSdeIZNu47TEtGgiyZX1EIPD7qz4bB7Il2fo85h2XDMQ= MIME-Version: 1.0 In-Reply-To: <20171013072731.GC29243@localhost> References: <20171013061321.31252-1-andrew.smirnov@gmail.com> <20171013061321.31252-2-andrew.smirnov@gmail.com> <20171013072731.GC29243@localhost> From: Andrey Smirnov Date: Fri, 13 Oct 2017 08:56:00 -0700 Message-ID: Subject: Re: [RESEND PATCH v7 1/1] platform: Add driver for RAVE Supervisory Processor To: Johan Hovold Cc: linux-kernel , Chris Healy , Lucas Stach , Nikita Yushchenko , Lee Jones , Greg Kroah-Hartman , Pavel Machek Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold 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 >> 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)? > 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