All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Add support for Congatec CGEB BIOS interface
@ 2015-06-11 20:48 Christian Gmeiner
  2015-06-11 20:48 ` [PATCH 1/4] x86: Add basic support for the " Christian Gmeiner
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-11 20:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: sameo, lee.jones, wsa, jdelvare, linux-i2c, wim, linux-watchdog,
	jingoohan1

The following series adds support for the Congatec CGEB interface
found on some Congatec x86 boards. The CGEB interface is a BIOS
interface which provides access to onboard peripherals like I2C
busses and watchdogs. It works by mapping BIOS code and searching
for magic values which specify the entry points to the CGEB call.
The CGEB call is an API provided by the BIOS which provides access
to the functions in an ioctl like fashion.

This series is based on patches from Sascha Hauer, which can be found
here:

http://patchwork.ozlabs.org/patch/219756/
http://patchwork.ozlabs.org/patch/219755/
http://patchwork.ozlabs.org/patch/219757/

I really want to get this patches into mainline, as I am carry them
for too long.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/4] x86: Add basic support for the Congatec CGEB BIOS interface
  2015-06-11 20:48 [PATCH 0/4] Add support for Congatec CGEB BIOS interface Christian Gmeiner
@ 2015-06-11 20:48 ` Christian Gmeiner
  2015-06-12  9:35     ` Paul Bolle
  2015-06-11 20:48 ` [PATCH 2/4] i2c: Add Congatec CGEB I2C driver Christian Gmeiner
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-11 20:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: sameo, lee.jones, wsa, jdelvare, linux-i2c, wim, linux-watchdog,
	jingoohan1, Sascha Hauer, Christian Gmeiner

From: Sascha Hauer <s.hauer@pengutronix.de>

The Congatec CGEB is a BIOS interface found on some Congatec x86
modules. It provides access to on board peripherals like I2C busses
and watchdogs. This driver contains the basic support for accessing
the CGEB interface and registers the child devices.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/mfd/Kconfig               |  10 +
 drivers/mfd/Makefile              |   1 +
 drivers/mfd/congatec-cgeb.c       | 591 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/congatec-cgeb.h | 105 +++++++
 4 files changed, 707 insertions(+)
 create mode 100644 drivers/mfd/congatec-cgeb.c
 create mode 100644 include/linux/mfd/congatec-cgeb.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index d5ad04d..619fb3f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -710,6 +710,16 @@ config MFD_RTSX_USB
 	  Realtek card reader supports access to many types of memory cards,
 	  such as Memory Stick Pro, Secure Digital and MultiMediaCard.
 
+config MFD_CONGATEC_CGEB
+	tristate "Support for the Congatec CGEB BIOS interface"
+	depends on X86_32
+	help
+	  The Congatec CGEB BIOS interface provides access to onboard
+	  peripherals like I2C busses and watchdogs. additional drivers must be
+	  enabled in order to use the functionality of the device.
+	  Say y or m here if you are using a congatec module with CGEB interface,
+	  otherwise say n.
+
 config MFD_RC5T583
 	bool "Ricoh RC5T583 Power Management system device"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 0e5cfeb..569370c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -162,6 +162,7 @@ obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
 obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
