[{"id":1768472,"web_url":"http://patchwork.ozlabs.org/comment/1768472/","msgid":"<20170914094314.qadkjvc6kup2nchx@dell>","list_archive_url":null,"date":"2017-09-14T09:43:14","subject":"Re: [patch v5 1/2] mfd: Add Mellanox regmap core driver","submitter":{"id":12720,"url":"http://patchwork.ozlabs.org/api/people/12720/","name":"Lee Jones","email":"lee.jones@linaro.org"},"content":"On Thu, 14 Sep 2017, Vadim Pasternak wrote:\n\n> This patch adds core regmap platform driver for Mellanox BMC cards with\n> the programmable devices based chassis control. The device logics,\n> controlled by software includes:\n> - Interrupt handling for chassis, ASIC, CPU events;\n> - LED handling;\n> - Exposes through sysfs mux, reset signals, reset cause notification;\n> The patch provides support for the access to programmable device through\n> the register map and allows dynamic device tree manipulation at runtime\n> for removable devices.\n> \n> This driver requires activator driver, which responsibility is to create\n> register map and pass it to regmap core. Such activator could be based for\n> example on I2C, SPI or MMIO interface.\n> \n> Drivers exposes the number of hwmon attributes to sysfs according to the\n> attribute groups, defined in the device tree. These attributes will be\n> located for example in /sys/class/hwmon/hwmonX folder, which is a symbolic\n> link to for example:\n> /sys/bus/i2c/devices/4-0072/mlxreg-core.1138/hwmon/hwmon10.\n> The attributes are divided to the groups, like in the below example:\n> MUX nodes\n> - safe_bios_disable\n> - spi_burn_bios_ci\n> - spi_burn_ncsi\n> - uart_sel\n> Reset nodes:\n> - sys_power_cycle\n> - bmc_upgrade\n> - asic_reset\n> Cause of reset nodes:\n> - cpu_kernel_panic\n> - cpu_shutdown\n> - bmc_warm_reset\n> General purpose RW nodes:\n> - version\n> \n> Drivers also probes LED and hotplug drivers, in case device tree\n> description contains LED and hotplug nodes.\n> \n> Signed-off-by: Vadim Pasternak \n> ---\n> v4->v5:\n> Comments pointed out by Lee:\n> - Remove mlxreg-i2c module from the patchset;\n> Changes added by Vadim:\n> - Remove include/linux/platform_data/mlxreg.h from the patcheset, since\n> it have been sent with\n> - Modify dts parsing routines;\n> - Disable compliation of_update_property if CONFIG_COMPILE_TEST is set;\n> v3->v4:\n> Comments pointed out by Lee:\n> - Split interrupt related logic into separate module and place this logic\n> within the existing Mellanox hotplug driver. Move for this reason this\n> driver from drivers/platform/x86 to drivers/platform/mellanox folder;\n> Comments pointed out by Rob:\n> - Make a separate patch /devicetree/bindings/vendor-prefixes.txt;\n> - Add .txt to Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core\n> and send it within this series;\n> - Modify \"compatible\" property;\n> - Modify explanation for \"deferred\" property;\n> - Describe each subnode by its own section;\n> - Don't use underscore in attribute names;\n> Changes added by Vadim:\n> - Add mlxreg_hotplug_device and mlxreg_core_item structures to mlxreg.h\n> and modify mlxreg_core_led_platform_data structure;\n> v2->v3:\n> Changes added by Vadim:\n> - Change name of field led data in struct mlxreg_core_led_platform_data\n> in mlxreg.h;\n> - fix data position assignment in mlxreg_core_get;\n> - update name of field led data in mlxreg_core_set_led;\n> v1->v2:\n> Comments pointed out by Pavel:\n> - Remove extra new line in mellanox,mlxreg-core;\n> - Replace three NOT with one in mlxreg_core_attr_show;\n> - Make error message in mlxreg_core_work_helper shorter;\n> - Make attribute assignment more readable;\n> - Separate mellanox,mlxreg-core file for sending to DT maintainers.\n> Comments pointed out by Jacek:\n> - Since brightness_set_blocking is used instead of\n> brightness_set, three fields from mlxreg_core_led_data are removed.\n> ---\n> MAINTAINERS | 6 +\n> drivers/mfd/Kconfig | 14 +\n> drivers/mfd/Makefile | 1 +\n> drivers/mfd/mlxreg-core.c | 843 ++++++++++++++++++++++++++++++++++++++++++++++\n> 4 files changed, 864 insertions(+)\n> create mode 100644 drivers/mfd/mlxreg-core.c\n\nI thought we agreed that this was not an MFD driver?\n\nIf it doesn't use the MFD framework, it's not an MFD driver.\n\nLooking at it very briefly, it appears at though it should actually be\nsplit up. In your commit message you say that this is a platform\ndriver that does LED handling and deals with the regmap and HWMON\nsubsystem.\n\nI suggest moving the core code into /drivers/platform/* and split the\nother sections into their own drivers within their own subsystems.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"dB6Rz4gR\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtD9j2xM6z9s7g\n\tfor ;\n\tThu, 14 Sep 2017 19:43:21 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751648AbdINJnT (ORCPT\n\t);\n\tThu, 14 Sep 2017 05:43:19 -0400","from mail-wm0-f48.google.com ([74.125.82.48]:46089 \"EHLO\n\tmail-wm0-f48.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751509AbdINJnS (ORCPT\n\t); Thu, 14 Sep 2017 05:43:18 -0400","by mail-wm0-f48.google.com with SMTP id i189so9106493wmf.1\n\tfor ;\n\tThu, 14 Sep 2017 02:43:17 -0700 (PDT)","from dell ([2.27.167.120]) by smtp.gmail.com with ESMTPSA id\n\to14sm14592228wra.54.2017.09.14.02.43.15\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 14 Sep 2017 02:43:16 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=X/RgEmMSweHY/CxZ/qR3F3poWCOy6CIBlpeJXT6GdIs=;\n\tb=dB6Rz4gREdM+/IX35DXV5cM/yfrRDSGwMpRwflK8hOmmTMfjOxBJQOUwiO63o1AxAU\n\tHc53cG8qdHl3FYcomLVvx8lLYu162iK09xchZu8izmMez2q8E5mEkC+CJt9we7/Pr368\n\tPGWerEyKVxjmy3bX7pIdTefCF3LDvc0HxGMHw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=X/RgEmMSweHY/CxZ/qR3F3poWCOy6CIBlpeJXT6GdIs=;\n\tb=d9MzxtmvU8ptFuze5ADZEX4Vk0cBaLWRmZThPzCVAZrBO6LDZUre+5/ocOZ5qMEOqu\n\tFkyqUR/6EjL0VqMRf3WMacge9Js4XnNY2PjVqsxNxV8i0zRMY3pLT0d/WMzAbBI2C3iS\n\tB/Uxi/Pxtzfto8LUhsnODHLdRzgtKzAyBCuWnrtAtYCPObWAwkLOle714Q7Irz0ANYdL\n\tu/aa4jd7Wlu/myR9CpZuG+drtmndNT5JvB1EyvJVnMKSBZg2jZk//DLyP91jZ30jrjax\n\tu7IyxiWSItK+c8o41j9G/BGMzMr0spa8Q1i0ym61aLq8p983gOlqKRoo9Ankb2OMV8pb\n\tZEcQ==","X-Gm-Message-State":"AHPjjUgogOMBuRb3lzlt0N/qOaAGzOmxvc9mOfc6kcEbkAQzYk/loGQV\n\tTsmEwiz10fe0LNmI","X-Google-Smtp-Source":"AOwi7QBv9KRnwNRCgKG74VKNT/XKFsVetUQvvjJWwW62N9W7SauiDXsPljQmP8C7txIpe4gY8XgQFA==","X-Received":"by 10.28.153.206 with SMTP id b197mr1510069wme.60.1505382197069; \n\tThu, 14 Sep 2017 02:43:17 -0700 (PDT)","Date":"Thu, 14 Sep 2017 10:43:14 +0100","From":"Lee Jones ","To":"Vadim Pasternak ","Cc":"robh+dt@kernel.org, pavel@ucw.cz, devicetree@vger.kernel.org,\n\tjacek.anaszewski@gmail.com, linux-leds@vger.kernel.org,\n\tjiri@resnulli.us, gregkh@linuxfoundation.org,\n\tandy.shevchenko@gmail.com, platform-driver-x86@vger.kernel.org","Subject":"Re: [patch v5 1/2] mfd: Add Mellanox regmap core driver","Message-ID":"<20170914094314.qadkjvc6kup2nchx@dell>","References":"<1505371738-128683-1-git-send-email-vadimp@mellanox.com>\n\t<1505371738-128683-2-git-send-email-vadimp@mellanox.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1505371738-128683-2-git-send-email-vadimp@mellanox.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1768552,"web_url":"http://patchwork.ozlabs.org/comment/1768552/","msgid":"<20170914121219.bgk77tbv7w3qaay6@dell>","list_archive_url":null,"date":"2017-09-14T12:12:19","subject":"Re: [patch v5 1/2] mfd: Add Mellanox regmap core driver","submitter":{"id":12720,"url":"http://patchwork.ozlabs.org/api/people/12720/","name":"Lee Jones","email":"lee.jones@linaro.org"},"content":"On Thu, 14 Sep 2017, Vadim Pasternak wrote:\n\n> \n> > > v4->v5:\n> > > Comments pointed out by Lee:\n> > > - Remove mlxreg-i2c module from the patchset; Changes added by\n> > > Vadim:\n> > > - Remove include/linux/platform_data/mlxreg.h from the patcheset, since\n> > > it have been sent with\n> > > - Modify dts parsing routines;\n> > > - Disable compliation of_update_property if CONFIG_COMPILE_TEST is\n> > > set;\n> > > v3->v4:\n> > > Comments pointed out by Lee:\n> > > - Split interrupt related logic into separate module and place this logic\n> > > within the existing Mellanox hotplug driver. Move for this reason this\n> > > driver from drivers/platform/x86 to drivers/platform/mellanox\n> > > folder; Comments pointed out by Rob:\n> > > - Make a separate patch /devicetree/bindings/vendor-prefixes.txt;\n> > > - Add .txt to Documentation/devicetree/bindings/mfd/mellanox,mlxreg-\n> > core\n> > > and send it within this series;\n> > > - Modify \"compatible\" property;\n> > > - Modify explanation for \"deferred\" property;\n> > > - Describe each subnode by its own section;\n> > > - Don't use underscore in attribute names; Changes added by Vadim:\n> > > - Add mlxreg_hotplug_device and mlxreg_core_item structures to mlxreg.h\n> > > and modify mlxreg_core_led_platform_data structure;\n> > > v2->v3:\n> > > Changes added by Vadim:\n> > > - Change name of field led data in struct mlxreg_core_led_platform_data\n> > > in mlxreg.h;\n> > > - fix data position assignment in mlxreg_core_get;\n> > > - update name of field led data in mlxreg_core_set_led;\n> > > v1->v2:\n> > > Comments pointed out by Pavel:\n> > > - Remove extra new line in mellanox,mlxreg-core;\n> > > - Replace three NOT with one in mlxreg_core_attr_show;\n> > > - Make error message in mlxreg_core_work_helper shorter;\n> > > - Make attribute assignment more readable;\n> > > - Separate mellanox,mlxreg-core file for sending to DT maintainers.\n> > > Comments pointed out by Jacek:\n> > > - Since brightness_set_blocking is used instead of\n> > > brightness_set, three fields from mlxreg_core_led_data are removed.\n> > > ---\n> > > MAINTAINERS | 6 +\n> > > drivers/mfd/Kconfig | 14 +\n> > > drivers/mfd/Makefile | 1 +\n> > > drivers/mfd/mlxreg-core.c | 843\n> > > ++++++++++++++++++++++++++++++++++++++++++++++\n> > > 4 files changed, 864 insertions(+)\n> > > create mode 100644 drivers/mfd/mlxreg-core.c\n> > \n> > I thought we agreed that this was not an MFD driver?\n> \n> Your comment was regarding mlxreg-i2c module, which I removed from the\n> patchset. So I was under impression that we agreed about mlx reg-i2c, but not\n> about mlxreg-core.\n> \n> > \n> > If it doesn't use the MFD framework, it's not an MFD driver.\n> > \n> > Looking at it very briefly, it appears at though it should actually be split up.\n> > In your commit message you say that this is a platform driver that does LED\n> > handling and deals with the regmap and HWMON subsystem.\n> > \n> > I suggest moving the core code into /drivers/platform/* and split the other\n> > sections into their own drivers within their own subsystems.\n> \n> According to your suggestion for patch v2 IC related code was splitted in v3,\n> while LED driver has been separated in initial patchset version.\n> Which other sections you suggested to split?\n> There is now hwmon driver functionality within mlxreg-core. It expose some\n> Registers with, f.e. reset and reset cause info to the user space through sysfs. \n> Also regmap is just the common interface, which is used by driver.\n> \n> This driver, LED, IC and as I wrote before, there is a plan to extend it with WD\n> and PWM drivers work against the same programmable device.\n> So at the moment I provided two child devices for led and platform subsystems\n> and planned to add two more for hwmon or pwm and watchdog subsystems.\n> \n> And all of them uses the same register space, accessible through regmap\n> Interface.\n> Based on the above I considered such driver as MFD.\n> \n> Why do you think that drivers/platform is more suitable location, is it because\n> I am using for subsystem drivers activation platform_device_add and not\n> mfd_add_devices?\n> Or there are some others considerations?\n\nAfter a closer look, it seems as though some of it could be converted\ninto an MFD driver, but my prediction is that it's going to require\nalmost a complete re-write and we're going to need to re-work it over\nseveral submissions.\n\nThe first thing you need to do is convert all of the hand rolled\nplatform device registration over to the MFD API.\n\nThe rest of the code is a mess. It's over-complicated and mostly\nunreadable. What is it that you're trying to achieve? What does the\ndevice do and how does it function? Do you have a datasheet that\nyou're working from?","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"KKtc16yG\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtHTm3nFLz9sRg\n\tfor ;\n\tThu, 14 Sep 2017 22:12:28 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751860AbdINMM0 (ORCPT\n\t);\n\tThu, 14 Sep 2017 08:12:26 -0400","from mail-wr0-f175.google.com ([209.85.128.175]:49600 \"EHLO\n\tmail-wr0-f175.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751620AbdINMMZ (ORCPT\n\t); Thu, 14 Sep 2017 08:12:25 -0400","by mail-wr0-f175.google.com with SMTP id u96so140843wrb.6\n\tfor ;\n\tThu, 14 Sep 2017 05:12:24 -0700 (PDT)","from dell ([2.27.167.120]) by smtp.gmail.com with ESMTPSA id\n\t77sm1891651wmx.10.2017.09.14.05.12.21\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 14 Sep 2017 05:12:21 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=/5g+aWW8HRSfTbkTLnSlev7UJyxO8IHNCvtDVx2oR9U=;\n\tb=KKtc16yGaupyF3a99YyCfNH9yLjFaHHGwbyZKd4DKBbd3YEq/otF9+UWzxuUdl9Jlu\n\tkoBHU4oA3iS5zuV5Q9sn1yD1XRmCWRAyz5rR1Y5hI5dObkPRYvHG/QXEuCsfXpZ44DiQ\n\twA9eyKqt6jXj2E5eY2G4IoiM9sW96cSgjlkYA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=/5g+aWW8HRSfTbkTLnSlev7UJyxO8IHNCvtDVx2oR9U=;\n\tb=hC+kfUddJMEfiVx8rOc3jk8uwQydU7LCxWTDS+4AR2uguUKIsbFIyAtsTeJrBcgFbo\n\ttD9zJA3xKGnC28mmfcYxlCXHQ2bAUiFje2eJEF7wdU3OTFtPpVLV9Taxgs1NJHOTWNxe\n\tov8tyCZ8Ys2mfv4UXlJ7HOmOdoloLdlDxyfCeTFFdBtxtP88soXhPBXSI7Nsq09Y5cPA\n\tVZPgVdtNiTaLzuXhlM/2AOJOOeAqPw3GXwAqJBUw+aLJT0wsKB5xkVAGFfA8BjTQ39hv\n\tnZRdeD/Ufti3Lmb412d8gE0j7CPGV8ZiqyW3U6+WLI7U6LO0t8LunQvDK/nftmVcAddE\n\tAa5A==","X-Gm-Message-State":"AHPjjUgHPG4yusC8jfDOTaG/Bjs6yIbHfJq9LdFceMys62LUvIVidjw3\n\to+LNmY+rO/nBWcjt","X-Google-Smtp-Source":"ADKCNb7dgilFMTtUDzI2lZ4aZOIVUbF4QahqWxlBgyAR2NRJ/1AWTeEXjEeQjntndTrfZWHk1+pIbQ==","X-Received":"by 10.223.175.217 with SMTP id\n\ty25mr20268381wrd.255.1505391144136; \n\tThu, 14 Sep 2017 05:12:24 -0700 (PDT)","Date":"Thu, 14 Sep 2017 13:12:19 +0100","From":"Lee Jones ","To":"Vadim Pasternak ","Cc":"\"robh+dt@kernel.org\" , \"pavel@ucw.cz\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"jacek.anaszewski@gmail.com\" ,\n\t\"linux-leds@vger.kernel.org\" ,\n\t\"jiri@resnulli.us\" ,\n\t\"gregkh@linuxfoundation.org\" ,\n\t\"andy.shevchenko@gmail.com\" ,\n\t\"platform-driver-x86@vger.kernel.org\" \n\t","Subject":"Re: [patch v5 1/2] mfd: Add Mellanox regmap core driver","Message-ID":"<20170914121219.bgk77tbv7w3qaay6@dell>","References":"<1505371738-128683-1-git-send-email-vadimp@mellanox.com>\n\t<1505371738-128683-2-git-send-email-vadimp@mellanox.com>\n\t<20170914094314.qadkjvc6kup2nchx@dell>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]