Re: [patch v2] drivers/platform/x86: introduce support for Mellanox hotplug driver

[Darren Hart]

On Thu, Oct 06, 2016 at 04:22:19PM +0000, vadimp@xxxxxxxxxxxx wrote:
> From: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> 
> Enable system support for the Mellanox Technologies hotplug platform
> driver, 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.
> This driver handles hot-plug events for the power suppliers, power
> cables and fans for the above systems.
> 
> The Kconfig currently controlling compilation of this code is:
> driver/platform/x86:config MLX_CPLD_PLATFORM
>                        tristate "Mellanox platform hotplug driver support"
> 
> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> ---
> v1->v2:
>  Comments pointed out by Darren:
>  - remove whitespaces in config;
>  - remove redundant includes for header files;
>  - fix comments styling;
>  - fix register value assignment in mlxcpld_hotplug_attr_show;
>  - mlxcpld_hotplug_attr_init - use local aliases to reduce line length;
>  - Add fields description in the struct mlxcpld_hotplug_device;

Thanks for the update Vadim. The driver appears well-formed in general. I still
have two concerns I'd like to see addressed:

1) Your comment blocks are 95% compliant with kernel-doc, but not quite. This
isn't strictly necessary for C files, but it really is a good idea for header
files, and since it's so close - please just use kernel-doc rather than almost
using it. Comments inline, but start with /** and describe fields with

 * @field: Description

2) The PSU and PWR event device create/destroy blocks are *really* deep and make
them unnecessarily difficult to read. They need to be refactored. Just breaking
out the for loops into their own functions will make it much cleaner and there
is not need to be concerned about minor performance issues with something like
this, but even if there was, we can ensure they are inlined.

> ---
>  MAINTAINERS                                   |   7 +
>  drivers/platform/x86/Kconfig                  |  10 +
>  drivers/platform/x86/Makefile                 |   1 +
>  drivers/platform/x86/mlxcpld-hotplug.c        | 527 ++++++++++++++++++++++++++
>  include/linux/platform_data/mlxcpld-hotplug.h |  92 +++++
>  5 files changed, 637 insertions(+)
>  create mode 100644 drivers/platform/x86/mlxcpld-hotplug.c
>  create mode 100644 include/linux/platform_data/mlxcpld-hotplug.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a306795..cbfe2a8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7649,6 +7649,13 @@ W:	http://www.mellanox.com
>  Q:	http://patchwork.ozlabs.org/project/netdev/list/
>  F:	drivers/net/ethernet/mellanox/mlxsw/
>  
> +MELLANOX MLX CPLD HOTPLUG DRIVER
> +M:	Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> +L:	platform-driver-x86@xxxxxxxxxxxxxxx
> +S:	Supported
> +F:	drivers/platform/x86/mlxcpld-hotplug.c
> +F:	include/linux/platform_data/mlxcpld-hotplug.h
> +
>  SOFT-ROCE DRIVER (rxe)
>  M:	Moni Shoua <monis@xxxxxxxxxxxx>
>  L:	linux-rdma@xxxxxxxxxxxxxxx
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 81b8dcc..c92e2f8 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1026,4 +1026,14 @@ config INTEL_TELEMETRY
>  	  used to get various SoC events and parameters
>  	  directly via debugfs files. Various tools may use
>  	  this interface for SoC state monitoring.
> +
> +config MLX_CPLD_PLATFORM
> +	tristate "Mellanox platform hotplug driver support"
> +	default n
> +	depends on MLX_PLATFORM
> +	select I2C
> +	---help---
> +	  This driver handles hot-plug events for the power suppliers, power
> +	  cables and fans on the wide range Mellanox IB and Ethernet systems.
> +
>  endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 2efa86d..1f06b63 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -71,3 +71,4 @@ obj-$(CONFIG_INTEL_TELEMETRY)	+= intel_telemetry_core.o \
>  				   intel_telemetry_pltdrv.o \
>  				   intel_telemetry_debugfs.o
>  obj-$(CONFIG_INTEL_PMC_CORE)    += intel_pmc_core.o
> +obj-$(CONFIG_MLX_CPLD_PLATFORM)	+= mlxcpld-hotplug.o
> diff --git a/drivers/platform/x86/mlxcpld-hotplug.c b/drivers/platform/x86/mlxcpld-hotplug.c
> new file mode 100644
> index 0000000..f57b4a6
> --- /dev/null
> +++ b/drivers/platform/x86/mlxcpld-hotplug.c
> @@ -0,0 +1,527 @@
> +/*
> + * drivers/platform/x86/mlxcpld-hotplug.c
> + * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
> + * Copyright (c) 2016 Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> + *
> + * 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/bitops.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/mlxcpld-hotplug.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
> +
> +/* Offset of event and mask registers from status register */
> +#define MLXCPLD_HOTPLUG_EVENT_OFF	1
> +#define MLXCPLD_HOTPLUG_MASK_OFF	2
> +#define MLXCPLD_HOTPLUG_AGGR_MASK_OFF	1
> +
> +#define MLXCPLD_HOTPLUG_ATTRS_NUM	8
> +
> +enum mlxcpld_hotplug_attr_type {
> +	MLXCPLD_HOTPLUG_ATTR_TYPE_PSU,
> +	MLXCPLD_HOTPLUG_ATTR_TYPE_PWR,
> +	MLXCPLD_HOTPLUG_ATTR_TYPE_FAN,
> +};
> +
> +/*
> + * mlxcpld_hotplug_priv_data - platform private data:
> + * @irq - platform interrupt number;
> + * @pdev - platform device;
> + * @plat - platform data;
> + * @hwmon - hwmon device;
> + * @mlxcpld_hotplug_attr - sysfs attributes array;
> + * @mlxcpld_hotplug_dev_attr - sysfs sensor device attribute array;
> + * @group - sysfs attribute group;
> + * @groups - list of sysfs attribute group for hwmon registration;
> + * @dwork - delayed work template;
> + * @lock - spin lock;
> + * @aggr_cache - last value of aggregation register status;
> + * @psu_cache - last value of PSU register status;
> + * @pwr_cache - last value of power register status;
> + * @fan_cache - last value of FAN register status;
> + */