+obj-$(CONFIG_MFD_CONGATEC_CGEB)	+= congatec-cgeb.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
diff --git a/drivers/mfd/congatec-cgeb.c b/drivers/mfd/congatec-cgeb.c
new file mode 100644
index 0000000..1d790f9
--- /dev/null
+++ b/drivers/mfd/congatec-cgeb.c
@@ -0,0 +1,591 @@
+/*
+ * CGEB driver
+ *
+ * (c) 2011 Sascha Hauer, Pengutronix
+ *
+ * Based on code from Congatec AG.
+ *
+ * CGEB is a BIOS interface found on congatech modules. It consists of
+ * code found in the BIOS memory map which is called in a ioctl like
+ * fashion. This file contains the basic driver for this interface
+ * which provides access to the GCEB interface and registers the child
+ * devices like I2C busses and watchdogs.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/io.h>
+#include <linux/string.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/congatec-cgeb.h>
+
+#include <generated/autoconf.h>
+#include <stddef.h>
+
+#define CGOS_BOARD_MAX_SIZE_ID_STRING 16
+
+#define CGEB_VERSION_MAJOR 1
+
+#define CGEB_GET_VERSION_MAJOR(v) (((unsigned long)(v)) >> 24)
+
+/* CGEB Low Descriptor located in 0xc0000-0xfffff */
+#define CGEB_LD_MAGIC "$CGEBLD$"
+
+struct cgeb_low_desc {
+	char magic[8];          /* descriptor magic string */
+	u16 size;               /* size of this descriptor */
+	u16 reserved;
+	char bios_name[8];      /* BIOS name and revision "ppppRvvv" */
+	u32 hi_desc_phys_addr;  /* phys addr of the high descriptor, can be 0 */
+};
+
+/* CGEB High Descriptor located in 0xfff00000-0xffffffff */
+#define CGEB_HD_MAGIC "$CGEBHD$"
+
+struct cgeb_high_desc {
+	char magic[8];          /* descriptor magic string */
+	u16 size;               /* size of this descriptor */
+	u16 reserved;
+	u32 data_size;          /* CGEB data area size */
+	u32 code_size;          /* CGEB code area size */
+	u32 entry_rel;          /* CGEB entry point relative to start */
+};
+
+struct cgeb_far_ptr {
+	u32 off;
+	u16 seg;
+	u16 pad;
+};
+
+struct cgeb_fps {
+	u32 size;               /* size of the parameter structure */
+	u32 fct;                /* function number */
+	struct cgeb_far_ptr data;       /* CGEB data area */
+	u32 cont;               /* private continuation pointer */
+	u32 subfps;             /* private sub function parameter
+                                * structure pointer
+                                */
+	u32 subfct;             /* sub function pointer */
+	u32 status;             /* result codes of the function */
+	u32 unit;               /* unit number or type */
+	u32 pars[4];            /* input parameters */
+	u32 rets[2];            /* return parameters */
+	void *iptr;             /* input pointer */
+	void *optr;             /* output pointer */
+};
+
+/* continuation status codes */
+#define CGEB_SUCCESS            0
+#define CGEB_NEXT               1
+#define CGEB_DELAY              2
+#define CGEB_NOIRQS             3
+
+#define CGEB_DBG_STR        0x100
+#define CGEB_DBG_HEX        0x101
+#define CGEB_DBG_DEC        0x102
+
+struct cgeb_map_mem {
+	unsigned long phys;     /* physical address */
+	unsigned long size;     /* size in bytes */
+	struct cgeb_far_ptr virt;
+};
+
+struct cgeb_map_mem_list {
+	unsigned long count;    /* number of memory map entries */
+	struct cgeb_map_mem entries[];
+};
+
+struct cgeb_boardinfo {
+	unsigned long size;
+	unsigned long flags;
+	unsigned long classes;
+	unsigned long primary_class;
+	char board[CGOS_BOARD_MAX_SIZE_ID_STRING];
+	/* optional */
+	char vendor[CGOS_BOARD_MAX_SIZE_ID_STRING];
+};
+
+struct cgeb_i2c_info {
+	unsigned long size;
+	unsigned long type;
+	unsigned long frequency;
+	unsigned long maxFrequency;
+};
+
+/* I2C Types */
+#define CGEB_I2C_TYPE_UNKNOWN 0
+#define CGEB_I2C_TYPE_PRIMARY 1
+#define CGEB_I2C_TYPE_SMB     2
+#define CGEB_I2C_TYPE_DDC     3
+#define CGEB_I2C_TYPE_BC      4
+
+struct cgeb_board_data {
+	void *code;
+	void *data;
+	unsigned short ds;
+	struct cgeb_map_mem_list *map_mem;
+	struct platform_device **devices;
+	int num_devices;
+
+	/*
+	 * entry points to a bimodal C style function that expects a far pointer
+	 * to a fps. If cs is 0 then it does a near return, otherwise a far
+	 * return. If we ever need a far return then we must not pass cs at all.
+	 * parameters are removed by the caller.
+	 */
+	void __attribute__((regparm(0)))(*entry)(unsigned short,
+			  struct cgeb_fps *, unsigned short);
+};
+
+static unsigned short get_data_segment(void)
+{
+	unsigned short ret;
+
+	asm volatile("mov %%ds, %0\n"
+			  : "=r"(ret)
+			  :
+			  : "memory"
+	);
+
+	return ret;
+}
+
+/*
+ * cgeb_invoke - invoke CGEB BIOS call.
+ *
+ * @board:     board context data
+ * @p:         CGEB parameters for this call
+ * @fct:       CGEB function code
+ * @return:    0 on success or negative error code on failure.
+ *
+ * Call the CGEB BIOS code with the given parameters.
+ */
+unsigned int cgeb_call(struct cgeb_board_data *board,
+		 struct cgeb_function_parameters *p, cgeb_function_t fct)
+{
+	struct cgeb_fps fps;
+	int i;
+
+	memset(&fps, 0, sizeof(fps));
+
+	fps.size = sizeof(fps);
+	fps.fct = fct;
+	fps.data.off = (unsigned long)board->data;
+	fps.data.seg = board->ds;
+	fps.data.pad = 0;
+	fps.status = 0;
+	fps.cont = fps.subfct = fps.subfps = 0;
+	fps.unit = p->unit;
+	for (i = 0; i < 4; i++)
+		fps.pars[i] = p->pars[i];
+	fps.iptr = p->iptr;
+	fps.optr = p->optr;
+
+	while (1) {
+		pr_debug("CGEB: CGEB: ->  size %02x, fct %02x, data %04x:%08x, status %08x\n",
+				fps.size, fps.fct, fps.data.seg, fps.data.off,
+				fps.status);
+
+		board->entry(0, &fps, fps.data.seg);
+
+		switch (fps.status) {
+		case CGEB_SUCCESS:
+			goto out;
+		case CGEB_NEXT:
+			break;  /* simply call again */
+		case CGEB_NOIRQS:
+			current->state = TASK_INTERRUPTIBLE;
+			schedule_timeout(0);
+			break;
+		case CGEB_DELAY:
+			usleep_range(fps.rets[0], fps.rets[0] + 1000);
+			break;
+		case CGEB_DBG_STR:
+			if (fps.optr)
+				pr_debug("CGEB: %s\n", (char *)fps.optr);
+			break;
+		case CGEB_DBG_HEX:
+			pr_debug("CGEB: 0x%08x\n", fps.rets[0]);
+			break;
+		case CGEB_DBG_DEC:
+			pr_debug("CGEB: %d\n", fps.rets[0]);
+			break;
+
+		default:
+			/* unknown continuation code */
+			return -EINVAL;
+		}
+	}
+out:
+	for (i = 0; i < 2; i++)
+		p->rets[i] = fps.rets[i];
+	p->optr = fps.optr;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cgeb_call);
+
+/*
+ * cgeb_call_simple - convenience wrapper for cgeb_call
+ *
+ * @board:     board context data
+ * @p:         CGEB parameters for this call
+ * @fct:       CGEB function code
+ * @return:    0 on success or negative error code on failure.
+ *
+ * Call the CGEB BIOS code with the given parameters.
+ */
+int cgeb_call_simple(struct cgeb_board_data *board,
+		 cgeb_function_t fct, unsigned long unit,
+		 unsigned long *optr, unsigned long *result)
+{
+	struct cgeb_function_parameters p;
+	unsigned int ret;
+
+	memset(&p, 0, sizeof(p));
+	p.unit = unit;
+	p.optr = optr;
+
+	ret = cgeb_call(board, &p, fct);
+	if (optr)
+		*optr = (unsigned long)p.optr;
+	if (result)
+		*result = p.rets[0];
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cgeb_call_simple);
+
+static void *cgeb_find_magic(void *_mem, size_t len, char *magic)
+{
+	unsigned long magic0 = ((unsigned long *)magic)[0];
+	unsigned long magic1 = ((unsigned long *)magic)[1];
+	int i = 0;
+
+	while (i < len) {
+		u32 *mem = _mem + i;
+
+		if (mem[0] == magic0 && mem[1] == magic1)
+			return mem;
+		i += 16;
+	}
+
+	return NULL;
+}
+
+static void cgeb_unmap_memory(struct cgeb_board_data *board)
+{
+	struct cgeb_map_mem_list *pmm;
+	struct cgeb_map_mem *pmme;
+	unsigned long i;
+
+	if (!board->map_mem)
+		return;
+
+	pmm = board->map_mem;
+	pmme = pmm->entries;
+	for (i = 0; i < pmm->count; i++, pmme++) {
+		if (pmme->virt.off)
+			iounmap((void *)pmme->virt.off);
+		pmme->virt.off = 0;
+		pmme->virt.seg = 0;
+	}
+}
+
+static int cgeb_map_memory(struct cgeb_board_data *board)
+{
+	struct cgeb_map_mem_list *pmm;
+	struct cgeb_map_mem *pmme;
+	int i;
+	int ret;
+
+	ret = cgeb_call_simple(board, CgebMapGetMem, 0, (void *)&board->map_mem,
+			NULL);
+	if (ret)
+		return ret;
+	if (!board->map_mem)
+		return 0;
+
+	pmm = board->map_mem;
+	pmme = pmm->entries;
+
+	pr_debug("CGEB: Memory Map with %lu entries\n", pmm->count);
+
+	for (i = 0; i < pmm->count; i++, pmme++) {
+		if (pmme->phys && pmme->size) {
+			pmme->virt.off =
+				(unsigned long)ioremap_cache(pmme->phys,
+							pmme->size);
+			if (!pmme->virt.off)
+				return -ENOMEM;
+		 } else {
+			pmme->virt.off = 0;
+		}
+
+		pmme->virt.seg = (pmme->virt.off) ? board->ds : 0;
+
+		pr_debug("CGEB:   Map phys %08lx, size %08lx, virt %04x:%08x\n",
+			pmme->phys, pmme->size, pmme->virt.seg,
+			pmme->virt.off);
+	}
+
+	return cgeb_call_simple(board, CgebMapChanged, 0, NULL, NULL);
+}
+
+static struct cgeb_board_data *cgeb_open(unsigned long base, unsigned long len)
+{
+	unsigned long dw;
+	struct cgeb_boardinfo *pbi;
+	struct cgeb_low_desc *low_desc;
+	struct cgeb_high_desc *high_desc = NULL;
+	unsigned long high_desc_phys;
+	unsigned long high_desc_len;
+	void __iomem *pcur, *high_desc_virt;
+	int ret;
+
+	struct cgeb_board_data *board;
+
+	board = kzalloc(sizeof(*board), GFP_KERNEL);
+	if (!board)
+		return NULL;
+
+	pcur = ioremap_cache(base, len);
+	if (!pcur)
+		goto err_kfree;
+
+	/* look for the CGEB descriptor */
+	low_desc = cgeb_find_magic(pcur, len, CGEB_LD_MAGIC);
+	if (!low_desc)
+		goto err_kfree;
+
+	pr_debug("CGEB: Found CGEB_LD_MAGIC\n");
+
+	if (low_desc->size < sizeof(struct cgeb_low_desc) - sizeof(long))
+		goto err_kfree;
+
+	if (low_desc->size >= sizeof(struct cgeb_low_desc)
+			&& low_desc->hi_desc_phys_addr)
+		high_desc_phys = low_desc->hi_desc_phys_addr;
+	else
+		high_desc_phys = 0xfff00000;
+
+	high_desc_len = (unsigned long) -(long)high_desc_phys;
+
+	pr_debug("CGEB: Looking for CGEB hi desc between phys 0x%lx and 0x%x\n",
+		high_desc_phys, -1);
+
+	high_desc_virt = ioremap_cache(high_desc_phys, high_desc_len);
+	if (!high_desc_virt)
+		goto err_kfree;
+
+	pr_debug("CGEB: Looking for CGEB hi desc between virt 0x%p and 0x%p\n",
+		high_desc_virt, high_desc_virt + high_desc_len - 1);
+
+	high_desc = cgeb_find_magic(high_desc_virt, high_desc_len,
+					CGEB_HD_MAGIC);
+	if (!high_desc)
+		goto err_iounmap;
+
+	pr_debug("CGEB: Found CGEB_HD_MAGIC\n");
+
+	if (high_desc->size < sizeof(struct cgeb_high_desc))
+		goto err_iounmap;
+
+	pr_debug("CGEB: data_size %u, code_size %u, entry_rel %u\n",
+		high_desc->data_size, high_desc->code_size, high_desc->entry_rel);
+
+	board->code = __vmalloc(high_desc->code_size, GFP_KERNEL,
+			PAGE_KERNEL_EXEC);
+	if (!board->code)
+		goto err_iounmap;
+
+	memcpy(board->code, high_desc, high_desc->code_size);
+
+	high_desc = board->code;
+
+	board->entry = board->code + high_desc->entry_rel;
+
+	board->ds = get_data_segment();
+
+	ret = cgeb_call_simple(board, CgebGetCgebVersion, 0, NULL, &dw);
+	if (ret)
+		goto err_vfree;
+
+	if (CGEB_GET_VERSION_MAJOR(dw) != CGEB_VERSION_MAJOR)
+		goto err_vfree;
+
+	pr_debug("CGEB: BIOS interface revision: %ld.%ld\n",
+			dw >> 16, dw & 0xffff);
+
+	if (high_desc->data_size) {
+		board->data = vmalloc(high_desc->data_size);
+		if (!board->data)
+			goto err_vfree;
+	} else {
+		 ret = cgeb_call_simple(board, CgebGetDataSize, 0, NULL, &dw);
+		 if (!ret && dw) {
+			board->data = vmalloc(dw);
+			if (!board->data)
+				goto err_vfree;
+		}
+	}
+
+	ret = cgeb_call_simple(board, CgebOpen, 0, NULL, NULL);
+	if (ret)
+		goto err_vfree_data;
+
+	ret = cgeb_map_memory(board);
+	if (ret)
+		goto err_free_map;
+
+	ret = cgeb_call_simple(board, CgebBoardGetInfo, 0, &dw, NULL);
+	if (ret)
+		goto err_free_map;
+
+	pbi = (struct cgeb_boardinfo *)dw;
+
+	pr_info("CGEB: Board name: %c%c%c%c\n",
+			pbi->board[0], pbi->board[1],
+			pbi->board[2], pbi->board[3]);
+
+	iounmap(high_desc_virt);
+
+	return board;
+
+err_free_map:
+	cgeb_unmap_memory(board);
+err_vfree_data:
+	vfree(board->data);
+err_vfree:
+	vfree(board->code);
+err_iounmap:
+	iounmap(high_desc_virt);
+err_kfree:
+	kfree(board);
+	return NULL;
+}
+
+static void cgeb_close(struct cgeb_board_data *board)
+{
+	cgeb_call_simple(board, CgebClose, 0, NULL, NULL);
+
+	cgeb_unmap_memory(board);
+
+	vfree(board->data);
+	vfree(board->code);
+}
+
+static unsigned long cgeb_i2c_get_type(struct cgeb_board_data *brd, int unit)
+{
+	struct cgeb_i2c_info *info;
+	int ret;
+
+	ret = cgeb_call_simple(brd, CgebI2CGetInfo, unit, (void *) &info, NULL);
+	if (ret)
+		return ret;
+	if (!info)
+		return -EINVAL;
+	return info->type;
+}
+
+static struct cgeb_board_data *cgeb_board;
+
+static int __init cgeb_init(void)
+{
+	struct cgeb_board_data *board;
+	unsigned long base;
+	int i, ret;
+	struct cgeb_pdata pdata;
+	unsigned long i2c_count, watchdog_count;
+	int num_devices = 0;
+
+	for (base = 0xf0000; base >= 0xc0000; base -= 0x10000) {
+		board = cgeb_open(base, 0x10000);
+		if (board)
+			break;
+	}
+
+	if (!board)
+		return -ENODEV;
+
+	cgeb_board = board;
+
+	pdata.board = board;
+
+	cgeb_call_simple(board, CgebI2CCount, 0, NULL, &i2c_count);
+	cgeb_call_simple(board, CgebWDogCount, 0, NULL, &watchdog_count);
+
+	board->num_devices = i2c_count + watchdog_count;
+	board->devices = kzalloc(sizeof(void *) * (board->num_devices),
+			 GFP_KERNEL);
+	if (!board->devices) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	for (i = 0; i < i2c_count; i++) {
+		ret = cgeb_i2c_get_type(board, i);
+		if (ret != CGEB_I2C_TYPE_PRIMARY)
+			continue;
+
+		 pdata.unit = i;
+
+		 board->devices[num_devices] =
+			platform_device_register_data(NULL, "cgeb-i2c", i,
+					&pdata, sizeof(pdata));
+		 num_devices++;
+	}
+
+	for (i = 0; i < watchdog_count; i++) {
+		board->devices[num_devices] =
+			platform_device_register_data(NULL, "cgeb-watchdog", i,
+					&pdata, sizeof(pdata));
+		pdata.unit = i;
+
+		num_devices++;
+	}
+
+	return 0;
+err_out:
+	cgeb_close(board);
+	kfree(board);
+
+	return ret;
+}
+
+static void cgeb_exit(void)
+{
+	struct cgeb_board_data *board = cgeb_board;
+	int i;
+
+	for (i = 0; i < board->num_devices; i++)
+		if (board->devices[i])
+			platform_device_unregister(board->devices[i]);
+
+	cgeb_close(board);
+
+	kfree(board->devices);
+	kfree(board);
+}
+
+module_init(cgeb_init);
+module_exit(cgeb_exit);
+
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_DESCRIPTION("CGEB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/congatec-cgeb.h b/include/linux/mfd/congatec-cgeb.h
new file mode 100644
index 0000000..cc18dc7
--- /dev/null
+++ b/include/linux/mfd/congatec-cgeb.h
@@ -0,0 +1,105 @@
+#ifndef __CONGATEC_CGEB_H
+#define __CONGATEC_CGEB_H
+
+/* CGEB interface functions */
+typedef enum {
+	CgebGetCgebVersion =            0,
+	CgebGetSysBiosVersion =         1,
+	CgebGetVgaBiosVersion =         2,
+	CgebGetDataSize =               3,
+	CgebOpen =                      4,
+	CgebClose =                     5,
+	CgebMapGetMem =                 6,
+	CgebMapChanged =                7,
+	CgebMapGetPorts =               8,
+	CgebDelayUs =                   9,
+	CgebCgbcReadWrite =             10,
+	CgebCgbcSetControl =            11,
+	CgebCgbcGetInfo =               12,
+	CgebCgbcHandleCommand =         13,
+	CgebBoardGetInfo =              14,
+	CgebBoardGetBootCounter =       15,
+	CgebBoardGetRunningTimeMeter =  16,
+	CgebBoardGetBootErrorLog =      17,
+	CgebVgaCount =                  18,
+	CgebVgaGetInfo =                19,
+	CgebVgaGetContrast =            20,
+	CgebVgaSetContrast =            21,
+	CgebVgaGetContrastEnable =      22,
+	CgebVgaSetContrastEnable =      23,
+	CgebVgaGetBacklight =           24,
+	CgebVgaSetBacklight =           25,
+	CgebVgaGetBacklightEnable =     26,
+	CgebVgaSetBacklightEnable =     27,
+	CgebVgaEndDarkBoot =            28,
+	CgebStorageAreaCount =          29,
+	CgebStorageAreaGetInfo =        30,
+	CgebStorageAreaRead =           31,
+	CgebStorageAreaWrite =          32,
+	CgebStorageAreaErase =          33,
+	CgebStorageAreaEraseStatus =    34,
+	CgebI2CCount =                  35,
+	CgebI2CGetInfo =                36,
+	CgebI2CGetAddrList =            37,
+	CgebI2CTransfer =               38,
+	CgebI2CGetFrequency =           39,
+	CgebI2CSetFrequency =           40,
+	CgebIOCount =                   41,
+	CgebIOGetInfo =                 42,
+	CgebIORead =                    43,
+	CgebIOWrite =                   44,
+	CgebIOGetDirection =            45,
+	CgebIOSetDirection =            46,
+	CgebWDogCount =                 47,
+	CgebWDogGetInfo =               48,
+	CgebWDogTrigger =               49,
+	CgebWDogGetConfig =             50,
+	CgebWDogSetConfig =             51,
+	CgebPerformanceGetCurrent =     52,
+	CgebPerformanceSetCurrent =     53,
+	CgebPerformanceGetPolicyCaps =  54,
+	CgebPerformanceGetPolicy =      55,
+	CgebPerformanceSetPolicy =      56,
+	CgebTemperatureCount =          57,
+	CgebTemperatureGetInfo =        58,
+	CgebTemperatureGetCurrent =     59,
+	CgebTemperatureSetLimits =      60,
+	CgebFanCount =                  61,
+	CgebFanGetInfo =                62,
+	CgebFanGetCurrent =             63,
+	CgebFanSetLimits =              64,
+	CgebVoltageCount =              65,
+	CgebVoltageGetInfo =            66,
+	CgebVoltageGetCurrent =         67,
+	CgebVoltageSetLimits =          68,
+	CgebStorageAreaLock =           69,
+	CgebStorageAreaUnlock =         70,
+	CgebStorageAreaIsLocked =       71,
+} cgeb_function_t;
+
+struct cgeb_function_parameters {
+	u32 unit;               /* unit number or type */
+	u32 pars[4];            /* input parameters */
+	u32 rets[2];            /* return parameters */
+	void *iptr;             /* input pointer */
+	void *optr;             /* output pointer */
+};
+
+struct cgeb_board_data;
+
+unsigned int cgeb_call(struct cgeb_board_data *,
+		struct cgeb_function_parameters *, cgeb_function_t);
+
+int cgeb_call_simple(struct cgeb_board_data *,
+		cgeb_function_t, unsigned long,
+		unsigned long *, unsigned long *);
+
+/*
+ * Platform data for child devices
+ */
+struct cgeb_pdata {
+	struct cgeb_board_data *board;
+	int unit;
+};
+
+#endif /* __CONGATEC_CGEB_H */
-- 
2.4.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] i2c: Add Congatec CGEB I2C driver
  2015-06-11 20:48 [PATCH 0/4] Add support for Congatec CGEB BIOS interface Christian Gmeiner
  2015-06-11 20:48 ` [PATCH 1/4] x86: Add basic support for the " Christian Gmeiner
@ 2015-06-11 20:48 ` Christian Gmeiner
  2015-06-12 11:05     ` Wolfram Sang
  2015-06-11 20:48 ` [PATCH 3/4] watchdog: Add Congatec CGEB watchdog driver Christian Gmeiner
  2015-06-11 20:48   ` Christian Gmeiner
  3 siblings, 1 reply; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-11 20:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: sameo, lee.jones, wsa, jdelvare, linux-i2c, wim, linux-watchdog,
	jingoohan1, Sascha Hauer, Christian Gmeiner

From: Sascha Hauer <s.hauer@pengutronix.de>

This driver provides a I2C bus driver for the CGEB interface
found on some Congatec x86 modules. No devices are registered
on the bus, the user has to do this via the i2c device /sys
interface.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/i2c/busses/Kconfig             |   7 ++
 drivers/i2c/busses/Makefile            |   1 +
 drivers/i2c/busses/i2c-congatec-cgeb.c | 205 +++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-congatec-cgeb.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2255af2..9c49d37 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -937,6 +937,13 @@ config I2C_RCAR
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-rcar.
 
+config I2C_CONGATEC_CGEB
+	tristate "Congatec CGEB I2C driver"
+	depends on MFD_CONGATEC_CGEB
+	help
+	  This driver provides support for the I2C busses accssable via
+	  the Congatec CGEB interface found on Congatec boards.
+
 comment "External I2C/SMBus adapter drivers"
 
 config I2C_DIOLAN_U2C
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index cdf941d..29e7ea7 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
 obj-$(CONFIG_I2C_XLR)		+= i2c-xlr.o
 obj-$(CONFIG_I2C_XLP9XX)	+= i2c-xlp9xx.o
 obj-$(CONFIG_I2C_RCAR)		+= i2c-rcar.o
+obj-$(CONFIG_I2C_CONGATEC_CGEB)	+= i2c-congatec-cgeb.o
 
 # External I2C/SMBus adapter drivers
 obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
diff --git a/drivers/i2c/busses/i2c-congatec-cgeb.c b/drivers/i2c/busses/i2c-congatec-cgeb.c
new file mode 100644
index 0000000..7802790
--- /dev/null
+++ b/drivers/i2c/busses/i2c-congatec-cgeb.c
@@ -0,0 +1,205 @@
+/*
+ * CGEB i2c driver
+ *
+ * (c) 2011 Sascha Hauer, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/mfd/congatec-cgeb.h>
+
+#define CG_I2C_FLAG_START   0x00080    /* send START condition */
+#define CG_I2C_FLAG_STOP    0x00040    /* send STOP condition */
+#define CG_I2C_FLAG_ALL_ACK 0x08000    /* send ACK on all read bytes */
+#define CG_I2C_FLAG_ALL_NAK 0x04000    /* send NAK on all read bytes */
+
+struct cgeb_i2c_priv {
+	struct cgeb_board_data          *board;
+	struct i2c_adapter      adapter;
+	int unit;
+};
+
+static u32 cgeb_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static int cgeb_i2c_set_speed(struct cgeb_i2c_priv *priv, int speed)
+{
+	struct cgeb_function_parameters fps;
+
+	memset(&fps, 0, sizeof(fps));
+
+	fps.unit = priv->unit;
+	fps.pars[0] = speed;
+
+	return cgeb_call(priv->board, &fps, CgebI2CSetFrequency);
+}
+
+static int cgeb_i2c_xfer(struct i2c_adapter *adapter,
+		struct i2c_msg *msgs, int num)
+{
+	struct cgeb_function_parameters fps;
+	int i, ret;
+	unsigned long flags = CG_I2C_FLAG_START;
+	struct cgeb_i2c_priv *priv = i2c_get_adapdata(adapter);
+	unsigned long rdlen, wrlen;
+	unsigned char *rdbuf, *wrbuf, *raw_wrbuf;
+	unsigned short lmax = 0;
+
+	/*
+	 * With cgeb the I2C address is part of the write data
+	 * buffer, so allocate a buffer with the length of the
+	 * longest write buffer + 1
+	 */
+	for (i = 0; i < num; i++)
+		if (!(msgs[i].flags & I2C_M_RD))
+			lmax = max(lmax, msgs[i].len);
+
+	raw_wrbuf = kmalloc(lmax + 1, GFP_KERNEL);
+	if (!raw_wrbuf)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].flags & I2C_M_RD) {
+			rdbuf = msgs[i].buf;
+			rdlen = msgs[i].len;
+			wrbuf = NULL;
+			wrlen = 0;
+		} else {
+			rdbuf = NULL;
+			rdlen = 0;
+			wrbuf = msgs[i].buf;
+			wrlen = msgs[i].len;
+		}
+
+		raw_wrbuf[0] = msgs[i].addr << 1;
+		if (wrlen)
+			memcpy(&raw_wrbuf[1], wrbuf, wrlen);
+
+		if (msgs[i].flags & I2C_M_RD)
+			raw_wrbuf[0] |= 1;
+
+		if (i == num - 1)
+			flags |= CG_I2C_FLAG_STOP;
+
+		dev_dbg(&adapter->dev,
+				"%s: rd: %p/%ld wr: %p/%ld flags: 0x%08lx %s\n",
+				__func__, rdbuf, rdlen, raw_wrbuf, wrlen + 1,
+				flags,
+				msgs[i].flags & I2C_M_RD ? "READ" : "WRITE");
+
+		memset(&fps, 0, sizeof(fps));
+
+		fps.unit = priv->unit;
+		fps.pars[0] = wrlen + 1;
+		fps.pars[1] = rdlen;
+		fps.pars[2] = flags;
+		fps.iptr = raw_wrbuf;
+		fps.optr = rdbuf;
+
+		ret = cgeb_call(priv->board, &fps, CgebI2CTransfer);
+		if (ret) {
+			ret = -EREMOTEIO;
+			goto out;
+		}
+	}
+
+	ret = num;
+
+out:
+	kfree(raw_wrbuf);
+
+	return ret;
+}
+
+static struct i2c_algorithm cgeb_i2c_algo = {
+	.master_xfer    = cgeb_i2c_xfer,
+	.functionality  = cgeb_i2c_func,
+};
+
+static int cgeb_i2c_probe(struct platform_device *pdev)
+{
+	struct cgeb_i2c_priv *priv;
+	struct cgeb_pdata *pdata = pdev->dev.platform_data;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	strcpy(priv->adapter.name, pdev->name);
+	priv->adapter.owner             = THIS_MODULE;
+	priv->adapter.algo              = &cgeb_i2c_algo;
+	priv->adapter.dev.parent        = &pdev->dev;
+	priv->unit = pdata->unit;
+	priv->board = pdata->board;
+	i2c_set_adapdata(&priv->adapter, priv);
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = cgeb_i2c_set_speed(priv, 400000);
+	if (ret)
+		/*
+		 * not a critical error, we can continue with the default speed.
+		 */
+		dev_warn(&pdev->dev, "Could not set speed to 400KHz\n");
+
+	ret = i2c_add_adapter(&priv->adapter);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "registration failed\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "registered\n");
+
+	return 0;
+};
+
+static int cgeb_i2c_remove(struct platform_device *pdev)
+{
+	struct cgeb_i2c_priv *priv = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&priv->adapter);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver cgeb_i2c_driver = {
+	.probe          = cgeb_i2c_probe,
+	.remove         = __exit_p(cgeb_i2c_remove),
+	.driver = {
+		.name   = "cgeb-i2c",
+		.owner  = THIS_MODULE,
+	},
+};
+
+static int __init cgeb_i2c_driver_init(void)
+{
+	return platform_driver_register(&cgeb_i2c_driver);
+}
+
+static void __exit cgeb_i2c_driver_exit(void)
+{
+	platform_driver_unregister(&cgeb_i2c_driver);
+}
+
+module_init(cgeb_i2c_driver_init);
+module_exit(cgeb_i2c_driver_exit);
+
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_DESCRIPTION("cgeb i2c driver");
+MODULE_LICENSE("GPL");
+
-- 
2.4.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] watchdog: Add Congatec CGEB watchdog driver
  2015-06-11 20:48 [PATCH 0/4] Add support for Congatec CGEB BIOS interface Christian Gmeiner
  2015-06-11 20:48 ` [PATCH 1/4] x86: Add basic support for the " Christian Gmeiner
  2015-06-11 20:48 ` [PATCH 2/4] i2c: Add Congatec CGEB I2C driver Christian Gmeiner
@ 2015-06-11 20:48 ` Christian Gmeiner
  2015-06-11 22:04     ` Guenter Roeck
  2015-06-11 20:48   ` Christian Gmeiner
  3 siblings, 1 reply; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-11 20:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: sameo, lee.jones, wsa, jdelvare, linux-i2c, wim, linux-watchdog,
	jingoohan1, Sascha Hauer, Christian Gmeiner

