Message-ID: <4497bb80-42e3-ca02-5f41-9c66186ace99@roeck-us.net>
Date:   Sun, 18 Sep 2016 23:02:33 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     vadimp@...lanox.com, tglx@...utronix.de
Cc:     mingo@...hat.com, hpa@...or.com, davem@...emloft.net,
        geert@...ux-m68k.org, akpm@...ux-foundation.org,
        gregkh@...uxfoundation.org, kvalo@...eaurora.org,
        mchehab@...nel.org, x86@...nel.org, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, jiri@...nulli.us
Subject: Re: [patch v2] x86/platform/mellanox: introduce support for Mellanox
 systems platform

On 09/15/2016 02:55 AM, vadimp@...lanox.com wrote:
> From: Vadim Pasternak <vadimp@...lanox.com>
>
> Enable system support for the Mellanox Technologies platform, which
> provides support for the next Mellanox basic systems: "msx6710",
> "msx6720", "msb7700", "msn2700", "msx1410", "msn2410", "msb7800",
> "msn2740", "msn2100" and also various number of derivative systems from
> the above basic types.
>
> The Kconfig currently controlling compilation of this code is:
> arch/x86/platform:config MLX_PLATFORM
> arch/x86/platform:      tristate "Mellanox Technologies platform support"
>
> Signed-off-by: Vadim Pasternak <vadimp@...lanox.com>
> ---
> v1->v2:
>  Comments pointed out by Greg:
>   - kick out all PCI related code;
> ---
>  MAINTAINERS                               |   7 +
>  arch/x86/Kconfig                          |  25 +++
>  arch/x86/platform/Makefile                |   1 +
>  arch/x86/platform/mellanox/Makefile       |   1 +
>  arch/x86/platform/mellanox/mlx-platform.c | 275 ++++++++++++++++++++++++++++++
>  5 files changed, 309 insertions(+)
>  create mode 100644 arch/x86/platform/mellanox/Makefile
>  create mode 100644 arch/x86/platform/mellanox/mlx-platform.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4705c94..8a675de 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7664,6 +7664,13 @@ W:	http://www.mellanox.com
>  Q:	http://patchwork.ozlabs.org/project/netdev/list/
>  F:	drivers/net/ethernet/mellanox/mlxsw/
>
> +MELLANOX PLATFORM DRIVER
> +M:      Vadim Pasternak <vadimp@...lanox.com>
> +L:      platform-driver-x86@...r.kernel.org
> +S:      Supported
> +W:      http://www.mellanox.com
> +F:      arch/x86/platform/mellanox/mlx-platform.c
> +
>  SOFT-ROCE DRIVER (rxe)
>  M:	Moni Shoua <monis@...lanox.com>
>  L:	linux-rdma@...r.kernel.org
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c580d8c..1346441 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -550,6 +550,31 @@ config X86_INTEL_QUARK
>  	  Say Y here if you have a Quark based system such as the Arduino
>  	  compatible Intel Galileo.
>
> +config MLX_PLATFORM
> +	tristate "Mellanox Technologies platform support"
> +	depends on X86_64
> +	depends on X86_EXTENDED_PLATFORM
> +	depends on PCI

No longer needed.

> +	depends on DMI
> +	depends on I2C_MLXCPLD
> +	depends on I2C_MUX_REG
> +	select HWMON
> +	select PMBUS
> +	select SENSORS_PMBUS
> +	select LM75
> +	select NEW_LEDS
> +	select LEDS_CLASS
> +	select LEDS_TRIGGERS
> +	select LEDS_TRIGGER_TIMER
> +	select LEDS_MLXCPLD

Kind of unusual to select drivers not directly related to this one like this.