If we're going through the effort to document these, we should use kernel-doc
format. See Documentation/kernel-documentation.rst:

Writing kernel-doc comments:Structure, union, and enumeration documentation

> +struct mlxcpld_hotplug_priv_data {
> +	int irq;
> +	struct platform_device *pdev;
> +	struct mlxcpld_hotplug_platform_data *plat;
> +	struct device *hwmon;
> +	struct attribute *mlxcpld_hotplug_attr[MLXCPLD_HOTPLUG_ATTRS_NUM + 1];
> +	struct sensor_device_attribute_2
> +			mlxcpld_hotplug_dev_attr[MLXCPLD_HOTPLUG_ATTRS_NUM];
> +	struct attribute_group group;
> +	const struct attribute_group *groups[2];
> +	struct delayed_work dwork;
> +	spinlock_t lock;
> +	u8 aggr_cache;
> +	u8 psu_cache;
> +	u8 pwr_cache;
> +	u8 fan_cache;
> +};
> +
> +static ssize_t mlxcpld_hotplug_attr_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mlxcpld_hotplug_priv_data *priv = platform_get_drvdata(pdev);
> +	int index = to_sensor_dev_attr_2(attr)->index;
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	u8 reg_val = 0;
> +
> +	switch (nr) {
> +	case MLXCPLD_HOTPLUG_ATTR_TYPE_PSU:
> +		/* Bit = 0 : PSU is present. */
> +		reg_val = !!!(inb(priv->plat->psu_reg_offset) & BIT(index));
> +		break;
> +
> +	case MLXCPLD_HOTPLUG_ATTR_TYPE_PWR:
> +		/* Bit = 1 : power cable is attached. */
> +		reg_val = !!(inb(priv->plat->pwr_reg_offset) & BIT(index %
> +						priv->plat->pwr_count));
> +		break;
> +
> +	case MLXCPLD_HOTPLUG_ATTR_TYPE_FAN:
> +		/* Bit = 0 : FAN is present. */
> +		reg_val = !!!(inb(priv->plat->fan_reg_offset) & BIT(index %
> +						priv->plat->fan_count));
> +		break;
> +	}
> +
> +	return sprintf(buf, "%u\n", reg_val);
> +}
> +
> +#define PRIV_ATTR(i) priv->mlxcpld_hotplug_attr[i]
> +#define PRIV_DEV_ATTR(i) priv->mlxcpld_hotplug_dev_attr[i]
> +static int mlxcpld_hotplug_attr_init(struct mlxcpld_hotplug_priv_data *priv)
> +{
> +	int num_attrs = priv->plat->psu_count + priv->plat->pwr_count +
> +			priv->plat->fan_count;
> +	int i;
> +
> +	priv->group.attrs = devm_kzalloc(&priv->pdev->dev, num_attrs *
> +					 sizeof(struct attribute *),
> +					 GFP_KERNEL);
> +	if (!priv->group.attrs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_attrs; i++) {
> +		PRIV_ATTR(i) = &PRIV_DEV_ATTR(i).dev_attr.attr;
> +
> +		if (i < priv->plat->psu_count) {
> +			PRIV_ATTR(i)->name = devm_kasprintf(&priv->pdev->dev,
> +						GFP_KERNEL, "psu%u", i + 1);
> +			PRIV_DEV_ATTR(i).nr = MLXCPLD_HOTPLUG_ATTR_TYPE_PSU;
> +		} else if (i < priv->plat->psu_count + priv->plat->pwr_count) {
> +			PRIV_ATTR(i)->name = devm_kasprintf(&priv->pdev->dev,
> +						GFP_KERNEL, "pwr%u", i %
> +						priv->plat->pwr_count + 1);
> +			PRIV_DEV_ATTR(i).nr = MLXCPLD_HOTPLUG_ATTR_TYPE_PWR;
> +		} else {
> +			PRIV_ATTR(i)->name = devm_kasprintf(&priv->pdev->dev,
> +						GFP_KERNEL, "fan%u", i %
> +						priv->plat->fan_count + 1);
> +			PRIV_DEV_ATTR(i).nr = MLXCPLD_HOTPLUG_ATTR_TYPE_FAN;
> +		}
> +
> +		if (!PRIV_ATTR(i)->name) {
> +			dev_err(&priv->pdev->dev, "Memory allocation failed for sysfs attribute %d.\n",
> +				i + 1);
> +			return -ENOMEM;
> +		}
> +
> +		PRIV_DEV_ATTR(i).dev_attr.attr.name = PRIV_ATTR(i)->name;
> +		PRIV_DEV_ATTR(i).dev_attr.attr.mode = S_IRUGO;
> +		PRIV_DEV_ATTR(i).dev_attr.show = mlxcpld_hotplug_attr_show;
> +		PRIV_DEV_ATTR(i).index = i;
> +		sysfs_attr_init(&PRIV_DEV_ATTR(i).dev_attr.attr);
> +	}
> +
> +	priv->group.attrs = priv->mlxcpld_hotplug_attr;
> +	priv->groups[0] = &priv->group;
> +	priv->groups[1] = NULL;
> +
> +	return 0;
> +}
> +
> +static int mlxcpld_hotplug_device_create(struct device *dev,
> +					 struct mlxcpld_hotplug_device *item)
> +{
> +	item->adapter = i2c_get_adapter(item->bus);
> +	if (!item->adapter) {
> +		dev_err(dev, "Failed to get adapter for bus %d\n",
> +			item->bus);
> +		return -EFAULT;
> +	}
> +
> +	item->client = i2c_new_device(item->adapter, &item->brdinfo);
> +	if (!item->client) {
> +		dev_err(dev, "Failed to create client %s at bus %d at addr 0x%02x\n",
> +			item->brdinfo.type, item->bus, item->brdinfo.addr);
> +		i2c_put_adapter(item->adapter);
> +		item->adapter = NULL;
> +		return -EFAULT;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mlxcpld_hotplug_device_destroy(struct mlxcpld_hotplug_device *item)
> +{
> +	if (item->client) {
> +		i2c_unregister_device(item->client);
> +		item->client = NULL;
> +	}
> +
> +	if (item->adapter) {
> +		i2c_put_adapter(item->adapter);
> +		item->adapter = NULL;
> +	}
> +}
> +
> +/*
> + * mlxcpld_hotplug_work_handler - performs traversing of CPLD interrupt
> + * registers according to the below hierarchy schema:
> + *
> + *                   Aggregation registers (status/mask)
> + * PSU registers:           *---*
> + * *-----------------*      |   |
> + * |status/event/mask|----->| * |
> + * *-----------------*      |   |
> + * Power registers:         |   |
> + * *-----------------*      |   |
> + * |status/event/mask|----->| * |---> CPU
> + * *-----------------*      |   |
> + * FAN registers:
> + * *-----------------*      |   |
> + * |status/event/mask|----->| * |
> + * *-----------------*      |   |
> + *                          *---*
> + * In case some system changed are detected: FAN in/out, PSU in/out, power
> + * cable attached/detached, relevant device is created or destroyed.
> + */
> +static void mlxcpld_hotplug_work_handler(struct work_struct *work)
> +{
> +	struct mlxcpld_hotplug_priv_data *priv = container_of(work,
> +				struct mlxcpld_hotplug_priv_data, dwork.work);
> +	u8 val, aggr_asserted, asserted;
> +	unsigned long flags;
> +	int bit;
> +
> +	/* Mask aggregation event. */
> +	outb(0, priv->plat->top_aggr_offset + MLXCPLD_HOTPLUG_AGGR_MASK_OFF);
> +	/* Read aggregation status. */
> +	val = inb(priv->plat->top_aggr_offset);
> +	val &= priv->plat->top_aggr_mask;

This, or the example below can both be argued for, but be consistent (assign and
mask on the same line or split).

> +	aggr_asserted = priv->aggr_cache ^ val;
> +	priv->aggr_cache = val;
> +
> +	if (aggr_asserted & priv->plat->top_aggr_psu_mask) {
> +		/* Mask psu presense event. */
> +		outb(0, priv->plat->psu_reg_offset + MLXCPLD_HOTPLUG_MASK_OFF);
> +		/* Read psu presense status. */
> +		val = inb(priv->plat->psu_reg_offset) & priv->plat->psu_mask;
> +		asserted = priv->psu_cache ^ val;
> +		priv->psu_cache = val;
> +		if (priv->plat->psu) {
> +			for_each_set_bit(bit, (unsigned long *)&asserted, 8) {
> +				if (val & BIT(bit))
> +					mlxcpld_hotplug_device_destroy(
> +							priv->plat->psu + bit);
> +				else
> +					mlxcpld_hotplug_device_create(
> +							&priv->pdev->dev,
> +							priv->plat->psu + bit);
> +			}
> +		}

This is still really hard to read, please refactor.

Is it valid to receive a PSU presense event and then fail the if
(priv->plat->psu) condition? If not, can you treat that as an error condition?

Worst case, you can put the for_each_set_bit in a separate function responsible
for managing PSU events. Same below for PWR events.

		if (priv->plat->psu)
			mlxcpld_psu_event_handler(priv, asserted)

Or similar.

> +
> +		/* Acknowledge psu presense event. */
> +		outb(0, priv->plat->psu_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +		/* Unmask psu presense event. */
> +		outb(priv->plat->psu_mask, priv->plat->psu_reg_offset +
> +						MLXCPLD_HOTPLUG_MASK_OFF);
> +
> +	}
> +
> +	if (aggr_asserted & priv->plat->top_aggr_pwr_mask) {
> +		/* Mask power cable event. */
> +		outb(0, priv->plat->pwr_reg_offset + MLXCPLD_HOTPLUG_MASK_OFF);
> +		/* Read power cable status. */
> +		val = inb(priv->plat->pwr_reg_offset) & priv->plat->pwr_mask;
> +		asserted = priv->pwr_cache ^ val;
> +		priv->pwr_cache = val;
> +
> +		if (priv->plat->pwr) {
> +			for_each_set_bit(bit, (unsigned long *)&asserted, 8) {
> +				if (val & BIT(bit))
> +					mlxcpld_hotplug_device_create(
> +							&priv->pdev->dev,
> +							priv->plat->pwr + bit);
> +				else
> +					mlxcpld_hotplug_device_destroy(
> +							priv->plat->pwr + bit);
> +			}
> +		}
> +
> +		/* Acknowledge power cable event. */
> +		outb(0, priv->plat->pwr_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +		/* Unmask power cable event */
> +		outb(priv->plat->pwr_mask, priv->plat->pwr_reg_offset +
> +						MLXCPLD_HOTPLUG_MASK_OFF);
> +
> +	}
> +
> +	if (aggr_asserted & priv->plat->top_aggr_fan_mask) {
> +		/* Mask fan presense event. */
> +		outb(0, priv->plat->fan_reg_offset + MLXCPLD_HOTPLUG_MASK_OFF);
> +		/* Read fan presense status. */
> +		val = inb(priv->plat->fan_reg_offset) & priv->plat->fan_mask;
> +		asserted = priv->fan_cache ^ val;
> +		priv->fan_cache = val;
> +
> +		if (priv->plat->fan) {
> +			for_each_set_bit(bit, (unsigned long *)&asserted, 8) {
> +				if (val & BIT(bit))
> +					mlxcpld_hotplug_device_destroy(
> +							priv->plat->fan + bit);
> +				else
> +					mlxcpld_hotplug_device_create(
> +							&priv->pdev->dev,
> +							priv->plat->fan + bit);
> +			}
> +		}
> +
> +		/* Acknowledge fan presense event. */
> +		outb(0, priv->plat->fan_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +		/* Unmask fan presense event. */
> +		outb(priv->plat->fan_mask, priv->plat->fan_reg_offset +
> +						MLXCPLD_HOTPLUG_MASK_OFF);
> +
> +		/* Unmask aggregation event (no need acknowledge). */
> +		outb(priv->plat->top_aggr_mask, priv->plat->top_aggr_offset +
> +						MLXCPLD_HOTPLUG_AGGR_MASK_OFF);
> +
> +
> +		spin_lock_irqsave(&priv->lock, flags);
> +
> +		/*
> +		 * It is possible, that some signals have been inserted, while
> +		 * interrupt has been masked by mlxcpld_hotplug_work_handler.
> +		 * In this case such signals will be missed. In order to handle
> +		 * these signals delayed work is canceled and work task
> +		 * re-scheduled for immediate execution. It allows to handle
> +		 * missed signals, if any. In other case work handler just
> +		 * validates that no new signals have been received during
> +		 * masking.
> +		 */
> +		cancel_delayed_work(&priv->dwork);
> +		schedule_delayed_work(&priv->dwork, 0);
> +
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +
> +		return;
> +	}
> +
> +	/* Unmask aggregation event (no need acknowledge). */
> +	outb(priv->plat->top_aggr_mask, priv->plat->top_aggr_offset +
> +						MLXCPLD_HOTPLUG_AGGR_MASK_OFF);
> +}
> +
> +static void mlxcpld_hotplug_set_irq(struct mlxcpld_hotplug_priv_data *priv)
> +{
> +	/* Clear psu presense event. */
> +	outb(0, priv->plat->psu_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +	/* Set psu initial status as mask and unmask psu event. */
> +	priv->psu_cache = priv->plat->psu_mask;
> +	outb(priv->plat->psu_mask, priv->plat->psu_reg_offset +
> +						MLXCPLD_HOTPLUG_MASK_OFF);
> +
> +	/* Clear power cable event. */
> +	outb(0, priv->plat->pwr_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +	/* Keep power initial status as zero and unmask power event. */
> +	outb(priv->plat->pwr_mask, priv->plat->pwr_reg_offset +
> +						MLXCPLD_HOTPLUG_MASK_OFF);
> +
> +	/* Clear fan presense event. */
> +	outb(0, priv->plat->fan_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +	/* Set fan initial status as mask and unmask fan event. */
> +	priv->fan_cache = priv->plat->fan_mask;
> +	outb(priv->plat->fan_mask, priv->plat->fan_reg_offset +
> +						MLXCPLD_HOTPLUG_MASK_OFF);
> +
> +	/* Keep aggregation initial status as zero and unmask events. */
> +	outb(priv->plat->top_aggr_mask, priv->plat->top_aggr_offset +
> +						MLXCPLD_HOTPLUG_AGGR_MASK_OFF);
> +
> +	/* Invoke work handler for initializing hot plug devices setting. */
> +	mlxcpld_hotplug_work_handler(&priv->dwork.work);
> +
> +	enable_irq(priv->irq);
> +}
> +
> +static void mlxcpld_hotplug_unset_irq(struct mlxcpld_hotplug_priv_data *priv)
> +{
> +	int i;
> +
> +	disable_irq(priv->irq);
> +	cancel_delayed_work_sync(&priv->dwork);
> +
> +	/* Mask aggregation event. */
> +	outb(0, priv->plat->top_aggr_offset + MLXCPLD_HOTPLUG_AGGR_MASK_OFF);
> +
> +	/* Mask psu presense event. */
> +	outb(0, priv->plat->psu_reg_offset + MLXCPLD_HOTPLUG_MASK_OFF);
> +	/* Clear psu presense event. */
> +	outb(0, priv->plat->psu_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +
> +	/* Mask power cable event. */
> +	outb(0, priv->plat->pwr_reg_offset + MLXCPLD_HOTPLUG_MASK_OFF);
> +	/* Clear power cable event. */
> +	outb(0, priv->plat->pwr_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +
> +	/* Mask fan presense event. */
> +	outb(0, priv->plat->fan_reg_offset + MLXCPLD_HOTPLUG_MASK_OFF);
> +	/* Clear fan presense event. */
> +	outb(0, priv->plat->fan_reg_offset + MLXCPLD_HOTPLUG_EVENT_OFF);
> +
> +	/* Remove all the attached devices. */
> +	for (i = 0; i < priv->plat->psu_count; i++)
> +		mlxcpld_hotplug_device_destroy(priv->plat->psu + i);
> +
> +	for (i = 0; i < priv->plat->pwr_count; i++)
> +		mlxcpld_hotplug_device_destroy(priv->plat->pwr + i);
> +
> +	for (i = 0; i < priv->plat->fan_count; i++)
> +		mlxcpld_hotplug_device_destroy(priv->plat->fan + i);
> +}
> +
> +static irqreturn_t mlxcpld_hotplug_irq_handler(int irq, void *dev)
> +{
> +	struct mlxcpld_hotplug_priv_data *priv =
> +				(struct mlxcpld_hotplug_priv_data *)dev;
> +
> +	/* Schedule work task for immediate execution.*/
> +	schedule_delayed_work(&priv->dwork, 0);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mlxcpld_hotplug_probe(struct platform_device *pdev)
> +{
> +	struct mlxcpld_hotplug_platform_data *pdata;
> +	struct mlxcpld_hotplug_priv_data *priv;
> +	int err;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Failed to get platform data.\n");
> +		return -EINVAL;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->pdev = pdev;
> +	priv->plat = pdata;
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get platform irq: %d\n",
> +			priv->irq);
> +		return priv->irq;
> +	}
> +
> +	err = devm_request_irq(&pdev->dev, priv->irq,
> +				mlxcpld_hotplug_irq_handler, 0, pdev->name,
> +				priv);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to request irq: %d\n", err);
> +		return err;
> +	}
> +	disable_irq(priv->irq);
> +
> +	INIT_DELAYED_WORK(&priv->dwork, mlxcpld_hotplug_work_handler);
> +	spin_lock_init(&priv->lock);
> +
> +	err = mlxcpld_hotplug_attr_init(priv);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to allocate attributes: %d\n", err);
> +		return err;
> +	}
> +
> +	priv->hwmon = devm_hwmon_device_register_with_groups(&pdev->dev,
> +					"mlxcpld_hotplug", priv, priv->groups);
> +	if (IS_ERR(priv->hwmon)) {
> +		dev_err(&pdev->dev, "Failed to register hwmon device %ld\n",
> +			PTR_ERR(priv->hwmon));
> +		return PTR_ERR(priv->hwmon);
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Perform initial interrupts setup. */
> +	mlxcpld_hotplug_set_irq(priv);
> +
> +	return 0;
> +}
> +
> +static int mlxcpld_hotplug_remove(struct platform_device *pdev)
> +{
> +	struct mlxcpld_hotplug_priv_data *priv = platform_get_drvdata(pdev);
> +
> +	/* Clean interrupts setup. */
> +	mlxcpld_hotplug_unset_irq(priv);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mlxcpld_hotplug_driver = {
> +	.driver = {
> +		.name = "mlxcpld-hotplug",
> +	},
> +	.probe = mlxcpld_hotplug_probe,
> +	.remove = mlxcpld_hotplug_remove,
> +};
> +
> +module_platform_driver(mlxcpld_hotplug_driver);
> +
> +MODULE_AUTHOR("Vadim Pasternak <vadimp@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Mellanox CPLD hotplug platform driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_ALIAS("platform:mlxcpld-hotplug");
> diff --git a/include/linux/platform_data/mlxcpld-hotplug.h b/include/linux/platform_data/mlxcpld-hotplug.h
> new file mode 100644
> index 0000000..2f1554e
> --- /dev/null
> +++ b/include/linux/platform_data/mlxcpld-hotplug.h
> @@ -0,0 +1,92 @@
> +/*
> + * include/linux/platform_data/mlxcpld-hotplug.h
> + * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
> + * Copyright (c) 2016 Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_MLXCPLD_HOTPLUG_H
> +#define __LINUX_PLATFORM_DATA_MLXCPLD_HOTPLUG_H
> +
> +/*
> + * mlxcpld_hotplug_device - I2C device data:
> + * @adapter - I2C device adapter;
> + * @client - I2C device client;
> + * @brdinfo - device board information;
> + * @bus - I2C bus, where device is attached;
> + */

Especially for header files, please use kernel-doc format.

> +struct mlxcpld_hotplug_device {
> +	struct i2c_adapter *adapter;
> +	struct i2c_client *client;
> +	struct i2c_board_info brdinfo;
> +	u16 bus;
> +};
> +
> +/*
> + * mlxcpld_hotplug_platform_data - device platform data:
> + * @top_aggr_offset - offset of top aggregation interrupt register;
> + * @top_aggr_mask - top aggregation interrupt common mask;
> + * @top_aggr_psu_mask - top aggregation interrupt PSU mask;
> + * @psu_reg_offset - offset of PSU interrupt register;
> + * @psu_mask - PSU interrupt mask;
> + * @psu_count - number of equipped replaceable PSUs;
> + * @psu - pointer to PSU devices data array;
> + * @top_aggr_pwr_mask - top aggregation interrupt power mask;
> + * @pwr_reg_offset - offset of power interrupt register
> + * @pwr_mask - power interrupt mask;
> + * @pwr_count - number of power sources;
> + * @pwr - pointer to power devices data array;
> + * @top_aggr_fan_mask - top aggregation interrupt FAN mask;
> + * @fan_reg_offset - offset of FAN interrupt register;
> + * @fan_mask - FAN interrupt mask;
> + * @fan_count - number of equipped replaceable FANs;
> + * @fan - pointer to FAN devices data array;
> + */

Same here please.

> +struct mlxcpld_hotplug_platform_data {
> +	u16 top_aggr_offset;
> +	u8 top_aggr_mask;
> +	u8 top_aggr_psu_mask;
> +	u16 psu_reg_offset;
> +	u8 psu_mask;
> +	u8 psu_count;
> +	struct mlxcpld_hotplug_device *psu;
> +	u8 top_aggr_pwr_mask;
> +	u16 pwr_reg_offset;
> +	u8 pwr_mask;
> +	u8 pwr_count;
> +	struct mlxcpld_hotplug_device *pwr;
> +	u8 top_aggr_fan_mask;
> +	u16 fan_reg_offset;
> +	u8 fan_mask;
> +	u8 fan_count;
> +	struct mlxcpld_hotplug_device *fan;
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_MLXCPLD_HOTPLUG_H */
> -- 
> 2.1.4
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center