From: Sascha Hauer <s.hauer@pengutronix.de>

This driver provides support for the CGEB watchdog found on some
Congatec x86 modules.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/watchdog/Kconfig                  |  10 ++
 drivers/watchdog/Makefile                 |   3 +
 drivers/watchdog/congatec_cgeb_watchdog.c | 181 ++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/watchdog/congatec_cgeb_watchdog.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..bb93cbc 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1088,6 +1088,16 @@ config SBC_EPX_C3_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called sbc_epx_c3.
 
+config CONGATEC_CGEB_WATCHDOG
+	tristate "Congatec CGEB watchdog"
+	depends on MFD_CONGATEC_CGEB
+	---help---
+	  This driver provides support for the watchdogs found on Congatec
+	  modules with the CGEB BIOS interface.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called congatec_cgeb_wdt.
+
 # M32R Architecture
 
 # M68K Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c19294..b24fd09 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -171,6 +171,9 @@ obj-$(CONFIG_SH_WDT) += shwdt.o
 obj-$(CONFIG_WATCHDOG_RIO)		+= riowd.o
 obj-$(CONFIG_WATCHDOG_CP1XXX)		+= cpwd.o
 
+# X86 Architecture
+obj-$(CONFIG_CONGATEC_CGEB_WATCHDOG) += congatec_cgeb_watchdog.o
+
 # XTENSA Architecture
 
 # Xen