> +	---help---
> +	  This option enables system support for the Mellanox Technologies
> +	  platform.
> +
> +	  Say Y here if you are building a kernel for Mellanox system.
> +
> +	  Otherwise, say N.
> +
>  config X86_INTEL_LPSS
>  	bool "Intel Low Power Subsystem Support"
>  	depends on X86 && ACPI
> diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
> index 184842e..3c3c19e 100644
> --- a/arch/x86/platform/Makefile
> +++ b/arch/x86/platform/Makefile
> @@ -8,6 +8,7 @@ obj-y	+= iris/
>  obj-y	+= intel/
>  obj-y	+= intel-mid/
>  obj-y	+= intel-quark/
> +obj-y	+= mellanox/
>  obj-y	+= olpc/
>  obj-y	+= scx200/
>  obj-y	+= sfi/
> diff --git a/arch/x86/platform/mellanox/Makefile b/arch/x86/platform/mellanox/Makefile
> new file mode 100644
> index 0000000..f43c931
> --- /dev/null
> +++ b/arch/x86/platform/mellanox/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_MLX_PLATFORM)	+= mlx-platform.o
> diff --git a/arch/x86/platform/mellanox/mlx-platform.c b/arch/x86/platform/mellanox/mlx-platform.c
> new file mode 100644
> index 0000000..d29da68
> --- /dev/null
> +++ b/arch/x86/platform/mellanox/mlx-platform.c
> @@ -0,0 +1,275 @@
> +/*
> + * arch/x86/platform/mellanox/mlx-platform.c
> + * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
> + * Copyright (c) 2016 Vadim Pasternak <vadimp@...lanox.com>
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the names of the copyright holders nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dmi.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/i2c-mux-reg.h>
> +
> +#define MLX_PLAT_DEVICE_NAME		"mlxplat"
> +
> +/* LPC bus IO offsets */
> +#define MLXPLAT_CPLD_LPC_I2C_BASE_ADRR		0x2000
> +#define MLXPLAT_CPLD_LPC_REG_BASE_ADRR		0x2500
> +#define MLXPLAT_CPLD_LPC_IO_RANGE		0x100
> +#define MLXPLAT_CPLD_LPC_I2C_CH1_OFF		0xdb
> +#define MLXPLAT_CPLD_LPC_I2C_CH2_OFF		0xda
> +#define MLXPLAT_CPLD_LPC_PIO_OFFSET		0x10000UL
> +#define MLXPLAT_CPLD_LPC_REG1	((MLXPLAT_CPLD_LPC_REG_BASE_ADRR + \
> +				  MLXPLAT_CPLD_LPC_I2C_CH1_OFF) | \
> +				  MLXPLAT_CPLD_LPC_PIO_OFFSET)
> +#define MLXPLAT_CPLD_LPC_REG2	((MLXPLAT_CPLD_LPC_REG_BASE_ADRR + \
> +				  MLXPLAT_CPLD_LPC_I2C_CH2_OFF) | \
> +				  MLXPLAT_CPLD_LPC_PIO_OFFSET)
> +
> +/* Start channel numbers */
> +#define MLXPLAT_CPLD_CH1			2
> +#define MLXPLAT_CPLD_CH2			10
> +
> +/* Number of LPC attached MUX platform devices */
> +#define MLXPLAT_CPLD_LPC_MUX_DEVS		2
> +
> +/* mlxplat_priv - platform private data
> + * @pdev_i2c - i2c controller platform device
> + * @pdev_mux - array of mux platform devices
> + */
> +struct mlxplat_priv {
> +	struct platform_device *pdev_i2c;
> +	struct platform_device *pdev_mux[MLXPLAT_CPLD_LPC_MUX_DEVS];
> +};
> +
> +/* Regions for LPC I2C controller and LPC base register space */
> +static struct resource mlxplat_lpc_resources[] = {
> +	[0] = DEFINE_RES_NAMED(MLXPLAT_CPLD_LPC_I2C_BASE_ADRR,
> +			       MLXPLAT_CPLD_LPC_IO_RANGE,
> +			       "mlxplat_cpld_lpc_i2c_ctrl", IORESOURCE_IO),
> +	[1] = DEFINE_RES_NAMED(MLXPLAT_CPLD_LPC_REG_BASE_ADRR,
> +			       MLXPLAT_CPLD_LPC_IO_RANGE,
> +			       "mlxplat_cpld_lpc_regs",
> +			       IORESOURCE_IO),
> +};
> +
> +/* Platform default channels */
> +static int mlxplat_default_channels[][8] = {
> +	{
> +		MLXPLAT_CPLD_CH1, MLXPLAT_CPLD_CH1 + 1, MLXPLAT_CPLD_CH1 + 2,
> +		MLXPLAT_CPLD_CH1 + 3, MLXPLAT_CPLD_CH1 + 4, MLXPLAT_CPLD_CH1 +
> +		5, MLXPLAT_CPLD_CH1 + 6, MLXPLAT_CPLD_CH1 + 7
> +	},
> +	{
> +		MLXPLAT_CPLD_CH2, MLXPLAT_CPLD_CH2 + 1, MLXPLAT_CPLD_CH2 + 2,
> +		MLXPLAT_CPLD_CH2 + 3, MLXPLAT_CPLD_CH2 + 4, MLXPLAT_CPLD_CH2 +

Odd line split.

> +		+ 5, MLXPLAT_CPLD_CH2 + 6, MLXPLAT_CPLD_CH2 + 7
> +	},
> +};
> +
> +/* Platform channels for MSN21xx system family */
> +static int mlxplat_msn21xx_channels[][8] = {
> +	{
> +		1, 2, 3, 4, 5, 6, 7, 8
> +	},
> +	{
> +		1, 2, 3, 4, 5, 6, 7, 8
> +	},

Seems to be a waste. A single dimensional array should be sufficient.

> +};
> +
> +/* Platform mux data */
> +struct i2c_mux_reg_platform_data mlxplat_mux_data[] = {

Why global ?

> +	{
> +		.parent = 1,
> +		.base_nr = MLXPLAT_CPLD_CH1,
> +		.write_only = 1,
> +		.reg = (void __iomem *)MLXPLAT_CPLD_LPC_REG1,
> +		.reg_size = 1,
> +		.idle_in_use = 1,
> +	},
> +	{
> +		.parent = 1,
> +		.base_nr = MLXPLAT_CPLD_CH2,
> +		.write_only = 1,
> +		.reg = (void __iomem *)MLXPLAT_CPLD_LPC_REG2,
> +		.reg_size = 1,
> +		.idle_in_use = 1,
> +	},
> +
> +};
> +
> +struct platform_device *mlxplat_dev;
> +