diff --git a/drivers/watchdog/congatec_cgeb_watchdog.c b/drivers/watchdog/congatec_cgeb_watchdog.c
new file mode 100644
index 0000000..e4523c4
--- /dev/null
+++ b/drivers/watchdog/congatec_cgeb_watchdog.c
@@ -0,0 +1,181 @@
+/*
+ * CGEB watchdog driver
+ *
+ * (c) 2011 Sascha Hauer, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/mfd/congatec-cgeb.h>
+
+#define CGOS_WDOG_MODE_REBOOT_PC    0
+#define CGOS_WDOG_MODE_RESTART_OS   1
+#define CGOS_WDOG_MODE_STAGED    0x80
+
+#define CGOS_WDOG_OPMODE_DISABLED      0
+#define CGOS_WDOG_OPMODE_ONETIME_TRIG  1
+#define CGOS_WDOG_OPMODE_SINGLE_EVENT  2
+#define CGOS_WDOG_OPMODE_EVENT_REPEAT  3
+
+#define CGOS_WDOG_EVENT_INT 0  /* NMI/IRQ */
+#define CGOS_WDOG_EVENT_SCI 1  /* SMI/SCI */
+#define CGOS_WDOG_EVENT_RST 2  /* system reset */
+#define CGOS_WDOG_EVENT_BTN 3  /* power button */
+
+#define CGOS_WDOG_EVENT_MAX_STAGES 3
+
+struct cgeb_watchdog_stage {
+	unsigned long timeout;
+	unsigned long event;
+};
+
+struct cgeb_watchdog_config {
+	unsigned long size;
+	unsigned long timeout; /* not used in staged mode */
+	unsigned long delay;
+	unsigned long mode;
+	/* optional parameters for staged watchdog */
+	unsigned long op_mode;
+	unsigned long stage_count;
+	struct cgeb_watchdog_stage stages[CGOS_WDOG_EVENT_MAX_STAGES];
+};
+
+struct cgeb_watchdog_priv {
+	struct cgeb_board_data  *board;
+	struct watchdog_device  wdd;
+	unsigned int            timeout_s;
+	int unit;
+};
+
+static struct watchdog_info cgeb_wdd_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.firmware_version = 0,
+	.identity = "cgeb watchdog",
+};
+
+static unsigned int watchdog_set_config(struct cgeb_watchdog_priv *priv,
+		unsigned int timeout_s)
+{
+	struct cgeb_board_data *board = priv->board;
+	struct cgeb_watchdog_config wdi;
+	struct cgeb_function_parameters fps;
+
+	memset(&wdi, 0, sizeof(wdi));
+	memset(&fps, 0, sizeof(fps));
+
+	fps.unit = priv->unit;
+	fps.iptr = &wdi;
+
+	wdi.timeout = timeout_s * 1000;
+	wdi.delay = 0;
+	wdi.size = sizeof(wdi);
+	wdi.mode = CGOS_WDOG_MODE_REBOOT_PC;
+
+	return cgeb_call(board, &fps, CgebWDogSetConfig);
+}
+
+static int cgeb_watchdog_start(struct watchdog_device *wdd)
+{
+	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
+
+	return watchdog_set_config(priv, priv->timeout_s);
+}
+
+static int cgeb_watchdog_stop(struct watchdog_device *wdd)
+{
+	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
+
+	return watchdog_set_config(priv, 0);
+}
+
+static int cgeb_watchdog_set_timeout(struct watchdog_device *wdd,
+		unsigned int timeout_s)
+{
+	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
+
+	if (!timeout_s)
+		return -EINVAL;
+
+	priv->timeout_s = timeout_s;
+
+	return 0;
+}
+
+struct watchdog_ops cgeb_watchdog_ops = {
+	.start = cgeb_watchdog_start,
+	.stop = cgeb_watchdog_stop,
+	.set_timeout = cgeb_watchdog_set_timeout,
+};
+
+static int cgeb_watchdog_probe(struct platform_device *pdev)
+{
+	struct cgeb_watchdog_priv *priv;
+	struct cgeb_pdata *pdata = pdev->dev.platform_data;
+	int ret;
+
+	dev_info(&pdev->dev, "registering\n");
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->wdd.ops = &cgeb_watchdog_ops;
+	priv->wdd.info = &cgeb_wdd_info;
+	priv->wdd.min_timeout = 1;
+	priv->wdd.max_timeout = 3600;
+	priv->board = pdata->board;
+	priv->unit = pdata->unit;
+
+	watchdog_set_drvdata(&priv->wdd, priv);
+	platform_set_drvdata(pdev, priv);
+
+	ret = watchdog_register_device(&priv->wdd);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int cgeb_watchdog_remove(struct platform_device *pdev)
+{
+	struct cgeb_watchdog_priv *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdd);
+
+	return 0;
+}
+
+static struct platform_driver cgeb_watchdog_driver = {
+	.probe          = cgeb_watchdog_probe,
+	.remove         = __exit_p(cgeb_watchdog_remove),
+	.driver = {
+		.name   = "cgeb-watchdog",
+		.owner  = THIS_MODULE,
+	},
+};
+
+static int __init cgeb_watchdog_driver_init(void)
+{
+	return platform_driver_register(&cgeb_watchdog_driver);
+}
+
+static void __exit cgeb_watchdog_driver_exit(void)
+{
+	platform_driver_unregister(&cgeb_watchdog_driver);
+}
+
+module_init(cgeb_watchdog_driver_init);
+module_exit(cgeb_watchdog_driver_exit);
+
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_DESCRIPTION("cgeb watchdog driver");
+MODULE_LICENSE("GPL");
-- 
2.4.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] backlight: Add Congatec CGEB backlight driver
@ 2015-06-11 20:48   ` Christian Gmeiner
  0 siblings, 0 replies; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-11 20:48 UTC (permalink / raw)
  To: linux-kernel
  Cc: sameo, lee.jones, wsa, jdelvare, linux-i2c, wim, linux-watchdog,
	jingoohan1, Christian Gmeiner

This driver provides support for the CGEB backlight found on some
Congatec x86 modules.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/video/backlight/Kconfig            |   7 ++
 drivers/video/backlight/Makefile           |   1 +
 drivers/video/backlight/congatec-cgeb_bl.c | 125 +++++++++++++++++++++++++++++
 3 files changed, 133 insertions(+)
 create mode 100644 drivers/video/backlight/congatec-cgeb_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 2d9923a..35e4913 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -451,6 +451,13 @@ config BACKLIGHT_BD6107
 	help
 	  If you have a Rohm BD6107 say Y to enable the backlight driver.
 
+config BACKLIGHT_CONGATEC_CGEB
+	tristate "Congatec CGEB Backlight driver"
+	depends on MFD_CONGATEC_CGEB
+	help
+	  This driver provides support for the backlight accssable via
+	  the Congatec CGEB interface found on Congatec boards.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index d67073f..5654d6a 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_BACKLIGHT_AS3711)		+= as3711_bl.o
 obj-$(CONFIG_BACKLIGHT_BD6107)		+= bd6107.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH)	+= cr_bllcd.o
 obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE)	+= backlight.o
+obj-$(CONFIG_BACKLIGHT_CONGATEC_CGEB)	+= congatec-cgeb_bl.o
 obj-$(CONFIG_BACKLIGHT_DA903X)		+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)		+= da9052_bl.o
 obj-$(CONFIG_BACKLIGHT_EP93XX)		+= ep93xx_bl.o
diff --git a/drivers/video/backlight/congatec-cgeb_bl.c b/drivers/video/backlight/congatec-cgeb_bl.c
new file mode 100644
index 0000000..4e5f97e
--- /dev/null
+++ b/drivers/video/backlight/congatec-cgeb_bl.c
@@ -0,0 +1,125 @@
+/*
+ * CGEB backlight driver
+ *
+ * (c) 2012 Christian Gmeiner <christian.gmeiner@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/backlight.h>
+#include <linux/mfd/congatec-cgeb.h>
+
+struct cgeb_backlight_data {
+	struct cgeb_board_data          *board;
+	int unit;
+};
+
+static int cgeb_backlight_update_status(struct backlight_device *bl)
+{
+	struct cgeb_backlight_data *data = bl_get_data(bl);
+	struct cgeb_function_parameters fps;
+	int brightness = bl->props.brightness;
+	int err;
+
+	memset(&fps, 0, sizeof(fps));
+	
+	fps.unit = data->unit;
+	fps.pars[0] = brightness;
+	
+	err = cgeb_call(data->board, &fps, CgebVgaSetBacklight);
+	return err;
+}
+
+static int cgeb_backlight_get_brightness(struct backlight_device *bl)
+{
+	struct cgeb_backlight_data *data = bl_get_data(bl);
+	unsigned long brightness;
+
+	cgeb_call_simple(data->board, CgebVgaGetBacklight, 0, NULL, &brightness);
+
+	return brightness;
+}
+
+static const struct backlight_ops cgeb_backlight_ops = {
+	.update_status	= cgeb_backlight_update_status,
+	.get_brightness	= cgeb_backlight_get_brightness,
+};
+
+static int cgeb_backlight_probe(struct platform_device *pdev)
+{  
+	struct backlight_device *bl;
+	struct backlight_properties props;
+	struct cgeb_backlight_data *data;
+	struct cgeb_pdata *pdata = pdev->dev.platform_data;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->unit = pdata->unit;
+	data->board = pdata->board;
+
+	props.max_brightness = 100;
+	props.brightness = 100;
+	props.type = BACKLIGHT_RAW;
+	
+	bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, data,
+					&cgeb_backlight_ops, &props);
+	if (IS_ERR(bl)) {
+		dev_err(&pdev->dev, "failed to register backlight\n");
+		ret = PTR_ERR(bl);
+		goto error_backlight_device_register;
+	}
+
+	platform_set_drvdata(pdev, bl);
+	
+	dev_info(&pdev->dev, "registered\n");
+	return 0;
+	
+error_backlight_device_register:
+	kfree(data);
+	return ret;
+};
+
+static int cgeb_backlight_remove(struct platform_device *pdev)
+{
+	struct backlight_device *bl = platform_get_drvdata(pdev);
+	struct cgeb_backlight_data *data = bl_get_data(bl);
+	struct cgeb_function_parameters fps;
+
+	backlight_device_unregister(bl);
+
+	/* on module unload set brightness to 100% */
+	memset(&fps, 0, sizeof(fps));
+	fps.unit = data->unit;
+	fps.pars[0] = 100;
+	cgeb_call(data->board, &fps, CgebVgaSetBacklight);
+
+	kfree(data);
+	return 0;
+}
+
+static struct platform_driver cgeb_backlight_driver = {
+	.probe          = cgeb_backlight_probe,
+	.remove         = __exit_p(cgeb_backlight_remove),
+	.driver = {
+	        .name   = "cgeb-backlight",
+	        .owner  = THIS_MODULE,
+	},
+};
+
+module_platform_driver(cgeb_backlight_driver);
+
+MODULE_AUTHOR("Christian Gmeiner <christian.gmeiner@gmail.com>");
+MODULE_DESCRIPTION("cgeb backlight driver");
+MODULE_LICENSE("GPL");
-- 
2.4.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] backlight: Add Congatec CGEB backlight driver
@ 2015-06-11 20:48   ` Christian Gmeiner
  0 siblings, 0 replies; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-11 20:48 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	wsa-z923LK4zBo2bacvFa/9K2g, jdelvare-l3A5Bk7waGM,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, wim-IQzOog9fTRqzQB+pC5nmwQ,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	jingoohan1-Re5JQEeQqe8AvxtiuMwx3w, Christian Gmeiner

This driver provides support for the CGEB backlight found on some
Congatec x86 modules.

Signed-off-by: Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/video/backlight/Kconfig            |   7 ++
 drivers/video/backlight/Makefile           |   1 +
 drivers/video/backlight/congatec-cgeb_bl.c | 125 +++++++++++++++++++++++++++++
 3 files changed, 133 insertions(+)
 create mode 100644 drivers/video/backlight/congatec-cgeb_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 2d9923a..35e4913 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -451,6 +451,13 @@ config BACKLIGHT_BD6107
 	help
 	  If you have a Rohm BD6107 say Y to enable the backlight driver.
 
+config BACKLIGHT_CONGATEC_CGEB
+	tristate "Congatec CGEB Backlight driver"
+	depends on MFD_CONGATEC_CGEB
+	help
+	  This driver provides support for the backlight accssable via
+	  the Congatec CGEB interface found on Congatec boards.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index d67073f..5654d6a 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_BACKLIGHT_AS3711)		+= as3711_bl.o
 obj-$(CONFIG_BACKLIGHT_BD6107)		+= bd6107.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH)	+= cr_bllcd.o
 obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE)	+= backlight.o
+obj-$(CONFIG_BACKLIGHT_CONGATEC_CGEB)	+= congatec-cgeb_bl.o
 obj-$(CONFIG_BACKLIGHT_DA903X)		+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)		+= da9052_bl.o
 obj-$(CONFIG_BACKLIGHT_EP93XX)		+= ep93xx_bl.o
diff --git a/drivers/video/backlight/congatec-cgeb_bl.c b/drivers/video/backlight/congatec-cgeb_bl.c
new file mode 100644
index 0000000..4e5f97e
--- /dev/null
+++ b/drivers/video/backlight/congatec-cgeb_bl.c
@@ -0,0 +1,125 @@
+/*
+ * CGEB backlight driver
+ *
+ * (c) 2012 Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/backlight.h>
+#include <linux/mfd/congatec-cgeb.h>
+
+struct cgeb_backlight_data {
+	struct cgeb_board_data          *board;
+	int unit;
+};
+
+static int cgeb_backlight_update_status(struct backlight_device *bl)
+{
+	struct cgeb_backlight_data *data = bl_get_data(bl);
+	struct cgeb_function_parameters fps;
+	int brightness = bl->props.brightness;
+	int err;
+
+	memset(&fps, 0, sizeof(fps));
+	
+	fps.unit = data->unit;
+	fps.pars[0] = brightness;
+	
+	err = cgeb_call(data->board, &fps, CgebVgaSetBacklight);
+	return err;
+}
+
+static int cgeb_backlight_get_brightness(struct backlight_device *bl)
+{
+	struct cgeb_backlight_data *data = bl_get_data(bl);
+	unsigned long brightness;
+
+	cgeb_call_simple(data->board, CgebVgaGetBacklight, 0, NULL, &brightness);
+
+	return brightness;
+}
+
+static const struct backlight_ops cgeb_backlight_ops = {
+	.update_status	= cgeb_backlight_update_status,
+	.get_brightness	= cgeb_backlight_get_brightness,
+};
+
+static int cgeb_backlight_probe(struct platform_device *pdev)
+{  
+	struct backlight_device *bl;
+	struct backlight_properties props;
+	struct cgeb_backlight_data *data;
+	struct cgeb_pdata *pdata = pdev->dev.platform_data;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->unit = pdata->unit;
+	data->board = pdata->board;
+
+	props.max_brightness = 100;
+	props.brightness = 100;
+	props.type = BACKLIGHT_RAW;
+	
+	bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, data,
+					&cgeb_backlight_ops, &props);
+	if (IS_ERR(bl)) {
+		dev_err(&pdev->dev, "failed to register backlight\n");
+		ret = PTR_ERR(bl);
+		goto error_backlight_device_register;
+	}
+
+	platform_set_drvdata(pdev, bl);
+	
+	dev_info(&pdev->dev, "registered\n");
+	return 0;
+	
+error_backlight_device_register:
+	kfree(data);
+	return ret;
+};
+
+static int cgeb_backlight_remove(struct platform_device *pdev)
+{
+	struct backlight_device *bl = platform_get_drvdata(pdev);
+	struct cgeb_backlight_data *data = bl_get_data(bl);
+	struct cgeb_function_parameters fps;
+
+	backlight_device_unregister(bl);
+
+	/* on module unload set brightness to 100% */
+	memset(&fps, 0, sizeof(fps));
+	fps.unit = data->unit;
+	fps.pars[0] = 100;
+	cgeb_call(data->board, &fps, CgebVgaSetBacklight);
+
+	kfree(data);
+	return 0;
+}
+
+static struct platform_driver cgeb_backlight_driver = {
+	.probe          = cgeb_backlight_probe,
+	.remove         = __exit_p(cgeb_backlight_remove),
+	.driver = {
+	        .name   = "cgeb-backlight",
+	        .owner  = THIS_MODULE,
+	},
+};
+
+module_platform_driver(cgeb_backlight_driver);
+
+MODULE_AUTHOR("Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("cgeb backlight driver");
+MODULE_LICENSE("GPL");
-- 
2.4.2

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] watchdog: Add Congatec CGEB watchdog driver
@ 2015-06-11 22:04     ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2015-06-11 22:04 UTC (permalink / raw)
  To: Christian Gmeiner, linux-kernel
  Cc: sameo, lee.jones, wsa, jdelvare, linux-i2c, wim, linux-watchdog,
	jingoohan1, Sascha Hauer