Is this global on purpose ? If so, why ?

> +static int __init mlxplat_dmi_default_matched(const struct dmi_system_id *dmi)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mlxplat_mux_data); i++) {
> +		mlxplat_mux_data[i].values = mlxplat_default_channels[i];
> +		mlxplat_mux_data[i].n_values =
> +				ARRAY_SIZE(mlxplat_default_channels[i]);
> +	}
> +
> +	return 1;
> +};
> +
> +static int __init mlxplat_dmi_msn21xx_matched(const struct dmi_system_id *dmi)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mlxplat_mux_data); i++) {
> +		mlxplat_mux_data[i].values = mlxplat_msn21xx_channels[i],
> +		mlxplat_mux_data[i].n_values =
> +				ARRAY_SIZE(mlxplat_msn21xx_channels[i]);
> +	}
> +
> +	return 1;
> +};
> +
> +static struct dmi_system_id mlxplat_dmi_table[] __initdata = {
> +	{
> +		.callback = mlxplat_dmi_default_matched,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox Technologies"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MSN24"),
> +		},
> +	},
> +	{
> +		.callback = mlxplat_dmi_default_matched,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox Technologies"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MSN27"),
> +		},
> +	},
> +	{
> +		.callback = mlxplat_dmi_default_matched,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox Technologies"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MSB"),
> +		},
> +	},
> +	{
> +		.callback = mlxplat_dmi_default_matched,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox Technologies"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MSX"),
> +		},
> +	},
> +	{
> +		.callback = mlxplat_dmi_msn21xx_matched,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox Technologies"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MSN21"),
> +		},
> +	},
> +	{ }
> +};
> +
> +static int __init mlxplat_init(void)
> +{
> +	struct mlxplat_priv *priv;
> +	int i;
> +	int err;
> +
> +	if (!dmi_check_system(mlxplat_dmi_table))
> +		return -ENODEV;
> +
> +	mlxplat_dev = platform_device_register_simple(MLX_PLAT_DEVICE_NAME, -1,
> +					mlxplat_lpc_resources,
> +					ARRAY_SIZE(mlxplat_lpc_resources));
> +
> +	if (!mlxplat_dev) {
> +		pr_err("Alloc %s platform device failed\n",
> +			MLX_PLAT_DEVICE_NAME);

Error messages for memory allocations are unnecessary.

> +		return -ENOMEM;
> +	}
> +
> +	priv = devm_kzalloc(&mlxplat_dev->dev, sizeof(struct mlxplat_priv),
> +			    GFP_KERNEL);
> +	if (!priv) {
> +		err = -ENOMEM;
> +		dev_err(&mlxplat_dev->dev, "Failed to allocate mlxplat_priv\n");

Same here.

> +		goto fail_alloc;
> +	}
> +	platform_set_drvdata(mlxplat_dev, priv);
> +
> +	priv->pdev_i2c = platform_device_register_simple("i2c_mlxcpld", -1,
> +							 NULL, 0);
> +	if (IS_ERR(priv->pdev_i2c)) {
> +		err = PTR_ERR(priv->pdev_i2c);
> +		goto fail_alloc;
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(mlxplat_mux_data); i++) {
> +		priv->pdev_mux[i] = platform_device_register_resndata(
> +						&mlxplat_dev->dev,
> +						"i2c-mux-reg", i, NULL,
> +						0, &mlxplat_mux_data[i],
> +						sizeof(mlxplat_mux_data[i]));
> +		if (IS_ERR(priv->pdev_mux[i])) {
> +			err = PTR_ERR(priv->pdev_mux[i]);
> +			goto fail_platform_mux_register;
> +		}
> +	}
> +
> +	return err;

return 0;

> +
> +fail_platform_mux_register:
> +	for (i--; i > 0 ; i--)

 >= 0 ?

> +		platform_device_unregister(priv->pdev_mux[i]);
> +	platform_device_unregister(priv->pdev_i2c);
> +fail_alloc:
> +	platform_device_unregister(mlxplat_dev);
> +
> +	return err;
> +}
> +
> +static void __exit mlxplat_exit(void)
> +{
> +	struct mlxplat_priv *priv = platform_get_drvdata(mlxplat_dev);
> +	int i;
> +
> +	for (i = ARRAY_SIZE(mlxplat_mux_data) - 1; i >= 0 ; i--)
> +		platform_device_unregister(priv->pdev_mux[i]);
> +
> +	platform_device_unregister(priv->pdev_i2c);
> +	platform_device_unregister(mlxplat_dev);
> +}
> +
> +module_init(mlxplat_init);
> +module_exit(mlxplat_exit);
> +
> +MODULE_AUTHOR("Vadim Pasternak (vadimp@...lanox.com)");
> +MODULE_DESCRIPTION("Mellanox platform driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:mlx-platform");
>
Should this possibly be a dmi module alias ?

Powered by blists - more mailing lists