On 06/11/2015 01:48 PM, Christian Gmeiner wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
>
> This driver provides support for the CGEB watchdog found on some
> Congatec x86 modules.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>

Hi Christian,

comments inline.

Thanks,
Guenter

> ---
>   drivers/watchdog/Kconfig                  |  10 ++
>   drivers/watchdog/Makefile                 |   3 +
>   drivers/watchdog/congatec_cgeb_watchdog.c | 181 ++++++++++++++++++++++++++++++
>   3 files changed, 194 insertions(+)
>   create mode 100644 drivers/watchdog/congatec_cgeb_watchdog.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..bb93cbc 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1088,6 +1088,16 @@ config SBC_EPX_C3_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called sbc_epx_c3.
>
> +config CONGATEC_CGEB_WATCHDOG
> +	tristate "Congatec CGEB watchdog"
> +	depends on MFD_CONGATEC_CGEB
> +	---help---
> +	  This driver provides support for the watchdogs found on Congatec
> +	  modules with the CGEB BIOS interface.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called congatec_cgeb_wdt.
> +
>   # M32R Architecture
>
>   # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c19294..b24fd09 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -171,6 +171,9 @@ obj-$(CONFIG_SH_WDT) += shwdt.o
>   obj-$(CONFIG_WATCHDOG_RIO)		+= riowd.o
>   obj-$(CONFIG_WATCHDOG_CP1XXX)		+= cpwd.o
>
> +# X86 Architecture
> +obj-$(CONFIG_CONGATEC_CGEB_WATCHDOG) += congatec_cgeb_watchdog.o
> +

No need for a second set of x86 drivers. Please add to the existing set.
Also, please align with other '+='.


>   # XTENSA Architecture
>
>   # Xen
> diff --git a/drivers/watchdog/congatec_cgeb_watchdog.c b/drivers/watchdog/congatec_cgeb_watchdog.c
> new file mode 100644
> index 0000000..e4523c4
> --- /dev/null
> +++ b/drivers/watchdog/congatec_cgeb_watchdog.c
> @@ -0,0 +1,181 @@
> +/*
> + * CGEB watchdog driver
> + *
> + * (c) 2011 Sascha Hauer, Pengutronix

2011 ?

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/mfd/congatec-cgeb.h>
> +
> +#define CGOS_WDOG_MODE_REBOOT_PC    0
> +#define CGOS_WDOG_MODE_RESTART_OS   1
> +#define CGOS_WDOG_MODE_STAGED    0x80
> +
> +#define CGOS_WDOG_OPMODE_DISABLED      0
> +#define CGOS_WDOG_OPMODE_ONETIME_TRIG  1
> +#define CGOS_WDOG_OPMODE_SINGLE_EVENT  2
> +#define CGOS_WDOG_OPMODE_EVENT_REPEAT  3
> +
> +#define CGOS_WDOG_EVENT_INT 0  /* NMI/IRQ */
> +#define CGOS_WDOG_EVENT_SCI 1  /* SMI/SCI */
> +#define CGOS_WDOG_EVENT_RST 2  /* system reset */
> +#define CGOS_WDOG_EVENT_BTN 3  /* power button */
> +
> +#define CGOS_WDOG_EVENT_MAX_STAGES 3
> +
Please use tabs after the defines.

#define CGOS_WDOG_MODE_REBOOT_PC	0
#define CGOS_WDOG_MODE_RESTART_OS	1
#define CGOS_WDOG_MODE_STAGED		0x80

#define CGOS_WDOG_OPMODE_DISABLED	0
#define CGOS_WDOG_OPMODE_ONETIME_TRIG	1
#define CGOS_WDOG_OPMODE_SINGLE_EVENT	2
#define CGOS_WDOG_OPMODE_EVENT_REPEAT	3

and so on.

> +struct cgeb_watchdog_stage {
> +	unsigned long timeout;
> +	unsigned long event;
> +};
> +
> +struct cgeb_watchdog_config {
> +	unsigned long size;
> +	unsigned long timeout; /* not used in staged mode */
> +	unsigned long delay;
> +	unsigned long mode;
> +	/* optional parameters for staged watchdog */
> +	unsigned long op_mode;
> +	unsigned long stage_count;
> +	struct cgeb_watchdog_stage stages[CGOS_WDOG_EVENT_MAX_STAGES];
> +};
> +
Since this is passed on to cgeb_call, shoul it be part of an API
defined elsewhere ?

Also, I am a bit concerned about the use of 'unsigned long'.
If this is part of an ABI to the BIOS, shouldn't it be u32 ?
Sure, the driver is currently limited to X64_32, but that doesn't
mean we have to make it extra difficult to port it to X86_64
if that is ever asked for.

> +struct cgeb_watchdog_priv {
> +	struct cgeb_board_data  *board;
> +	struct watchdog_device  wdd;
> +	unsigned int            timeout_s;
> +	int unit;
> +};
> +
> +static struct watchdog_info cgeb_wdd_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.firmware_version = 0,

No need to initialize static variables with 0.

> +	.identity = "cgeb watchdog",
> +};
> +
Please move the above next to the probe function.

> +static unsigned int watchdog_set_config(struct cgeb_watchdog_priv *priv,
> +		unsigned int timeout_s)
> +{
> +	struct cgeb_board_data *board = priv->board;
> +	struct cgeb_watchdog_config wdi;
> +	struct cgeb_function_parameters fps;
> +
						= { 0 };

> +	memset(&wdi, 0, sizeof(wdi));
> +	memset(&fps, 0, sizeof(fps));

would be identical to the memset.

> +
> +	fps.unit = priv->unit;
> +	fps.iptr = &wdi;
> +
> +	wdi.timeout = timeout_s * 1000;
> +	wdi.delay = 0;

Already set to 0.

> +	wdi.size = sizeof(wdi);
> +	wdi.mode = CGOS_WDOG_MODE_REBOOT_PC;
> +
> +	return cgeb_call(board, &fps, CgebWDogSetConfig);
> +}
> +
> +static int cgeb_watchdog_start(struct watchdog_device *wdd)
> +{
> +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	return watchdog_set_config(priv, priv->timeout_s);
> +}
> +
> +static int cgeb_watchdog_stop(struct watchdog_device *wdd)
> +{
> +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	return watchdog_set_config(priv, 0);
> +}
> +
> +static int cgeb_watchdog_set_timeout(struct watchdog_device *wdd,
> +		unsigned int timeout_s)

Please align continuation lines with '('.

> +{
> +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	if (!timeout_s)
> +		return -EINVAL;

Unnecessary, checked by infrastructure.

> +
> +	priv->timeout_s = timeout_s;
> +
You also need to set wdd->timeout. It also seems to me that
priv->timeout_s is not really necessary.

> +	return 0;
> +}
> +
> +struct watchdog_ops cgeb_watchdog_ops = {
> +	.start = cgeb_watchdog_start,
> +	.stop = cgeb_watchdog_stop,
> +	.set_timeout = cgeb_watchdog_set_timeout,
> +};
> +
> +static int cgeb_watchdog_probe(struct platform_device *pdev)
> +{
> +	struct cgeb_watchdog_priv *priv;
> +	struct cgeb_pdata *pdata = pdev->dev.platform_data;
> +	int ret;
> +
> +	dev_info(&pdev->dev, "registering\n");

Unnecessary noise.

> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->wdd.ops = &cgeb_watchdog_ops;
> +	priv->wdd.info = &cgeb_wdd_info;
> +	priv->wdd.min_timeout = 1;
> +	priv->wdd.max_timeout = 3600;
> +	priv->board = pdata->board;

You should check if pdata == NULL and either use defaults
in that case or bail out.

> +	priv->unit = pdata->unit;
> +
> +	watchdog_set_drvdata(&priv->wdd, priv);
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = watchdog_register_device(&priv->wdd);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

just
	return watchdog_register_device(&priv->wdd);

> +}
> +
> +static int cgeb_watchdog_remove(struct platform_device *pdev)
> +{
> +	struct cgeb_watchdog_priv *priv = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&priv->wdd);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cgeb_watchdog_driver = {
> +	.probe          = cgeb_watchdog_probe,
> +	.remove         = __exit_p(cgeb_watchdog_remove),
> +	.driver = {
> +		.name   = "cgeb-watchdog",
> +		.owner  = THIS_MODULE,

AFAIK .owner is no longer necessary.

> +	},
> +};
> +
> +static int __init cgeb_watchdog_driver_init(void)
> +{
> +	return platform_driver_register(&cgeb_watchdog_driver);
> +}
> +
> +static void __exit cgeb_watchdog_driver_exit(void)
> +{
> +	platform_driver_unregister(&cgeb_watchdog_driver);
> +}
> +
> +module_init(cgeb_watchdog_driver_init);
> +module_exit(cgeb_watchdog_driver_exit);

module_platform_driver(cgeb_watchdog_driver);

> +
> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> +MODULE_DESCRIPTION("cgeb watchdog driver");
> +MODULE_LICENSE("GPL");
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] watchdog: Add Congatec CGEB watchdog driver
@ 2015-06-11 22:04     ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2015-06-11 22:04 UTC (permalink / raw)
  To: Christian Gmeiner, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	wsa-z923LK4zBo2bacvFa/9K2g, jdelvare-l3A5Bk7waGM,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, wim-IQzOog9fTRqzQB+pC5nmwQ,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	jingoohan1-Re5JQEeQqe8AvxtiuMwx3w, Sascha Hauer

On 06/11/2015 01:48 PM, Christian Gmeiner wrote:
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> This driver provides support for the CGEB watchdog found on some
> Congatec x86 modules.
>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Hi Christian,

comments inline.

Thanks,
Guenter

> ---
>   drivers/watchdog/Kconfig                  |  10 ++
>   drivers/watchdog/Makefile                 |   3 +
>   drivers/watchdog/congatec_cgeb_watchdog.c | 181 ++++++++++++++++++++++++++++++
>   3 files changed, 194 insertions(+)
>   create mode 100644 drivers/watchdog/congatec_cgeb_watchdog.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..bb93cbc 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1088,6 +1088,16 @@ config SBC_EPX_C3_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called sbc_epx_c3.
>
> +config CONGATEC_CGEB_WATCHDOG
> +	tristate "Congatec CGEB watchdog"
> +	depends on MFD_CONGATEC_CGEB
> +	---help---
> +	  This driver provides support for the watchdogs found on Congatec
> +	  modules with the CGEB BIOS interface.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called congatec_cgeb_wdt.
> +
>   # M32R Architecture
>
>   # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c19294..b24fd09 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -171,6 +171,9 @@ obj-$(CONFIG_SH_WDT) += shwdt.o
>   obj-$(CONFIG_WATCHDOG_RIO)		+= riowd.o
>   obj-$(CONFIG_WATCHDOG_CP1XXX)		+= cpwd.o
>
> +# X86 Architecture
> +obj-$(CONFIG_CONGATEC_CGEB_WATCHDOG) += congatec_cgeb_watchdog.o
> +

No need for a second set of x86 drivers. Please add to the existing set.
Also, please align with other '+='.


>   # XTENSA Architecture
>
>   # Xen
> diff --git a/drivers/watchdog/congatec_cgeb_watchdog.c b/drivers/watchdog/congatec_cgeb_watchdog.c
> new file mode 100644
> index 0000000..e4523c4
> --- /dev/null
> +++ b/drivers/watchdog/congatec_cgeb_watchdog.c
> @@ -0,0 +1,181 @@
> +/*
> + * CGEB watchdog driver
> + *
> + * (c) 2011 Sascha Hauer, Pengutronix

2011 ?

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/mfd/congatec-cgeb.h>
> +
> +#define CGOS_WDOG_MODE_REBOOT_PC    0
> +#define CGOS_WDOG_MODE_RESTART_OS   1
> +#define CGOS_WDOG_MODE_STAGED    0x80
> +
> +#define CGOS_WDOG_OPMODE_DISABLED      0
> +#define CGOS_WDOG_OPMODE_ONETIME_TRIG  1
> +#define CGOS_WDOG_OPMODE_SINGLE_EVENT  2
> +#define CGOS_WDOG_OPMODE_EVENT_REPEAT  3
> +
> +#define CGOS_WDOG_EVENT_INT 0  /* NMI/IRQ */
> +#define CGOS_WDOG_EVENT_SCI 1  /* SMI/SCI */
> +#define CGOS_WDOG_EVENT_RST 2  /* system reset */
> +#define CGOS_WDOG_EVENT_BTN 3  /* power button */
> +
> +#define CGOS_WDOG_EVENT_MAX_STAGES 3
> +
Please use tabs after the defines.

#define CGOS_WDOG_MODE_REBOOT_PC	0
#define CGOS_WDOG_MODE_RESTART_OS	1
#define CGOS_WDOG_MODE_STAGED		0x80

#define CGOS_WDOG_OPMODE_DISABLED	0
#define CGOS_WDOG_OPMODE_ONETIME_TRIG	1
#define CGOS_WDOG_OPMODE_SINGLE_EVENT	2
#define CGOS_WDOG_OPMODE_EVENT_REPEAT	3

and so on.

> +struct cgeb_watchdog_stage {
> +	unsigned long timeout;
> +	unsigned long event;
> +};
> +
> +struct cgeb_watchdog_config {
> +	unsigned long size;
> +	unsigned long timeout; /* not used in staged mode */
> +	unsigned long delay;
> +	unsigned long mode;
> +	/* optional parameters for staged watchdog */
> +	unsigned long op_mode;
> +	unsigned long stage_count;
> +	struct cgeb_watchdog_stage stages[CGOS_WDOG_EVENT_MAX_STAGES];
> +};
> +
Since this is passed on to cgeb_call, shoul it be part of an API
defined elsewhere ?

Also, I am a bit concerned about the use of 'unsigned long'.
If this is part of an ABI to the BIOS, shouldn't it be u32 ?
Sure, the driver is currently limited to X64_32, but that doesn't
mean we have to make it extra difficult to port it to X86_64
if that is ever asked for.

> +struct cgeb_watchdog_priv {
> +	struct cgeb_board_data  *board;
> +	struct watchdog_device  wdd;
> +	unsigned int            timeout_s;
> +	int unit;
> +};
> +
> +static struct watchdog_info cgeb_wdd_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.firmware_version = 0,

No need to initialize static variables with 0.

> +	.identity = "cgeb watchdog",
> +};
> +
Please move the above next to the probe function.

> +static unsigned int watchdog_set_config(struct cgeb_watchdog_priv *priv,
> +		unsigned int timeout_s)
> +{
> +	struct cgeb_board_data *board = priv->board;
> +	struct cgeb_watchdog_config wdi;
> +	struct cgeb_function_parameters fps;
> +
						= { 0 };

> +	memset(&wdi, 0, sizeof(wdi));
> +	memset(&fps, 0, sizeof(fps));

would be identical to the memset.

> +
> +	fps.unit = priv->unit;
> +	fps.iptr = &wdi;
> +
> +	wdi.timeout = timeout_s * 1000;
> +	wdi.delay = 0;

Already set to 0.

> +	wdi.size = sizeof(wdi);
> +	wdi.mode = CGOS_WDOG_MODE_REBOOT_PC;
> +
> +	return cgeb_call(board, &fps, CgebWDogSetConfig);
> +}
> +
> +static int cgeb_watchdog_start(struct watchdog_device *wdd)
> +{
> +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	return watchdog_set_config(priv, priv->timeout_s);
> +}
> +
> +static int cgeb_watchdog_stop(struct watchdog_device *wdd)
> +{
> +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	return watchdog_set_config(priv, 0);
> +}
> +
> +static int cgeb_watchdog_set_timeout(struct watchdog_device *wdd,
> +		unsigned int timeout_s)

Please align continuation lines with '('.

> +{
> +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	if (!timeout_s)
> +		return -EINVAL;

Unnecessary, checked by infrastructure.

> +
> +	priv->timeout_s = timeout_s;
> +
You also need to set wdd->timeout. It also seems to me that
priv->timeout_s is not really necessary.

> +	return 0;
> +}
> +
> +struct watchdog_ops cgeb_watchdog_ops = {
> +	.start = cgeb_watchdog_start,
> +	.stop = cgeb_watchdog_stop,
> +	.set_timeout = cgeb_watchdog_set_timeout,
> +};
> +
> +static int cgeb_watchdog_probe(struct platform_device *pdev)
> +{
> +	struct cgeb_watchdog_priv *priv;
> +	struct cgeb_pdata *pdata = pdev->dev.platform_data;
> +	int ret;
> +
> +	dev_info(&pdev->dev, "registering\n");

Unnecessary noise.

> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->wdd.ops = &cgeb_watchdog_ops;
> +	priv->wdd.info = &cgeb_wdd_info;
> +	priv->wdd.min_timeout = 1;
> +	priv->wdd.max_timeout = 3600;
> +	priv->board = pdata->board;

You should check if pdata == NULL and either use defaults
in that case or bail out.

> +	priv->unit = pdata->unit;
> +
> +	watchdog_set_drvdata(&priv->wdd, priv);
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = watchdog_register_device(&priv->wdd);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

just
	return watchdog_register_device(&priv->wdd);

> +}
> +
> +static int cgeb_watchdog_remove(struct platform_device *pdev)
> +{
> +	struct cgeb_watchdog_priv *priv = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&priv->wdd);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cgeb_watchdog_driver = {
> +	.probe          = cgeb_watchdog_probe,
> +	.remove         = __exit_p(cgeb_watchdog_remove),
> +	.driver = {
> +		.name   = "cgeb-watchdog",
> +		.owner  = THIS_MODULE,

AFAIK .owner is no longer necessary.

> +	},
> +};
> +
> +static int __init cgeb_watchdog_driver_init(void)
> +{
> +	return platform_driver_register(&cgeb_watchdog_driver);
> +}
> +
> +static void __exit cgeb_watchdog_driver_exit(void)
> +{
> +	platform_driver_unregister(&cgeb_watchdog_driver);
> +}
> +
> +module_init(cgeb_watchdog_driver_init);
> +module_exit(cgeb_watchdog_driver_exit);

module_platform_driver(cgeb_watchdog_driver);

> +
> +MODULE_AUTHOR("Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
> +MODULE_DESCRIPTION("cgeb watchdog driver");
> +MODULE_LICENSE("GPL");
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] x86: Add basic support for the Congatec CGEB BIOS interface
@ 2015-06-12  9:35     ` Paul Bolle
  0 siblings, 0 replies; 14+ messages in thread
From: Paul Bolle @ 2015-06-12  9:35 UTC (permalink / raw)
  To: Christian Gmeiner
  Cc: linux-kernel, sameo, lee.jones, wsa, jdelvare, linux-i2c, wim,
	linux-watchdog, jingoohan1, Sascha Hauer

A few nits.

On Thu, 2015-06-11 at 22:48 +0200, Christian Gmeiner wrote:
> --- /dev/null
> +++ b/drivers/mfd/congatec-cgeb.c

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

> +/*
> + * cgeb_invoke - invoke CGEB BIOS call.

s/cgeb_invoke/cgeb_call/

> + *
> + * @board:     board context data
> + * @p:         CGEB parameters for this call
> + * @fct:       CGEB function code
> + * @return:    0 on success or negative error code on failure.
> + *
> + * Call the CGEB BIOS code with the given parameters.
> + */
> +unsigned int cgeb_call(struct cgeb_board_data *board,
> +		 struct cgeb_function_parameters *p, cgeb_function_t fct)
> +{
> +	[...]
> +}

> +static int __init cgeb_init(void)
> +{
> +	[...]
> +}
> +
> +static void cgeb_exit(void)
> +{
> +	[...]
> +}
> +
> +module_init(cgeb_init);
> +module_exit(cgeb_exit);

cgeb_exit is only used through module_exit(). So I guess it could be
marked __exit, right?

> +MODULE_LICENSE("GPL");

The comment at the top of this file states the license is GPL v2. The
MODULE_LICENSE() macro states, according to include/linux/module.h, that
the license is GPL v2 or later. So I think one of these two needs to
change.

I spotted the same license mismatch in 2/4, 3/4 and 4/4.

Thanks,


Paul Bolle


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] x86: Add basic support for the Congatec CGEB BIOS interface
@ 2015-06-12  9:35     ` Paul Bolle
  0 siblings, 0 replies; 14+ messages in thread
From: Paul Bolle @ 2015-06-12  9:35 UTC (permalink / raw)
  To: Christian Gmeiner
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	wsa-z923LK4zBo2bacvFa/9K2g, jdelvare-l3A5Bk7waGM,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, wim-IQzOog9fTRqzQB+pC5nmwQ,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	jingoohan1-Re5JQEeQqe8AvxtiuMwx3w, Sascha Hauer

A few nits.

On Thu, 2015-06-11 at 22:48 +0200, Christian Gmeiner wrote:
> --- /dev/null
> +++ b/drivers/mfd/congatec-cgeb.c

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

> +/*
> + * cgeb_invoke - invoke CGEB BIOS call.

s/cgeb_invoke/cgeb_call/

> + *
> + * @board:     board context data
> + * @p:         CGEB parameters for this call
> + * @fct:       CGEB function code
> + * @return:    0 on success or negative error code on failure.
> + *
> + * Call the CGEB BIOS code with the given parameters.
> + */
> +unsigned int cgeb_call(struct cgeb_board_data *board,
> +		 struct cgeb_function_parameters *p, cgeb_function_t fct)
> +{
> +	[...]
> +}

> +static int __init cgeb_init(void)
> +{
> +	[...]
> +}
> +
> +static void cgeb_exit(void)
> +{
> +	[...]
> +}
> +
> +module_init(cgeb_init);
> +module_exit(cgeb_exit);

cgeb_exit is only used through module_exit(). So I guess it could be
marked __exit, right?

> +MODULE_LICENSE("GPL");

The comment at the top of this file states the license is GPL v2. The
MODULE_LICENSE() macro states, according to include/linux/module.h, that
the license is GPL v2 or later. So I think one of these two needs to
change.

I spotted the same license mismatch in 2/4, 3/4 and 4/4.

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] i2c: Add Congatec CGEB I2C driver
@ 2015-06-12 11:05     ` Wolfram Sang
  0 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2015-06-12 11:05 UTC (permalink / raw)
  To: Christian Gmeiner
  Cc: linux-kernel, sameo, lee.jones, jdelvare, linux-i2c, wim,
	linux-watchdog, jingoohan1, Sascha Hauer

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]


Oh, this one again. Like last time, close to be ready.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/congatec-cgeb.h>

Please sort the includes.

> +struct cgeb_i2c_priv {
> +	struct cgeb_board_data          *board;
> +	struct i2c_adapter      adapter;
> +	int unit;
> +};

Only a single space for indentation

> +	ret = cgeb_i2c_set_speed(priv, 400000);

Not all slaves handle 400kHz. So, this might be a bit demanding. Then
again, it is probably fix which slaves are connected and they work
currently?

> +		/*
> +		 * not a critical error, we can continue with the default speed.
> +		 */

Single line comment, even if it is a little over 80 chars

> +	platform_set_drvdata(pdev, NULL);

Not needed.

> +
> +static struct platform_driver cgeb_i2c_driver = {
> +	.probe          = cgeb_i2c_probe,
> +	.remove         = __exit_p(cgeb_i2c_remove),

Leftover annotation?

> +	.driver = {
> +		.name   = "cgeb-i2c",
> +		.owner  = THIS_MODULE,

Owner not needed

> +	},
> +};
> +
> +static int __init cgeb_i2c_driver_init(void)
> +{
> +	return platform_driver_register(&cgeb_i2c_driver);
> +}
> +
> +static void __exit cgeb_i2c_driver_exit(void)
> +{
> +	platform_driver_unregister(&cgeb_i2c_driver);
> +}
> +
> +module_init(cgeb_i2c_driver_init);
> +module_exit(cgeb_i2c_driver_exit);

module_platform_driver?

> +
> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> +MODULE_DESCRIPTION("cgeb i2c driver");
> +MODULE_LICENSE("GPL");

GPL v2


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] i2c: Add Congatec CGEB I2C driver
@ 2015-06-12 11:05     ` Wolfram Sang
  0 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2015-06-12 11:05 UTC (permalink / raw)
  To: Christian Gmeiner
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	jdelvare-l3A5Bk7waGM, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	wim-IQzOog9fTRqzQB+pC5nmwQ,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	jingoohan1-Re5JQEeQqe8AvxtiuMwx3w, Sascha Hauer

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]


Oh, this one again. Like last time, close to be ready.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/congatec-cgeb.h>

Please sort the includes.

> +struct cgeb_i2c_priv {
> +	struct cgeb_board_data          *board;
> +	struct i2c_adapter      adapter;
> +	int unit;
> +};

Only a single space for indentation

> +	ret = cgeb_i2c_set_speed(priv, 400000);

Not all slaves handle 400kHz. So, this might be a bit demanding. Then
again, it is probably fix which slaves are connected and they work
currently?

> +		/*
> +		 * not a critical error, we can continue with the default speed.
> +		 */

Single line comment, even if it is a little over 80 chars

> +	platform_set_drvdata(pdev, NULL);

Not needed.

> +
> +static struct platform_driver cgeb_i2c_driver = {
> +	.probe          = cgeb_i2c_probe,
> +	.remove         = __exit_p(cgeb_i2c_remove),

Leftover annotation?

> +	.driver = {
> +		.name   = "cgeb-i2c",
> +		.owner  = THIS_MODULE,

Owner not needed

> +	},
> +};
> +
> +static int __init cgeb_i2c_driver_init(void)
> +{
> +	return platform_driver_register(&cgeb_i2c_driver);
> +}
> +
> +static void __exit cgeb_i2c_driver_exit(void)
> +{
> +	platform_driver_unregister(&cgeb_i2c_driver);
> +}
> +
> +module_init(cgeb_i2c_driver_init);
> +module_exit(cgeb_i2c_driver_exit);

module_platform_driver?

> +
> +MODULE_AUTHOR("Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
> +MODULE_DESCRIPTION("cgeb i2c driver");
> +MODULE_LICENSE("GPL");

GPL v2


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] x86: Add basic support for the Congatec CGEB BIOS interface
  2015-06-12  9:35     ` Paul Bolle
@ 2015-06-15  7:24       ` Christian Gmeiner
  -1 siblings, 0 replies; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-15  7:24 UTC (permalink / raw)
  To: Paul Bolle
  Cc: LKML, Samuel Ortiz, Lee Jones, Wolfram Sang, jdelvare, linux-i2c,
	wim, linux-watchdog, jingoohan1, Sascha Hauer

Hi Paul

Thanks for looking over my patches!

2015-06-12 11:35 GMT+02:00 Paul Bolle <pebolle@tiscali.nl>:
> A few nits.
>
> On Thu, 2015-06-11 at 22:48 +0200, Christian Gmeiner wrote:
>> --- /dev/null
>> +++ b/drivers/mfd/congatec-cgeb.c
>
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>
>> +/*
>> + * cgeb_invoke - invoke CGEB BIOS call.
>
> s/cgeb_invoke/cgeb_call/
>

ok

>> + *
>> + * @board:     board context data
>> + * @p:         CGEB parameters for this call
>> + * @fct:       CGEB function code
>> + * @return:    0 on success or negative error code on failure.
>> + *
>> + * Call the CGEB BIOS code with the given parameters.
>> + */
>> +unsigned int cgeb_call(struct cgeb_board_data *board,
>> +              struct cgeb_function_parameters *p, cgeb_function_t fct)
>> +{
>> +     [...]
>> +}
>
>> +static int __init cgeb_init(void)
>> +{
>> +     [...]
>> +}
>> +
>> +static void cgeb_exit(void)
>> +{
>> +     [...]
>> +}
>> +
>> +module_init(cgeb_init);
>> +module_exit(cgeb_exit);
>
> cgeb_exit is only used through module_exit(). So I guess it could be
> marked __exit, right?
>

That is correct - will be fixed in v2.

>> +MODULE_LICENSE("GPL");
>
> The comment at the top of this file states the license is GPL v2. The
> MODULE_LICENSE() macro states, according to include/linux/module.h, that
> the license is GPL v2 or later. So I think one of these two needs to
> change.
>
> I spotted the same license mismatch in 2/4, 3/4 and 4/4.
>

Will be fixed in v2.

Thanks
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] x86: Add basic support for the Congatec CGEB BIOS interface
@ 2015-06-15  7:24       ` Christian Gmeiner
  0 siblings, 0 replies; 14+ messages in thread
From: Christian Gmeiner @ 2015-06-15  7:24 UTC (permalink / raw)
  To: Paul Bolle
  Cc: LKML, Samuel Ortiz, Lee Jones, Wolfram Sang,
	jdelvare-l3A5Bk7waGM, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	wim-IQzOog9fTRqzQB+pC5nmwQ,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	jingoohan1-Re5JQEeQqe8AvxtiuMwx3w, Sascha Hauer

Hi Paul

Thanks for looking over my patches!

2015-06-12 11:35 GMT+02:00 Paul Bolle <pebolle-IWqWACnzNjzz+pZb47iToQ@public.gmane.org>:
> A few nits.
>
> On Thu, 2015-06-11 at 22:48 +0200, Christian Gmeiner wrote:
>> --- /dev/null
>> +++ b/drivers/mfd/congatec-cgeb.c
>
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>
>> +/*
>> + * cgeb_invoke - invoke CGEB BIOS call.
>
> s/cgeb_invoke/cgeb_call/
>

ok

>> + *
>> + * @board:     board context data
>> + * @p:         CGEB parameters for this call
>> + * @fct:       CGEB function code
>> + * @return:    0 on success or negative error code on failure.
>> + *
>> + * Call the CGEB BIOS code with the given parameters.
>> + */
>> +unsigned int cgeb_call(struct cgeb_board_data *board,
>> +              struct cgeb_function_parameters *p, cgeb_function_t fct)
>> +{
>> +     [...]
>> +}
>
>> +static int __init cgeb_init(void)
>> +{
>> +     [...]
>> +}
>> +
>> +static void cgeb_exit(void)
>> +{
>> +     [...]
>> +}
>> +
>> +module_init(cgeb_init);
>> +module_exit(cgeb_exit);
>
> cgeb_exit is only used through module_exit(). So I guess it could be
> marked __exit, right?
>

That is correct - will be fixed in v2.

>> +MODULE_LICENSE("GPL");
>
> The comment at the top of this file states the license is GPL v2. The
> MODULE_LICENSE() macro states, according to include/linux/module.h, that
> the license is GPL v2 or later. So I think one of these two needs to
> change.
>
> I spotted the same license mismatch in 2/4, 3/4 and 4/4.
>

Will be fixed in v2.

Thanks
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-06-15  7:24 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-11 20:48 [PATCH 0/4] Add support for Congatec CGEB BIOS interface Christian Gmeiner
2015-06-11 20:48 ` [PATCH 1/4] x86: Add basic support for the " Christian Gmeiner
2015-06-12  9:35   ` Paul Bolle
2015-06-12  9:35     ` Paul Bolle
2015-06-15  7:24     ` Christian Gmeiner
2015-06-15  7:24       ` Christian Gmeiner
2015-06-11 20:48 ` [PATCH 2/4] i2c: Add Congatec CGEB I2C driver Christian Gmeiner
2015-06-12 11:05   ` Wolfram Sang
2015-06-12 11:05     ` Wolfram Sang
2015-06-11 20:48 ` [PATCH 3/4] watchdog: Add Congatec CGEB watchdog driver Christian Gmeiner
2015-06-11 22:04   ` Guenter Roeck
2015-06-11 22:04     ` Guenter Roeck
2015-06-11 20:48 ` [PATCH 4/4] backlight: Add Congatec CGEB backlight driver Christian Gmeiner
2015-06-11 20:48   ` Christian Gmeiner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.