* [PATCH] platform/x86: Add IMS/ZII SCU driver
@ 2017-01-11 21:26 Florian Fainelli
2017-01-13 10:15 ` kbuild test robot
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Florian Fainelli @ 2017-01-11 21:26 UTC (permalink / raw)
To: platform-driver-x86
Cc: cphealy, Guenter Roeck, Florian Fainelli, Darren Hart, open list
From: Guenter Roeck <linux@roeck-us.net>
This patch adds support for the IMS (now Zodiac Inflight Innovations)
SCU Generation 1/2/3 platform driver. This driver registers all the
on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
GPIO expanders, Kontrom CPLD/I2C master, and more.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Darren,
This is against your "for-next" branch thanks!
drivers/platform/x86/Kconfig | 6 +
drivers/platform/x86/Makefile | 2 +
drivers/platform/x86/scu.c | 1626 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 1634 insertions(+)
create mode 100644 drivers/platform/x86/scu.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 5fe8be089b8b..3b3e0cb4b45b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -375,6 +375,12 @@ config SURFACE3_WMI
To compile this driver as a module, choose M here: the module will
be called surface3-wmi.
+config SCU
+ tristate "SCU driver"
+ depends on DMI
+ ---help---
+ Enable SCU driver
+
config THINKPAD_ACPI
tristate "ThinkPad ACPI Laptop Extras"
depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index d4111f0f8a78..06d71af65ce8 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -37,6 +37,8 @@ obj-$(CONFIG_MSI_WMI) += msi-wmi.o
obj-$(CONFIG_SURFACE3_WMI) += surface3-wmi.o
obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o
+obj-$(CONFIG_SCU) += scu.o
+
# toshiba_acpi must link after wmi to ensure that wmi devices are found
# before toshiba_acpi initializes
obj-$(CONFIG_ACPI_TOSHIBA) += toshiba_acpi.o
diff --git a/drivers/platform/x86/scu.c b/drivers/platform/x86/scu.c
new file mode 100644
index 000000000000..30e438061947
--- /dev/null
+++ b/drivers/platform/x86/scu.c
@@ -0,0 +1,1626 @@
+/*
+ * SCU board driver
+ *
+ * Copyright (c) 2012, 2014 Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2016 Zodiac Inflight Innovations
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/leds.h>
+#include <linux/platform_data/mdio-gpio.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/err.h>
+#include <linux/dmi.h>
+#include <linux/i2c.h>
+#include <linux/i2c-gpio.h>
+#include <linux/version.h>
+#include <linux/platform_data/at24.h>
+#include <linux/platform_data/pca953x.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
+#include <linux/nvmem-consumer.h>
+#include <net/dsa.h>
+#include <asm/processor.h>
+#include <asm/byteorder.h>
+
+#define SCU_EXT_GPIO_BASE(x) (0 + (x) * 8)
+#define SCU_EXT_GPIO(chip, x) (SCU_EXT_GPIO_BASE(chip) + (x))
+
+/* Front panel LEDs */
+#define SCU_RD_LED_GPIO SCU_EXT_GPIO(1, 0)
+#define SCU_WLES_LED_GPIO SCU_EXT_GPIO(1, 1)
+#define SCU_LD_FAIL_LED_GPIO SCU_EXT_GPIO(1, 2)
+/* shared control with PIC */
+#define SCU_SW_LED_GPIO SCU_EXT_GPIO(1, 3)
+
+#define SCU_SD_ACTIVE_1_GPIO SCU_EXT_GPIO(2, 0)
+#define SCU_SD_ERROR_1_GPIO SCU_EXT_GPIO(2, 1)
+#define SCU_SD_ACTIVE_2_GPIO SCU_EXT_GPIO(2, 2)
+#define SCU_SD_ERROR_2_GPIO SCU_EXT_GPIO(2, 3)
+#define SCU_SD_ACTIVE_3_GPIO SCU_EXT_GPIO(2, 4)
+#define SCU_SD_ERROR_3_GPIO SCU_EXT_GPIO(2, 5)
+
+#define SCU_SD_ACTIVE_4_GPIO SCU_EXT_GPIO(3, 0)
+#define SCU_SD_ERROR_4_GPIO SCU_EXT_GPIO(3, 1)
+#define SCU_SD_ACTIVE_5_GPIO SCU_EXT_GPIO(3, 2)
+#define SCU_SD_ERROR_5_GPIO SCU_EXT_GPIO(3, 3)
+#define SCU_SD_ACTIVE_6_GPIO SCU_EXT_GPIO(3, 4)
+#define SCU_SD_ERROR_6_GPIO SCU_EXT_GPIO(3, 5)
+
+struct __packed eeprom_data {
+ unsigned short length; /* 0 - 1 */
+ unsigned char checksum; /* 2 */
+ unsigned char have_gsm_modem; /* 3 */
+ unsigned char have_cdma_modem; /* 4 */
+ unsigned char have_wifi_modem; /* 5 */
+ unsigned char have_rhdd; /* 6 */
+ unsigned char have_dvd; /* 7 */
+ unsigned char have_tape; /* 8 */
+ unsigned char have_humidity_sensor; /* 9 */
+ unsigned char have_fiber_channel; /* 10 */
+ unsigned char lru_part_number[11]; /* 11 - 21 Box Part Number */
+ unsigned char lru_revision[7]; /* 22 - 28 Box Revision */
+ unsigned char lru_serial_number[7]; /* 29 - 35 Box Serial Number */
+ unsigned char lru_date_of_manufacture[7];
+ /* 36 - 42 Box Date of Manufacture */
+ unsigned char board_part_number[11];
+ /* 43 - 53 Base Board Part Number */
+ unsigned char board_revision[7];
+ /* 54 - 60 Base Board Revision */
+ unsigned char board_serial_number[7];
+ /* 61 - 67 Base Board Serial Number */
+ unsigned char board_date_of_manufacture[7];
+ /* 68 - 74 Base Board Date of Manufacture */
+ unsigned char board_updated_date_of_manufacture[7];
+ /* 75 - 81 Updated Box Date of Manufacture */
+ unsigned char board_updated_revision[7];
+ /* 82 - 88 Updated Box Revision */
+ unsigned char dummy[7]; /* 89 - 95 spare/filler */
+};
+
+enum scu_version { scu1, scu2, scu3, unknown };
+
+struct scu_data;
+
+struct scu_platform_data {
+ const char *board_type;
+ const char *lru_part_number;
+ const char *board_part_number;
+ enum scu_version version;
+ int eeprom_len;
+ struct i2c_board_info *i2c_board_info;
+ int num_i2c_board_info;
+ struct spi_board_info *spi_board_info;
+ int num_spi_board_info;
+ void (*init)(struct scu_data *);
+ void (*remove)(struct scu_data *);
+};
+
+struct scu_data {
+ struct device *dev; /* SCU platform device */
+ struct net_device *netdev; /* Ethernet device */
+ struct platform_device *mdio_dev; /* MDIO device */
+ struct platform_device *dsa_dev; /* DSA device */
+ struct proc_dir_entry *rave_proc_dir;
+ struct mutex write_lock;
+ struct platform_device *leds_pdev[3];
+ struct i2c_adapter *adapter; /* I2C adapter */
+ struct spi_master *master; /* SPI master */
+ struct i2c_client *client[10]; /* I2C clients */
+ struct spi_device *spidev[1]; /* SPI devices */
+ const struct scu_platform_data *pdata;
+ bool have_write_magic;
+ struct eeprom_data eeprom;
+ struct nvmem_device *nvmem;
+ bool eeprom_accessible;
+ bool eeprom_valid;
+};
+
+#define SCU_EEPROM_LEN_EEPROM 36
+#define SCU_EEPROM_LEN_GEN1 36
+#define SCU_EEPROM_LEN_GEN2 75
+#define SCU_EEPROM_LEN_GEN3 75 /* Preliminary */
+
+#define SCU_LRU_PARTNUM_GEN1 "00-5001"
+#define SCU_LRU_PARTNUM_GEN2 "00-5010"
+#define SCU_LRU_PARTNUM_GEN3 "00-5013"
+
+#define SCU_ZII_BOARD_PARTNUM "05-0041"
+
+#define SCU_WRITE_MAGIC 5482328594ULL
+
+/* sysfs */
+
+unsigned char scu_get_checksum(unsigned char *ptr, int len)
+{
+ unsigned char checksum = 0;
+ int i;
+
+ for (i = 0; i < len; i++)
+ checksum += ptr[i];
+ return checksum;
+}
+
+static int scu_update_checksum(struct scu_data *data)
+{
+ unsigned char checksum;
+ int ret;
+
+ data->eeprom.checksum = 0;
+ checksum = scu_get_checksum((unsigned char *)&data->eeprom,
+ data->pdata->eeprom_len);
+ data->eeprom.checksum = ~checksum + 1;
+ ret = nvmem_device_write(data->nvmem,
+ 0x300 + offsetof(struct eeprom_data, checksum),
+ sizeof(data->eeprom.checksum),
+ &data->eeprom.checksum);
+ if (ret <= 0)
+ return ret < 0 ? ret : -EIO;
+ return 0;
+}
+
+static ssize_t board_type_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", data->pdata->board_type);
+}
+
+static ssize_t attribute_magic_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", data->have_write_magic);
+}
+
+static ssize_t attribute_magic_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ unsigned long long magic;
+ int err;
+
+ err = kstrtoull(buf, 10, &magic);
+ if (err)
+ return err;
+
+ data->have_write_magic = magic == SCU_WRITE_MAGIC;
+
+ return count;
+}
+
+static ssize_t scu_object_show(char *buf, char *data, int len)
+{
+ return snprintf(buf, PAGE_SIZE, "%.*s\n", len, data);
+}
+
+static ssize_t scu_object_store(struct scu_data *data, int offset,
+ const char *in, char *out, ssize_t len)
+{
+ char buffer[12] = { 0 };
+ char *cp;
+ int ret;
+
+ if (!data->have_write_magic)
+ return -EACCES;
+
+ strlcpy(buffer, in, sizeof(buffer));
+ /* do not copy newline from input string */
+ cp = strchr(buffer, '\n');
+ if (cp)
+ *cp = '\0';
+
+ if (len > sizeof(buffer))
+ len = sizeof(buffer);
+
+ mutex_lock(&data->write_lock);
+ strncpy(out, buffer, len);
+
+ /* Write entire eeprom if it was marked invalid */
+ if (!data->eeprom_valid) {
+ offset = 0;
+ /* Write checksumed and non checksumed data */
+ len = sizeof(data->eeprom);
+ out = (char *)&data->eeprom;
+ }
+
+ ret = nvmem_device_write(data->nvmem, 0x300 + offset, len, out);
+ if (ret <= 0) {
+ data->eeprom_valid = false;
+ if (ret == 0)
+ ret = -EIO;
+ goto error;
+ }
+ if (offset < data->pdata->eeprom_len) {
+ /*
+ * Write to checksummed area of eeprom
+ * Update checksum
+ */
+ ret = scu_update_checksum(data);
+ if (ret < 0) {
+ data->eeprom_valid = false;
+ goto error;
+ }
+ }
+ data->eeprom_valid = true;
+error:
+ mutex_unlock(&data->write_lock);
+ return ret;
+}
+
+static ssize_t lru_part_number_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.lru_part_number,
+ sizeof(data->eeprom.lru_part_number));
+}
+
+static ssize_t lru_part_number_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data, lru_part_number),
+ buf, data->eeprom.lru_part_number,
+ sizeof(data->eeprom.lru_part_number));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t lru_serial_number_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.lru_serial_number,
+ sizeof(data->eeprom.lru_serial_number));
+}
+
+static ssize_t lru_serial_number_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data, lru_serial_number),
+ buf, data->eeprom.lru_serial_number,
+ sizeof(data->eeprom.lru_serial_number));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t lru_revision_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.lru_revision,
+ sizeof(data->eeprom.lru_revision));
+}
+
+static ssize_t lru_revision_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data, lru_revision),
+ buf, data->eeprom.lru_revision,
+ sizeof(data->eeprom.lru_revision));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t lru_date_of_manufacture_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.lru_date_of_manufacture,
+ sizeof(data->eeprom.lru_date_of_manufacture));
+}
+
+static ssize_t lru_date_of_manufacture_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data,
+ lru_date_of_manufacture),
+ buf, data->eeprom.lru_date_of_manufacture,
+ sizeof(data->eeprom.lru_date_of_manufacture));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t board_part_number_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.board_part_number,
+ sizeof(data->eeprom.board_part_number));
+}
+
+static ssize_t board_part_number_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data, board_part_number),
+ buf, data->eeprom.board_part_number,
+ sizeof(data->eeprom.board_part_number));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t board_serial_number_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.board_serial_number,
+ sizeof(data->eeprom.board_serial_number));
+}
+
+static ssize_t board_serial_number_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data, board_serial_number),
+ buf, data->eeprom.board_serial_number,
+ sizeof(data->eeprom.board_serial_number));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t board_revision_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.board_revision,
+ sizeof(data->eeprom.board_revision));
+}
+
+static ssize_t board_revision_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data, board_revision),
+ buf, data->eeprom.board_revision,
+ sizeof(data->eeprom.board_revision));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t board_date_of_manufacture_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.board_date_of_manufacture,
+ sizeof(data->eeprom.board_date_of_manufacture));
+}
+
+static ssize_t board_date_of_manufacture_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data,
+ board_date_of_manufacture),
+ buf, data->eeprom.board_date_of_manufacture,
+ sizeof(data->eeprom.board_date_of_manufacture));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t
+board_updated_revision_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf, data->eeprom.board_updated_revision,
+ sizeof(data->eeprom.board_updated_revision));
+}
+
+static ssize_t
+board_updated_revision_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data,
+ board_updated_revision),
+ buf, data->eeprom.board_updated_revision,
+ sizeof(data->eeprom.board_updated_revision));
+ return ret < 0 ? ret : count;
+}
+
+static ssize_t
+board_updated_date_of_manufacture_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ return scu_object_show(buf,
+ data->eeprom.board_updated_date_of_manufacture,
+ sizeof(data->eeprom.board_updated_date_of_manufacture));
+}
+
+static ssize_t
+board_updated_date_of_manufacture_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct scu_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ ret = scu_object_store(data,
+ offsetof(struct eeprom_data,
+ board_updated_date_of_manufacture),
+ buf, data->eeprom.board_updated_date_of_manufacture,
+ sizeof(data->eeprom.board_updated_date_of_manufacture));
+ return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_RO(board_type);
+static DEVICE_ATTR_RW(attribute_magic);
+static DEVICE_ATTR_RW(lru_part_number);
+static DEVICE_ATTR_RW(lru_serial_number);
+static DEVICE_ATTR_RW(lru_revision);
+static DEVICE_ATTR_RW(lru_date_of_manufacture); /* SCU2/SCU3 only */
+static DEVICE_ATTR_RW(board_part_number); /* SCU2/SCU3 only */
+static DEVICE_ATTR_RW(board_serial_number); /* SCU2/SCU3 only */
+static DEVICE_ATTR_RW(board_revision); /* SCU2/SCU3 only */
+static DEVICE_ATTR_RW(board_date_of_manufacture); /* SCU2/SCU3 only */
+static DEVICE_ATTR_RW(board_updated_revision); /* SCU2/SCU3 only */
+static DEVICE_ATTR_RW(board_updated_date_of_manufacture); /* SCU2/SCU3 only */
+
+static struct attribute *scu_base_attrs[] = {
+ &dev_attr_board_type.attr,
+ NULL,
+};
+
+static struct attribute_group scu_base_group = {
+ .attrs = scu_base_attrs,
+};
+
+static struct attribute *scu_eeprom_attrs[] = {
+ &dev_attr_attribute_magic.attr,
+ &dev_attr_lru_part_number.attr, /* 1 */
+ &dev_attr_lru_serial_number.attr,
+ &dev_attr_lru_revision.attr,
+ &dev_attr_lru_date_of_manufacture.attr, /* 4 */
+ &dev_attr_board_part_number.attr,
+ &dev_attr_board_serial_number.attr,
+ &dev_attr_board_revision.attr,
+ &dev_attr_board_date_of_manufacture.attr,
+ &dev_attr_board_updated_revision.attr,
+ &dev_attr_board_updated_date_of_manufacture.attr,
+ NULL
+};
+
+static umode_t scu_attr_is_visible(struct kobject *kobj, struct attribute *attr,
+ int index)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct scu_data *data = dev_get_drvdata(dev);
+ umode_t mode = attr->mode;
+
+ /*
+ * If the eeprom has not been processed, disable its attributes.
+ * If it has been processed but is not accessible, disable
+ * write accesses to it.
+ */
+ if (index >= 1 && !data->eeprom_accessible)
+ mode &= 0444;
+
+ if (index >= 4 && data->pdata->version == scu1)
+ return 0;
+
+ return mode;
+}
+
+static struct attribute_group scu_eeprom_group = {
+ .attrs = scu_eeprom_attrs,
+ .is_visible = scu_attr_is_visible,
+};
+
+#define SCU_EEPROM_TEST_SCRATCHPAD_SIZE 32
+
+static ssize_t scu_eeprom_test_scratchpad_read(struct file *filp,
+ struct kobject *kobj,
+ struct bin_attribute *attr,
+ char *buf, loff_t off,
+ size_t count)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ if (count == 0)
+ return 0;
+
+ if (off >= attr->size)
+ return -EFBIG;
+
+ if (off + count >= SCU_EEPROM_TEST_SCRATCHPAD_SIZE)
+ count = SCU_EEPROM_TEST_SCRATCHPAD_SIZE - off;
+
+ return nvmem_device_read(data->nvmem, off, count, buf);
+}
+
+static ssize_t scu_eeprom_test_scratchpad_write(struct file *filp,
+ struct kobject *kobj,
+ struct bin_attribute *attr,
+ char *buf, loff_t off,
+ size_t count)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct scu_data *data = dev_get_drvdata(dev);
+
+ if (count == 0)
+ return 0;
+
+ if (off >= attr->size)
+ return -EFBIG;
+
+ if (off + count >= SCU_EEPROM_TEST_SCRATCHPAD_SIZE)
+ count = SCU_EEPROM_TEST_SCRATCHPAD_SIZE - off;
+
+ return nvmem_device_write(data->nvmem, off, count, buf);
+}
+
+/* base offset for 32 byte "eeprom_test_scratchpad" file is 0 */
+
+static struct bin_attribute scu_eeprom_test_scratchpad_file = {
+ .attr = {
+ .name = "eeprom_test_scratchpad",
+ .mode = 0644,
+ },
+ .size = SCU_EEPROM_TEST_SCRATCHPAD_SIZE,
+ .read = scu_eeprom_test_scratchpad_read,
+ .write = scu_eeprom_test_scratchpad_write,
+};
+
+/* platform data */
+
+static struct gpio_led pca_gpio_leds1[] = {
+ { /* bit 0 */
+ .name = "scu_status:g:RD",
+ .gpio = SCU_RD_LED_GPIO,
+ .active_low = 1,
+ .default_trigger = "heartbeat",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 1 */
+ .name = "scu_status:a:WLess",
+ .gpio = SCU_WLES_LED_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 2 */
+ .name = "scu_status:r:LDFail",
+ .gpio = SCU_LD_FAIL_LED_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 3 */
+ .name = "scu_status:a:SW",
+ .gpio = SCU_SW_LED_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ }
+};
+
+static struct gpio_led_platform_data pca_gpio_led_info1 = {
+ .leds = pca_gpio_leds1,
+ .num_leds = ARRAY_SIZE(pca_gpio_leds1),
+};
+
+static struct gpio_led pca_gpio_leds2[] = {
+ { /* bit 0 */
+ .name = "SD1:g:active",
+ .gpio = SCU_SD_ACTIVE_1_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 1 */
+ .name = "SD1:a:error",
+ .gpio = SCU_SD_ERROR_1_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 2 */
+ .name = "SD2:g:active",
+ .gpio = SCU_SD_ACTIVE_2_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 3 */
+ .name = "SD2:a:error",
+ .gpio = SCU_SD_ERROR_2_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 4 */
+ .name = "SD3:g:active",
+ .gpio = SCU_SD_ACTIVE_3_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 5 */
+ .name = "SD3:a:error",
+ .gpio = SCU_SD_ERROR_3_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ }
+};
+
+static struct gpio_led_platform_data pca_gpio_led_info2 = {
+ .leds = pca_gpio_leds2,
+ .num_leds = ARRAY_SIZE(pca_gpio_leds2),
+};
+
+static struct gpio_led pca_gpio_leds3[] = {
+ { /* bit 0 */
+ .name = "SD4:g:active",
+ .gpio = SCU_SD_ACTIVE_4_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 1 */
+ .name = "SD4:a:error",
+ .gpio = SCU_SD_ERROR_4_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 2 */
+ .name = "SD5:g:active",
+ .gpio = SCU_SD_ACTIVE_5_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 3 */
+ .name = "SD5:a:error",
+ .gpio = SCU_SD_ERROR_5_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 4 */
+ .name = "SD6:g:active",
+ .gpio = SCU_SD_ACTIVE_6_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ },
+ { /* bit 5 */
+ .name = "SD6:a:error",
+ .gpio = SCU_SD_ERROR_6_GPIO,
+ .active_low = 1,
+ .default_trigger = "none",
+ .default_state = LEDS_GPIO_DEFSTATE_OFF,
+ }
+};
+
+static struct gpio_led_platform_data pca_gpio_led_info3 = {
+ .leds = pca_gpio_leds3,
+ .num_leds = ARRAY_SIZE(pca_gpio_leds3),
+};
+
+static void pca_leds_register(struct device *parent,
+ struct scu_data *data)
+{
+ data->leds_pdev[0] =
+ platform_device_register_data(parent, "leds-gpio", 1,
+ &pca_gpio_led_info1,
+ sizeof(pca_gpio_led_info1));
+ data->leds_pdev[1] =
+ platform_device_register_data(parent, "leds-gpio", 2,
+ &pca_gpio_led_info2,
+ sizeof(pca_gpio_led_info2));
+ data->leds_pdev[2] =
+ platform_device_register_data(parent, "leds-gpio", 3,
+ &pca_gpio_led_info3,
+ sizeof(pca_gpio_led_info3));
+}
+
+static void pca_leds_unregister(struct scu_data *data)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(data->leds_pdev); i++)
+ platform_device_unregister(data->leds_pdev[i]);
+}
+
+static const char *pca9538_ext0_gpio_names[8] = {
+ "pca9538_ext0:wireless_ena_1",
+ "pca9538_ext0:wireless_ena_2",
+ "pca9538_ext0:wireless_a_radio_disable",
+ "pca9538_ext0:wireless_a_reset",
+ "pca9538_ext0:in_spare_1",
+ "pca9538_ext0:in_spare_2",
+ "pca9538_ext0:wireless_b_radio_disable",
+ "pca9538_ext0:wireless_b_reset",
+};
+
+static const char *pca9538_ext1_gpio_names[8] = {
+ "pca9538_ext1:rd_led_on",
+ "pca9538_ext1:wless_led_on",
+ "pca9538_ext1:ld_fail_led_on",
+ "pca9538_ext1:sw_led_on",
+ "pca9538_ext1:discrete_out_1",
+ "pca9538_ext1:discrete_out_2",
+ "pca9538_ext1:discrete_out_3",
+ "pca9538_ext1:discrete_out_4",
+};
+
+static const char *pca9538_ext2_gpio_names[8] = {
+ "pca9538_ext2:sd_active_1",
+ "pca9538_ext2:sd_error_1",
+ "pca9538_ext2:sd_active_2",
+ "pca9538_ext2:sd_error_2",
+ "pca9538_ext2:sd_active_3",
+ "pca9538_ext2:sd_error_3",
+ "pca9538_ext2:hub_6_reset",
+ "pca9538_ext2:hub_6_config_status",
+};
+
+static const char *pca9538_ext3_gpio_names[8] = {
+ "pca9538_ext3:sd_active_4",
+ "pca9538_ext3:sd_error_4",
+ "pca9538_ext3:sd_active_5",
+ "pca9538_ext3:sd_error_5",
+ "pca9538_ext3:sd_active_6",
+ "pca9538_ext3:sd_error_6",
+ "pca9538_ext3:hub_2_reset",
+ "pca9538_ext3:hub_2_config_status",
+};
+
+static const char *pca9557_gpio_names[8] = {
+ "pca9557:sd_card_detect_1",
+ "pca9557:sd_card_detect_2",
+ "pca9557:sd_card_detect_3",
+ "pca9557:sd_card_detect_4",
+ "pca9557:sd_card_detect_5",
+ "pca9557:sd_card_detect_6",
+ "pca9557:spare1",
+ "pca9557:spare2",
+};
+
+static int scu_gpio_common_setup(unsigned int gpio_base, unsigned int ngpio,
+ u32 mask, u32 is_input, u32 is_active,
+ u32 active_low)
+{
+ int i;
+ unsigned long flags;
+
+ for (i = 0; i < ngpio; i++) {
+ if (!(mask & (1 << i)))
+ continue;
+ flags = GPIOF_EXPORT_DIR_FIXED;
+ if (is_input & (1 << i)) {
+ flags |= GPIOF_DIR_IN;
+ } else {
+ flags |= GPIOF_DIR_OUT;
+ if (is_active & (1 << i))
+ flags |= GPIOF_INIT_HIGH;
+ }
+ if (active_low & (1 << i))
+ flags |= GPIOF_ACTIVE_LOW;
+ gpio_request_one(gpio_base + i, flags, NULL);
+ }
+ return 0;
+}
+
+static int pca9538_ext0_setup(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_setup(gpio_base, ngpio, 0xff, 0x33, 0xcc, 0x00);
+ return 0;
+}
+
+static int pca9538_ext1_setup(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_setup(gpio_base, ngpio, 0xf0, 0x00, 0x00, 0x00);
+ return 0;
+}
+
+static int pca9538_ext2_setup(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_setup(gpio_base, ngpio, 0xc0, 0x80, 0x40, 0x00);
+ return 0;
+}
+
+static int pca9538_ext3_setup(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_setup(gpio_base, ngpio, 0xc0, 0x80, 0x40, 0x00);
+ return 0;
+}
+
+static int pca9557_setup(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_setup(gpio_base, ngpio, 0x3f, 0x3f, 0x00, 0x3f);
+ return 0;
+}
+
+static void scu_gpio_common_teardown(unsigned int gpio_base, int ngpio,
+ u32 mask)
+{
+ int i;
+
+ for (i = 0; i < ngpio; i++) {
+ if (mask & (1 << i)) {
+ gpio_unexport(gpio_base + i);
+ gpio_free(gpio_base + i);
+ }
+ }
+}
+
+static int pca9538_ext0_teardown(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_teardown(gpio_base, ngpio, 0xff);
+ return 0;
+}
+
+static int pca9538_ext1_teardown(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_teardown(gpio_base, ngpio, 0xf0);
+ return 0;
+}
+
+static int pca9538_ext2_teardown(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_teardown(gpio_base, ngpio, 0xc0);
+ return 0;
+}
+
+static int pca9538_ext3_teardown(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_teardown(gpio_base, ngpio, 0xc0);
+ return 0;
+}
+
+static int pca9557_teardown(struct i2c_client *client,
+ unsigned int gpio_base, unsigned int ngpio,
+ void *context)
+{
+ scu_gpio_common_teardown(gpio_base, ngpio, 0x3f);
+ return 0;
+}
+
+static int scu_gpiochip_match_name(struct gpio_chip *chip, void *data)
+{
+ return !strcmp(chip->label, data);
+}
+
+static struct gpio_chip *scu_find_chip_by_name(const char *name)
+{
+ return gpiochip_find((void *)name, scu_gpiochip_match_name);
+}
+
+static struct pca953x_platform_data scu_pca953x_pdata[] = {
+ [0] = {.gpio_base = SCU_EXT_GPIO_BASE(0),
+ .irq_base = -1,
+ .setup = pca9538_ext0_setup,
+ .teardown = pca9538_ext0_teardown,
+ .names = pca9538_ext0_gpio_names},
+ [1] = {.gpio_base = SCU_EXT_GPIO_BASE(1),
+ .irq_base = -1,
+ .setup = pca9538_ext1_setup,
+ .teardown = pca9538_ext1_teardown,
+ .names = pca9538_ext1_gpio_names},
+ [2] = {.gpio_base = SCU_EXT_GPIO_BASE(2),
+ .irq_base = -1,
+ .setup = pca9538_ext2_setup,
+ .teardown = pca9538_ext2_teardown,
+ .names = pca9538_ext2_gpio_names},
+ [3] = {.gpio_base = SCU_EXT_GPIO_BASE(3),
+ .irq_base = -1,
+ .setup = pca9538_ext3_setup,
+ .teardown = pca9538_ext3_teardown,
+ .names = pca9538_ext3_gpio_names},
+ [4] = {.gpio_base = SCU_EXT_GPIO_BASE(4),
+ .irq_base = -1,
+ .setup = pca9557_setup,
+ .teardown = pca9557_teardown,
+ .names = pca9557_gpio_names},
+};
+
+static struct i2c_board_info scu_i2c_info_scu3[] = {
+ { I2C_BOARD_INFO("pca9538", 0x70),
+ .platform_data = &scu_pca953x_pdata[0],},
+ { I2C_BOARD_INFO("pca9557", 0x1b),
+ .platform_data = &scu_pca953x_pdata[4],},
+};
+
+static void pch_gpio_setup(struct scu_data *data)
+{
+ struct gpio_chip *chip = scu_find_chip_by_name("gpio_ich");
+ struct mdio_gpio_platform_data pdata = { };
+
+ scu_pca953x_pdata[0].irq_base = -1;
+ scu_i2c_info_scu3[0].irq = 0;
+ if (chip) {
+ int irq = gpio_to_irq(chip->base + 1); /* GPI1 */
+
+ if (irq > 0) {
+ scu_pca953x_pdata[0].irq_base = 0;
+ scu_i2c_info_scu3[0].irq = irq;
+ }
+
+ pdata.mdc = chip->base + 17; /* GPO1 */
+ pdata.mdo = chip->base + 21; /* GPO2 */
+ pdata.mdo_active_low = true;
+ pdata.mdio = chip->base + 2; /* GPI2 */
+
+ data->mdio_dev = platform_device_register_data(&platform_bus,
+ "mdio-gpio", 0,
+ &pdata,
+ sizeof(pdata));
+ if (IS_ERR(data->mdio_dev)) {
+ dev_err(data->dev, "Failed to register MDIO device\n");
+ data->mdio_dev = NULL;
+ }
+ /* generic: 0, 3 (input), 16, 20 (output) */
+ scu_gpio_common_setup(chip->base, 22, 0x110009, 0x00000b,
+ 0x100000, 0x0);
+ }
+}
+
+static void pch_gpio_teardown(struct scu_data *data)
+{
+ struct gpio_chip *chip = scu_find_chip_by_name("gpio_ich");
+
+ if (chip) {
+ platform_device_unregister(data->mdio_dev);
+ scu_gpio_common_teardown(chip->base, 22, 0x30000b);
+ }
+}
+
+static struct dsa_chip_data switch_chip_data = {
+ .eeprom_len = 0x200,
+ .port_names[0] = "cpu",
+ .port_names[1] = "port1",
+ .port_names[2] = "port2",
+ .port_names[3] = "port8",
+ .port_names[4] = "host2esb",
+ .port_names[5] = 0, /* unused */
+};
+
+static void scu_setup_ethernet_switch(struct scu_data *data)
+{
+ struct dsa_platform_data switch_data = {
+ .nr_chips = 1,
+ .chip = &switch_chip_data,
+ };
+
+ switch_data.netdev = &data->netdev->dev;
+ switch_chip_data.host_dev = &data->mdio_dev->dev;
+ data->dsa_dev = platform_device_register_data(&platform_bus, "dsa",
+ 0, &switch_data,
+ sizeof(switch_data));
+ if (IS_ERR(data->dsa_dev)) {
+ dev_err(data->dev, "Failed to register DSA device\n");
+ data->dsa_dev = NULL;
+ }
+}
+
+static void scu3_init(struct scu_data *data)
+{
+ pch_gpio_setup(data);
+ if (data->mdio_dev)
+ scu_setup_ethernet_switch(data);
+}
+
+static void scu3_remove(struct scu_data *data)
+{
+ pch_gpio_teardown(data);
+ platform_device_unregister(data->dsa_dev);
+}
+
+static struct i2c_board_info scu_i2c_info_scu2[] = {
+ { I2C_BOARD_INFO("sc18is602", 0x28)},
+ { I2C_BOARD_INFO("pca9538", 0x70),
+ .platform_data = &scu_pca953x_pdata[0],},
+};
+
+static struct spi_board_info scu_spi_info[] = {
+ {
+ .modalias = "b53-switch",
+ .bus_num = 0,
+ .chip_select = 0,
+ .max_speed_hz = 2000000,
+ .mode = SPI_MODE_3,
+ },
+};
+
+static struct scu_platform_data scu_platform_data[] = {
+ [scu1] = {
+ .board_type = "SCU1 x86",
+ .lru_part_number = SCU_LRU_PARTNUM_GEN1,
+ .version = scu1,
+ .eeprom_len = SCU_EEPROM_LEN_GEN1,
+ .i2c_board_info = scu_i2c_info_scu2,
+ .num_i2c_board_info = ARRAY_SIZE(scu_i2c_info_scu2),
+ .spi_board_info = scu_spi_info,
+ .num_spi_board_info = ARRAY_SIZE(scu_spi_info),
+ },
+ [scu2] = {
+ .board_type = "SCU2 x86",
+ .lru_part_number = SCU_LRU_PARTNUM_GEN2,
+ .version = scu2,
+ .eeprom_len = SCU_EEPROM_LEN_GEN2,
+ .i2c_board_info = scu_i2c_info_scu2,
+ .num_i2c_board_info = ARRAY_SIZE(scu_i2c_info_scu2),
+ .spi_board_info = scu_spi_info,
+ .num_spi_board_info = ARRAY_SIZE(scu_spi_info),
+ },
+ [scu3] = {
+ .board_type = "SCU3 x86",
+ .lru_part_number = SCU_LRU_PARTNUM_GEN3,
+ .board_part_number = SCU_ZII_BOARD_PARTNUM,
+ .version = scu3,
+ .eeprom_len = SCU_EEPROM_LEN_GEN3,
+ .i2c_board_info = scu_i2c_info_scu3,
+ .num_i2c_board_info = ARRAY_SIZE(scu_i2c_info_scu3),
+ .init = scu3_init,
+ .remove = scu3_remove,
+ },
+ [unknown] = {
+ .board_type = "UNKNOWN",
+ .version = unknown,
+ .eeprom_len = SCU_EEPROM_LEN_GEN3,
+ .i2c_board_info = scu_i2c_info_scu3,
+ .num_i2c_board_info = ARRAY_SIZE(scu_i2c_info_scu3),
+ .init = scu3_init,
+ .remove = scu3_remove,
+ },
+};
+
+static int scu_instantiate_i2c(struct scu_data *data, int base,
+ struct i2c_board_info *info, int count)
+{
+ int i;
+
+ for (i = 0; i < count; i++) {
+ data->client[base + i] = i2c_new_device(data->adapter, info);
+ if (!data->client[base + i]) {
+ /*
+ * Unfortunately this call does not tell us
+ * why it failed. Pick the most likely reason.
+ */
+ return -EBUSY;
+ }
+ info++;
+ }
+ return 0;
+}
+
+static int scu_instantiate_spi(struct scu_data *data,
+ struct spi_board_info *info, int count)
+{
+ struct spi_master *master;
+ int i;
+
+ /* SPI bus number matches i2c bus number (set by sc18is602 driver) */
+ master = spi_busnum_to_master(data->adapter->nr);
+ if (!master) {
+ dev_err(data->dev, "Failed to find SPI adapter\n");
+ return -ENODEV;
+ }
+ data->master = master;
+
+ for (i = 0; i < count; i++) {
+ info->bus_num = master->bus_num;
+ /* ignore errors */
+ data->spidev[i] = spi_new_device(master, info);
+ info++;
+ }
+ return 0;
+}
+
+static void populate_unit_info(struct nvmem_device *nvmem,
+ void *context);
+
+static struct at24_platform_data at24c08 = {
+ .byte_len = 1024,
+ .page_size = 16,
+ .setup = populate_unit_info,
+};
+
+static struct i2c_board_info scu_i2c_info_common[] = {
+ { I2C_BOARD_INFO("scu_pic", 0x20)},
+ { I2C_BOARD_INFO("at24", 0x54),
+ .platform_data = &at24c08},
+ { I2C_BOARD_INFO("ds1682", 0x6b)},
+ { I2C_BOARD_INFO("pca9538", 0x71),
+ .platform_data = &scu_pca953x_pdata[1],},
+ { I2C_BOARD_INFO("pca9538", 0x72),
+ .platform_data = &scu_pca953x_pdata[2],},
+ { I2C_BOARD_INFO("pca9538", 0x73),
+ .platform_data = &scu_pca953x_pdata[3],},
+};
+
+/*
+ * This is the callback function when a a specifc at24 eeprom is found.
+ * Its reads out the eeprom contents via the read function passed back in via
+ * struct memory_accessor. It then calls part_number_proc, serial_number_proc,
+ * and dom_proc to populate the procfs entries for each specific field.
+ */
+static void populate_unit_info(struct nvmem_device *nvmem,
+ void *context)
+{
+ const struct scu_platform_data *pdata = &scu_platform_data[unknown];
+ struct scu_data *data = context;
+ unsigned char *ptr;
+ int i, len;
+
+ data->nvmem = nvmem;
+
+ ptr = (unsigned char *)&data->eeprom;
+ /* Read Data structure from EEPROM */
+ if (nvmem_device_read(nvmem, 0x300, sizeof(data->eeprom), ptr) <= 0) {
+ dev_err(data->dev, "Failed to read eeprom data\n");
+ goto error;
+ }
+
+ /* EEPROM is accessible, permit write access to it */
+ data->eeprom_accessible = true;
+
+ /* Special case - eeprom not programmed */
+ if (data->eeprom.length == 0xffff && data->eeprom.checksum == 0xff) {
+ /* Assume it is SCU3, but report different board type */
+ memset(&data->eeprom, '\0', sizeof(data->eeprom));
+ data->eeprom.length = cpu_to_le16(SCU_EEPROM_LEN_EEPROM);
+ goto unprogrammed;
+ }
+
+ /* Sanity check */
+ if (le16_to_cpu(data->eeprom.length) != SCU_EEPROM_LEN_EEPROM) {
+ dev_err(data->dev,
+ "Bad eeprom data length: Expected %d, got %d\n",
+ SCU_EEPROM_LEN_EEPROM,
+ le16_to_cpu(data->eeprom.length));
+ goto error;
+ }
+
+ /* Update platform data based on part number retrieved from EEPROM */
+ for (i = 0; i < ARRAY_SIZE(scu_platform_data); i++) {
+ const struct scu_platform_data *tpdata = &scu_platform_data[i];
+
+ if (tpdata->lru_part_number == NULL)
+ continue;
+ if (!strncmp(data->eeprom.lru_part_number,
+ tpdata->lru_part_number,
+ strlen(tpdata->lru_part_number))) {
+ pdata = tpdata;
+ break;
+ }
+ }
+
+unprogrammed:
+ data->pdata = pdata;
+ /*
+ * We know as much as we will ever find out about the platform.
+ * Perform final platform initialization and instantiate additional
+ * I2C devices as well as SPI devices now, prior to validating
+ * the EEPROM checksum.
+ */
+ if (pdata->init)
+ pdata->init(data);
+
+ if (pdata->i2c_board_info)
+ scu_instantiate_i2c(data, ARRAY_SIZE(scu_i2c_info_common),
+ pdata->i2c_board_info,
+ pdata->num_i2c_board_info);
+
+ if (pdata->spi_board_info)
+ scu_instantiate_spi(data, pdata->spi_board_info,
+ pdata->num_spi_board_info);
+
+ len = data->pdata->eeprom_len;
+
+ /* Validate checksum */
+ if (scu_get_checksum(ptr, len)) {
+ dev_err(data->dev,
+ "EEPROM data checksum error: expected 0, got 0x%x [len=%d]\n",
+ scu_get_checksum(ptr, len), len);
+ /* TBD: Do we want to clear the eeprom in this case ? */
+ goto error_noclean;
+ }
+
+ /* Create sysfs attributes based on retrieved platform data */
+ data->eeprom_valid = true;
+ goto done;
+
+error:
+ memset(&data->eeprom, '\0', sizeof(data->eeprom));
+ data->eeprom.length = cpu_to_le16(SCU_EEPROM_LEN_EEPROM);
+error_noclean:
+ data->eeprom_valid = false;
+done:
+ if (sysfs_create_group(&data->dev->kobj, &scu_eeprom_group))
+ ;
+ if (sysfs_create_bin_file(&data->dev->kobj,
+ &scu_eeprom_test_scratchpad_file))
+ ;
+}
+
+static int scu_i2c_adap_name_match(struct device *dev, void *data)
+{
+ struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+ if (!adap)
+ return false;
+
+ return !strcmp(adap->name, (char *)data);
+}
+
+static struct i2c_adapter *scu_find_i2c_adapter(char *name)
+{
+ struct device *dev;
+ struct i2c_adapter *adap;
+
+ dev = bus_find_device(&i2c_bus_type, NULL, name,
+ scu_i2c_adap_name_match);
+ if (!dev)
+ return NULL;
+
+ adap = i2c_verify_adapter(dev);
+ if (!adap)
+ put_device(dev);
+
+ return adap;
+}
+
+const char *scu_modules[] = {
+ "kempld-core",
+ "i2c-kempld",
+ "spi-sc18is602",
+ "lpc_ich",
+ "gpio_ich",
+ "mdio-gpio",
+ "dsa",
+ NULL
+};
+
+#ifdef CONFIG_MODULES
+static void scu_request_modules(bool wait)
+{
+ struct module *m;
+ int i;
+
+ /*
+ * Try to load modules which we are going to need later on.
+ * Fail silently; if loading the module is not successful
+ * we'll bail out later on.
+ */
+ for (i = 0; scu_modules[i]; i++) {
+ mutex_lock(&module_mutex);
+ m = find_module(scu_modules[i]);
+ mutex_unlock(&module_mutex);
+ if (!m) {
+ if (wait)
+ request_module(scu_modules[i]);
+ else
+ request_module_nowait(scu_modules[i]);
+ }
+ }
+}
+#else
+static inline void scu_request_modules(bool wait) { }
+#endif
+
+static int proc_board_type_show(struct seq_file *m, void *v)
+{
+ struct scu_data *data = (struct scu_data *)m->private;
+
+ seq_printf(m, "%s\n", data->pdata->board_type);
+
+ return 0;
+}
+
+static int scu_proc_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, proc_board_type_show, PDE_DATA(inode));
+}
+
+static const struct file_operations scu_proc_fops = {
+ .owner = THIS_MODULE,
+ .open = scu_proc_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int scu_probe(struct platform_device *pdev)
+{
+ struct proc_dir_entry *rave_board_type;
+ struct device *dev = &pdev->dev;
+ struct i2c_adapter *adapter;
+ struct net_device *ndev;
+ struct scu_data *data;
+ int i, ret;
+
+ scu_request_modules(true);
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, data);
+
+ data->dev = dev;
+
+ mutex_init(&data->write_lock);
+
+ /* look for ethernet device attached to 'e1000e' driver */
+ rtnl_lock();
+ for_each_netdev(&init_net, ndev) {
+ if (ndev->dev.parent && ndev->dev.parent->driver &&
+ !strcmp(ndev->dev.parent->driver->name, "e1000e")) {
+ data->netdev = ndev;
+ break;
+ }
+ }
+ rtnl_unlock();
+
+ if (!data->netdev)
+ return -EPROBE_DEFER;
+
+ /*
+ * The adapter driver should have been loaded by now.
+ * If not, try again later.
+ */
+ adapter = scu_find_i2c_adapter("i2c-kempld");
+ if (!adapter) {
+ ret = -EPROBE_DEFER;
+ goto error_put_net;
+ }
+ data->adapter = adapter;
+
+ data->rave_proc_dir = proc_mkdir("rave", NULL);
+ if (!data->rave_proc_dir) {
+ ret = -ENODEV;
+ goto error_put;
+ }
+ rave_board_type = proc_create_data("board_type", 0, data->rave_proc_dir,
+ &scu_proc_fops, data);
+ if (rave_board_type == NULL) {
+ ret = -ENODEV;
+ goto error_remove;
+ }
+
+ at24c08.context = data;
+
+ ret = scu_instantiate_i2c(data, 0, scu_i2c_info_common,
+ ARRAY_SIZE(scu_i2c_info_common));
+ if (ret)
+ goto error_i2c_client;
+
+ pca_leds_register(dev, data);
+
+ ret = sysfs_create_group(&dev->kobj, &scu_base_group);
+ if (ret)
+ goto error_group;
+
+ return 0;
+
+error_group:
+ pca_leds_unregister(data);
+error_i2c_client:
+ for (i = 0; i < ARRAY_SIZE(data->client) && data->client[i]; i++)
+ i2c_unregister_device(data->client[i]);
+error_remove:
+ proc_remove(data->rave_proc_dir);
+error_put:
+ put_device(&adapter->dev);
+error_put_net:
+ dev_put(data->netdev);
+ return ret;
+}
+
+static int __exit scu_remove(struct platform_device *pdev)
+{
+ struct scu_data *data = platform_get_drvdata(pdev);
+ int i;
+
+ sysfs_remove_bin_file(&data->dev->kobj,
+ &scu_eeprom_test_scratchpad_file);
+ sysfs_remove_group(&pdev->dev.kobj, &scu_eeprom_group);
+ sysfs_remove_group(&pdev->dev.kobj, &scu_base_group);
+
+ if (data->pdata && data->pdata->remove)
+ data->pdata->remove(data);
+
+ pca_leds_unregister(data);
+
+ for (i = 0; i < ARRAY_SIZE(data->spidev); i++)
+ spi_unregister_device(data->spidev[i]);
+ spi_master_put(data->master);
+
+ for (i = 0; i < ARRAY_SIZE(data->client) && data->client[i]; i++)
+ i2c_unregister_device(data->client[i]);
+
+ proc_remove(data->rave_proc_dir);
+
+ put_device(&data->adapter->dev);
+ dev_put(data->netdev);
+
+ return 0;
+}
+
+static struct platform_driver scu_driver = {
+ .probe = scu_probe,
+ .remove = __exit_p(scu_remove),
+ .driver = {
+ .name = "scu",
+ .owner = THIS_MODULE,
+ },
+};
+
+static struct platform_device *scu_pdev;
+
+static int scu_create_platform_device(const struct dmi_system_id *id)
+{
+ int ret;
+
+ scu_pdev = platform_device_alloc("scu", -1);
+ if (!scu_pdev)
+ return -ENOMEM;
+
+ ret = platform_device_add(scu_pdev);
+ if (ret)
+ goto err;
+
+ return 0;
+err:
+ platform_device_put(scu_pdev);
+ return ret;
+}
+
+static const struct dmi_system_id scu_device_table[] __initconst = {
+ {
+ .ident = "IMS SCU version 1, Core 2 Duo",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Kontron"),
+ DMI_MATCH(DMI_BOARD_NAME, "PXT"),
+ },
+ .callback = scu_create_platform_device,
+ },
+ {
+ .ident = "IMS SCU version 2, Ivy Bridge",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Kontron"),
+ DMI_MATCH(DMI_BOARD_NAME, "COMe-bSC6"),
+ },
+ .callback = scu_create_platform_device,
+ },
+ {
+ .ident = "IMS SCU version 2, Ivy Bridge",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Kontron"),
+ DMI_MATCH(DMI_BOARD_NAME, "COMe-bIP2"),
+ },
+ .callback = scu_create_platform_device,
+ },
+ {
+ .ident = "IMS SCU version 2, Sandy Bridge",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Kontron"),
+ DMI_MATCH(DMI_BOARD_NAME, "COMe-bSC2"),
+ },
+ .callback = scu_create_platform_device,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(dmi, scu_device_table);
+
+static int __init scu_init(void)
+{
+ if (!dmi_check_system(scu_device_table))
+ return -ENODEV;
+
+ scu_request_modules(false);
+
+ return platform_driver_register(&scu_driver);
+}
+module_init(scu_init);
+
+static void __exit scu_exit(void)
+{
+ if (scu_pdev)
+ platform_device_unregister(scu_pdev);
+
+ platform_driver_unregister(&scu_driver);
+}
+module_exit(scu_exit);
+
+MODULE_ALIAS("platform:scu");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Guenter Roeck");
+MODULE_DESCRIPTION("IMS SCU platform driver");
+MODULE_DEVICE_TABLE(dmi, scu_device_table);
--
2.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-11 21:26 [PATCH] platform/x86: Add IMS/ZII SCU driver Florian Fainelli
@ 2017-01-13 10:15 ` kbuild test robot
2017-01-13 10:15 ` [PATCH] platform/x86: fix platform_no_drv_owner.cocci warnings kbuild test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2017-01-13 10:15 UTC (permalink / raw)
To: Florian Fainelli
Cc: kbuild-all, platform-driver-x86, cphealy, Guenter Roeck,
Florian Fainelli, Darren Hart, open list
Hi Guenter,
[auto build test WARNING on platform-drivers-x86/for-next]
[also build test WARNING on v4.10-rc3 next-20170113]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Florian-Fainelli/platform-x86-Add-IMS-ZII-SCU-driver/20170113-163943
base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/platform/x86/scu.c:1541:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] platform/x86: fix platform_no_drv_owner.cocci warnings
2017-01-11 21:26 [PATCH] platform/x86: Add IMS/ZII SCU driver Florian Fainelli
2017-01-13 10:15 ` kbuild test robot
@ 2017-01-13 10:15 ` kbuild test robot
2017-01-13 16:38 ` [PATCH] platform/x86: Add IMS/ZII SCU driver Andy Shevchenko
2017-01-24 21:08 ` Andy Shevchenko
3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2017-01-13 10:15 UTC (permalink / raw)
To: Florian Fainelli
Cc: kbuild-all, platform-driver-x86, cphealy, Guenter Roeck,
Florian Fainelli, Darren Hart, open list
drivers/platform/x86/scu.c:1541:3-8: No need to set .owner here. The core will do it.
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
scu.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/platform/x86/scu.c
+++ b/drivers/platform/x86/scu.c
@@ -1538,7 +1538,6 @@ static struct platform_driver scu_driver
.remove = __exit_p(scu_remove),
.driver = {
.name = "scu",
- .owner = THIS_MODULE,
},
};
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-11 21:26 [PATCH] platform/x86: Add IMS/ZII SCU driver Florian Fainelli
2017-01-13 10:15 ` kbuild test robot
2017-01-13 10:15 ` [PATCH] platform/x86: fix platform_no_drv_owner.cocci warnings kbuild test robot
@ 2017-01-13 16:38 ` Andy Shevchenko
2017-01-13 18:22 ` Darren Hart
` (2 more replies)
2017-01-24 21:08 ` Andy Shevchenko
3 siblings, 3 replies; 13+ messages in thread
From: Andy Shevchenko @ 2017-01-13 16:38 UTC (permalink / raw)
To: Florian Fainelli
Cc: Platform Driver, cphealy, Guenter Roeck, Darren Hart, open list
On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> This patch adds support for the IMS (now Zodiac Inflight Innovations)
> SCU Generation 1/2/3 platform driver. This driver registers all the
> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
> GPIO expanders, Kontrom CPLD/I2C master, and more.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Darren,
>
> This is against your "for-next" branch thanks!
I'm going to review this later, though few of comments.
> +config SCU
No, no. We have enough mess with Intel's SCU/PMIC here, not add more.
At least add manufacturer as prefix to option and file name.
Btw, Darren, would it be good idea to start creating folders to make a
bit of order in the subsystem? For first I would move Intel's PMIC/SCU
stuff to somewhere (not sure if it should be per manufacturer or per
function).
> +obj-$(CONFIG_SCU) += scu.o
For file name as well.
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/leds.h>
> +#include <linux/platform_data/mdio-gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/err.h>
> +#include <linux/dmi.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-gpio.h>
> +#include <linux/version.h>
> +#include <linux/platform_data/at24.h>
> +#include <linux/platform_data/pca953x.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +#include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/nvmem-consumer.h>
Is it possible to keep them in order?
Do you need all of them?
Does it sound like MFD driver + individual drivers?
> +struct __packed eeprom_data {
> + unsigned short length; /* 0 - 1 */
> + unsigned char checksum; /* 2 */
> + unsigned char have_gsm_modem; /* 3 */
> + unsigned char have_cdma_modem; /* 4 */
> + unsigned char have_wifi_modem; /* 5 */
> + unsigned char have_rhdd; /* 6 */
> + unsigned char have_dvd; /* 7 */
> + unsigned char have_tape; /* 8 */
> + unsigned char have_humidity_sensor; /* 9 */
> + unsigned char have_fiber_channel; /* 10 */
> + unsigned char lru_part_number[11]; /* 11 - 21 Box Part Number */
> + unsigned char lru_revision[7]; /* 22 - 28 Box Revision */
> + unsigned char lru_serial_number[7]; /* 29 - 35 Box Serial Number */
> + unsigned char lru_date_of_manufacture[7];
> + /* 36 - 42 Box Date of Manufacture */
> + unsigned char board_part_number[11];
> + /* 43 - 53 Base Board Part Number */
> + unsigned char board_revision[7];
> + /* 54 - 60 Base Board Revision */
> + unsigned char board_serial_number[7];
> + /* 61 - 67 Base Board Serial Number */
> + unsigned char board_date_of_manufacture[7];
> + /* 68 - 74 Base Board Date of Manufacture */
> + unsigned char board_updated_date_of_manufacture[7];
> + /* 75 - 81 Updated Box Date of Manufacture */
> + unsigned char board_updated_revision[7];
> + /* 82 - 88 Updated Box Revision */
> + unsigned char dummy[7]; /* 89 - 95 spare/filler */
> +};
Would it be better to use fixed-width types here:
u8
u16
> +enum scu_version { scu1, scu2, scu3, unknown };
MANUFACTURER_SCU_VERSION_x
?
> +struct scu_data {
> + struct device *dev; /* SCU platform device */
> + struct net_device *netdev; /* Ethernet device */
> + struct platform_device *mdio_dev; /* MDIO device */
> + struct platform_device *dsa_dev; /* DSA device */
> + struct proc_dir_entry *rave_proc_dir;
> + struct mutex write_lock;
> + struct platform_device *leds_pdev[3];
> + struct i2c_adapter *adapter; /* I2C adapter */
> + struct spi_master *master; /* SPI master */
> + struct i2c_client *client[10]; /* I2C clients */
> + struct spi_device *spidev[1]; /* SPI devices */
Comments are good candidates for kernel doc.
> + const struct scu_platform_data *pdata;
> + bool have_write_magic;
> + struct eeprom_data eeprom;
> + struct nvmem_device *nvmem;
> + bool eeprom_accessible;
> + bool eeprom_valid;
unsigned int flag1:1;
unsigned int flag2:1;
?
> +/* platform data */
> +
> +static struct gpio_led pca_gpio_leds1[] = {
> + { /* bit 0 */
> + .name = "scu_status:g:RD",
> + .gpio = SCU_RD_LED_GPIO,
> + .active_low = 1,
> + .default_trigger = "heartbeat",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 1 */
> + .name = "scu_status:a:WLess",
> + .gpio = SCU_WLES_LED_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 2 */
> + .name = "scu_status:r:LDFail",
> + .gpio = SCU_LD_FAIL_LED_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 3 */
> + .name = "scu_status:a:SW",
> + .gpio = SCU_SW_LED_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + }
> +};
> +
> +static struct gpio_led_platform_data pca_gpio_led_info1 = {
> + .leds = pca_gpio_leds1,
> + .num_leds = ARRAY_SIZE(pca_gpio_leds1),
> +};
> +
> +static struct gpio_led pca_gpio_leds2[] = {
> + { /* bit 0 */
> + .name = "SD1:g:active",
> + .gpio = SCU_SD_ACTIVE_1_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 1 */
> + .name = "SD1:a:error",
> + .gpio = SCU_SD_ERROR_1_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 2 */
> + .name = "SD2:g:active",
> + .gpio = SCU_SD_ACTIVE_2_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 3 */
> + .name = "SD2:a:error",
> + .gpio = SCU_SD_ERROR_2_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 4 */
> + .name = "SD3:g:active",
> + .gpio = SCU_SD_ACTIVE_3_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 5 */
> + .name = "SD3:a:error",
> + .gpio = SCU_SD_ERROR_3_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + }
> +};
> +
> +static struct gpio_led_platform_data pca_gpio_led_info2 = {
> + .leds = pca_gpio_leds2,
> + .num_leds = ARRAY_SIZE(pca_gpio_leds2),
> +};
> +
> +static struct gpio_led pca_gpio_leds3[] = {
> + { /* bit 0 */
> + .name = "SD4:g:active",
> + .gpio = SCU_SD_ACTIVE_4_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 1 */
> + .name = "SD4:a:error",
> + .gpio = SCU_SD_ERROR_4_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 2 */
> + .name = "SD5:g:active",
> + .gpio = SCU_SD_ACTIVE_5_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 3 */
> + .name = "SD5:a:error",
> + .gpio = SCU_SD_ERROR_5_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 4 */
> + .name = "SD6:g:active",
> + .gpio = SCU_SD_ACTIVE_6_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + },
> + { /* bit 5 */
> + .name = "SD6:a:error",
> + .gpio = SCU_SD_ERROR_6_GPIO,
> + .active_low = 1,
> + .default_trigger = "none",
> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
> + }
> +};
Hmm... Can you utilize device tree for that?
Or built-in device properties?
> +static struct gpio_led_platform_data pca_gpio_led_info3 = {
> + .leds = pca_gpio_leds3,
> + .num_leds = ARRAY_SIZE(pca_gpio_leds3),
> +};
> +
> +static void pca_leds_register(struct device *parent,
> + struct scu_data *data)
> +{
> + data->leds_pdev[0] =
> + platform_device_register_data(parent, "leds-gpio", 1,
> + &pca_gpio_led_info1,
> + sizeof(pca_gpio_led_info1));
> + data->leds_pdev[1] =
> + platform_device_register_data(parent, "leds-gpio", 2,
> + &pca_gpio_led_info2,
> + sizeof(pca_gpio_led_info2));
> + data->leds_pdev[2] =
> + platform_device_register_data(parent, "leds-gpio", 3,
> + &pca_gpio_led_info3,
> + sizeof(pca_gpio_led_info3));
> +}
It really sounds like MFD to me.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-13 16:38 ` [PATCH] platform/x86: Add IMS/ZII SCU driver Andy Shevchenko
@ 2017-01-13 18:22 ` Darren Hart
2017-01-13 23:11 ` Florian Fainelli
2017-01-13 23:15 ` Florian Fainelli
2017-01-24 18:45 ` Florian Fainelli
2 siblings, 1 reply; 13+ messages in thread
From: Darren Hart @ 2017-01-13 18:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Florian Fainelli, Platform Driver, cphealy, Guenter Roeck, open list
On Fri, Jan 13, 2017 at 06:38:28PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > From: Guenter Roeck <linux@roeck-us.net>
> >
> > This patch adds support for the IMS (now Zodiac Inflight Innovations)
> > SCU Generation 1/2/3 platform driver. This driver registers all the
> > on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
> > GPIO expanders, Kontrom CPLD/I2C master, and more.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> > ---
> > Darren,
> >
> > This is against your "for-next" branch thanks!
>
> I'm going to review this later, though few of comments.
>
> > +config SCU
>
> No, no. We have enough mess with Intel's SCU/PMIC here, not add more.
>
> At least add manufacturer as prefix to option and file name.
>
> Btw, Darren, would it be good idea to start creating folders to make a
> bit of order in the subsystem? For first I would move Intel's PMIC/SCU
> stuff to somewhere (not sure if it should be per manufacturer or per
> function).
If we create folder, I'd recommend by manufacturer as that is typically along
the lines that our contributors work. Someone with a Dell laptop tends to focus
on Dell. The Intel engineers will focus primarily on Intel SoCs, etc.
I'd like to avoid getting too granualar as it adds overhead with Makefiles,
file navigation, etc. Any thoughts on a minimum file count to justify a new
directory? Maybe 5?
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-13 18:22 ` Darren Hart
@ 2017-01-13 23:11 ` Florian Fainelli
0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2017-01-13 23:11 UTC (permalink / raw)
To: Darren Hart, Andy Shevchenko
Cc: Platform Driver, cphealy, Guenter Roeck, open list
On 01/13/2017 10:22 AM, Darren Hart wrote:
>> Btw, Darren, would it be good idea to start creating folders to make a
>> bit of order in the subsystem? For first I would move Intel's PMIC/SCU
>> stuff to somewhere (not sure if it should be per manufacturer or per
>> function).
>
> If we create folder, I'd recommend by manufacturer as that is typically along
> the lines that our contributors work. Someone with a Dell laptop tends to focus
> on Dell. The Intel engineers will focus primarily on Intel SoCs, etc.
>
> I'd like to avoid getting too granualar as it adds overhead with Makefiles,
> file navigation, etc. Any thoughts on a minimum file count to justify a new
> directory? Maybe 5?
Chris would be in a better position than I am to comment whether up to 5
x86-based designs from ZII could show up, how about we start with naming
this file zii_scu.c, keep it under drivers/platform/x86/ for now, and if
we have 4 more (or less, whatever the threshold is), then we create a
zii/ directory and move this one (and the new ones) there?
--
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-13 16:38 ` [PATCH] platform/x86: Add IMS/ZII SCU driver Andy Shevchenko
2017-01-13 18:22 ` Darren Hart
@ 2017-01-13 23:15 ` Florian Fainelli
2017-01-14 5:49 ` Guenter Roeck
2017-01-24 19:39 ` Andy Shevchenko
2017-01-24 18:45 ` Florian Fainelli
2 siblings, 2 replies; 13+ messages in thread
From: Florian Fainelli @ 2017-01-13 23:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Platform Driver, cphealy, Guenter Roeck, Darren Hart, open list
On 01/13/2017 08:38 AM, Andy Shevchenko wrote:
> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>>
>> This patch adds support for the IMS (now Zodiac Inflight Innovations)
>> SCU Generation 1/2/3 platform driver. This driver registers all the
>> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
>> GPIO expanders, Kontrom CPLD/I2C master, and more.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
>> ---
>> Darren,
>>
>> This is against your "for-next" branch thanks!
>
> I'm going to review this later, though few of comments.
>
>> +config SCU
>
> No, no. We have enough mess with Intel's SCU/PMIC here, not add more.
OK OK.
>
> At least add manufacturer as prefix to option and file name.
>
> Btw, Darren, would it be good idea to start creating folders to make a
> bit of order in the subsystem? For first I would move Intel's PMIC/SCU
> stuff to somewhere (not sure if it should be per manufacturer or per
> function).
>
>> +obj-$(CONFIG_SCU) += scu.o
>
> For file name as well.
>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/leds.h>
>> +#include <linux/platform_data/mdio-gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio.h>
>> +#include <linux/err.h>
>> +#include <linux/dmi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-gpio.h>
>> +#include <linux/version.h>
>> +#include <linux/platform_data/at24.h>
>> +#include <linux/platform_data/pca953x.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/proc_fs.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/rtnetlink.h>
>> +#include <linux/nvmem-consumer.h>
>
> Is it possible to keep them in order?
> Do you need all of them?
> Does it sound like MFD driver + individual drivers?
My understanding of a valid MFD candidate driver is that is partitions a
shared resource space into individual resources that can all be managed
by their respective driver. The KemPLD driver is already a MFD driver,
here, this is more of a superset of all of that and an aggregate
x86-based module that has a number of on-board peripherals.
>
>> +struct __packed eeprom_data {
>> + unsigned short length; /* 0 - 1 */
>> + unsigned char checksum; /* 2 */
>> + unsigned char have_gsm_modem; /* 3 */
>> + unsigned char have_cdma_modem; /* 4 */
>> + unsigned char have_wifi_modem; /* 5 */
>> + unsigned char have_rhdd; /* 6 */
>> + unsigned char have_dvd; /* 7 */
>> + unsigned char have_tape; /* 8 */
>> + unsigned char have_humidity_sensor; /* 9 */
>> + unsigned char have_fiber_channel; /* 10 */
>> + unsigned char lru_part_number[11]; /* 11 - 21 Box Part Number */
>> + unsigned char lru_revision[7]; /* 22 - 28 Box Revision */
>> + unsigned char lru_serial_number[7]; /* 29 - 35 Box Serial Number */
>> + unsigned char lru_date_of_manufacture[7];
>> + /* 36 - 42 Box Date of Manufacture */
>> + unsigned char board_part_number[11];
>> + /* 43 - 53 Base Board Part Number */
>> + unsigned char board_revision[7];
>> + /* 54 - 60 Base Board Revision */
>> + unsigned char board_serial_number[7];
>> + /* 61 - 67 Base Board Serial Number */
>> + unsigned char board_date_of_manufacture[7];
>> + /* 68 - 74 Base Board Date of Manufacture */
>> + unsigned char board_updated_date_of_manufacture[7];
>> + /* 75 - 81 Updated Box Date of Manufacture */
>> + unsigned char board_updated_revision[7];
>> + /* 82 - 88 Updated Box Revision */
>> + unsigned char dummy[7]; /* 89 - 95 spare/filler */
>> +};
>
> Would it be better to use fixed-width types here:
> u8
> u16
>
>> +enum scu_version { scu1, scu2, scu3, unknown };
>
> MANUFACTURER_SCU_VERSION_x
> ?
OK.
>
>> +struct scu_data {
>> + struct device *dev; /* SCU platform device */
>> + struct net_device *netdev; /* Ethernet device */
>> + struct platform_device *mdio_dev; /* MDIO device */
>> + struct platform_device *dsa_dev; /* DSA device */
>> + struct proc_dir_entry *rave_proc_dir;
>> + struct mutex write_lock;
>> + struct platform_device *leds_pdev[3];
>> + struct i2c_adapter *adapter; /* I2C adapter */
>> + struct spi_master *master; /* SPI master */
>> + struct i2c_client *client[10]; /* I2C clients */
>> + struct spi_device *spidev[1]; /* SPI devices */
>
> Comments are good candidates for kernel doc.
>
>> + const struct scu_platform_data *pdata;
>> + bool have_write_magic;
>> + struct eeprom_data eeprom;
>> + struct nvmem_device *nvmem;
>
>> + bool eeprom_accessible;
>> + bool eeprom_valid;
>
> unsigned int flag1:1;
> unsigned int flag2:1;
>
> ?
Are you concerned with storage, or what motivates the preference for
bitfields vs. bools?
>
>
>> +/* platform data */
>> +
>> +static struct gpio_led pca_gpio_leds1[] = {
>> + { /* bit 0 */
>> + .name = "scu_status:g:RD",
>> + .gpio = SCU_RD_LED_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "heartbeat",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 1 */
>> + .name = "scu_status:a:WLess",
>> + .gpio = SCU_WLES_LED_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 2 */
>> + .name = "scu_status:r:LDFail",
>> + .gpio = SCU_LD_FAIL_LED_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 3 */
>> + .name = "scu_status:a:SW",
>> + .gpio = SCU_SW_LED_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + }
>> +};
>> +
>> +static struct gpio_led_platform_data pca_gpio_led_info1 = {
>> + .leds = pca_gpio_leds1,
>> + .num_leds = ARRAY_SIZE(pca_gpio_leds1),
>> +};
>> +
>> +static struct gpio_led pca_gpio_leds2[] = {
>> + { /* bit 0 */
>> + .name = "SD1:g:active",
>> + .gpio = SCU_SD_ACTIVE_1_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 1 */
>> + .name = "SD1:a:error",
>> + .gpio = SCU_SD_ERROR_1_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 2 */
>> + .name = "SD2:g:active",
>> + .gpio = SCU_SD_ACTIVE_2_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 3 */
>> + .name = "SD2:a:error",
>> + .gpio = SCU_SD_ERROR_2_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 4 */
>> + .name = "SD3:g:active",
>> + .gpio = SCU_SD_ACTIVE_3_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 5 */
>> + .name = "SD3:a:error",
>> + .gpio = SCU_SD_ERROR_3_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + }
>> +};
>> +
>> +static struct gpio_led_platform_data pca_gpio_led_info2 = {
>> + .leds = pca_gpio_leds2,
>> + .num_leds = ARRAY_SIZE(pca_gpio_leds2),
>> +};
>> +
>> +static struct gpio_led pca_gpio_leds3[] = {
>> + { /* bit 0 */
>> + .name = "SD4:g:active",
>> + .gpio = SCU_SD_ACTIVE_4_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 1 */
>> + .name = "SD4:a:error",
>> + .gpio = SCU_SD_ERROR_4_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 2 */
>> + .name = "SD5:g:active",
>> + .gpio = SCU_SD_ACTIVE_5_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 3 */
>> + .name = "SD5:a:error",
>> + .gpio = SCU_SD_ERROR_5_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 4 */
>> + .name = "SD6:g:active",
>> + .gpio = SCU_SD_ACTIVE_6_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + },
>> + { /* bit 5 */
>> + .name = "SD6:a:error",
>> + .gpio = SCU_SD_ERROR_6_GPIO,
>> + .active_low = 1,
>> + .default_trigger = "none",
>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>> + }
>> +};
>
> Hmm... Can you utilize device tree for that?
Not really an option here
> Or built-in device properties?
Not clear what you mean by that, can you expand?
>
>> +static struct gpio_led_platform_data pca_gpio_led_info3 = {
>> + .leds = pca_gpio_leds3,
>> + .num_leds = ARRAY_SIZE(pca_gpio_leds3),
>> +};
>> +
>> +static void pca_leds_register(struct device *parent,
>> + struct scu_data *data)
>> +{
>> + data->leds_pdev[0] =
>> + platform_device_register_data(parent, "leds-gpio", 1,
>> + &pca_gpio_led_info1,
>> + sizeof(pca_gpio_led_info1));
>> + data->leds_pdev[1] =
>> + platform_device_register_data(parent, "leds-gpio", 2,
>> + &pca_gpio_led_info2,
>> + sizeof(pca_gpio_led_info2));
>> + data->leds_pdev[2] =
>> + platform_device_register_data(parent, "leds-gpio", 3,
>> + &pca_gpio_led_info3,
>> + sizeof(pca_gpio_led_info3));
>> +}
>
> It really sounds like MFD to me.
It's more of a board description of attached peripherals (all of them),
more than a multi-function device, the whole module is by nature, "multi
function" since it has a bunch of different I/Os and on-module peripherals.
Thanks for your feedback!
--
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-13 23:15 ` Florian Fainelli
@ 2017-01-14 5:49 ` Guenter Roeck
2017-01-24 19:39 ` Andy Shevchenko
1 sibling, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2017-01-14 5:49 UTC (permalink / raw)
To: Florian Fainelli, Andy Shevchenko
Cc: Platform Driver, cphealy, Darren Hart, open list
On 01/13/2017 03:15 PM, Florian Fainelli wrote:
> On 01/13/2017 08:38 AM, Andy Shevchenko wrote:
>> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> From: Guenter Roeck <linux@roeck-us.net>
>>>
>>> This patch adds support for the IMS (now Zodiac Inflight Innovations)
>>> SCU Generation 1/2/3 platform driver. This driver registers all the
>>> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
>>> GPIO expanders, Kontrom CPLD/I2C master, and more.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>>> ---
>>> Darren,
>>>
>>> This is against your "for-next" branch thanks!
>>
>> I'm going to review this later, though few of comments.
>>
>>> +config SCU
>>
>> No, no. We have enough mess with Intel's SCU/PMIC here, not add more.
>
> OK OK.
>
>>
>> At least add manufacturer as prefix to option and file name.
>>
>> Btw, Darren, would it be good idea to start creating folders to make a
>> bit of order in the subsystem? For first I would move Intel's PMIC/SCU
>> stuff to somewhere (not sure if it should be per manufacturer or per
>> function).
>>
>>> +obj-$(CONFIG_SCU) += scu.o
>>
>> For file name as well.
>>
>>> +#include <linux/kernel.h>
>>> +#include <linux/init.h>
>>> +#include <linux/types.h>
>>> +#include <linux/string.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/platform_data/mdio-gpio.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/err.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/i2c-gpio.h>
>>> +#include <linux/version.h>
>>> +#include <linux/platform_data/at24.h>
>>> +#include <linux/platform_data/pca953x.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/proc_fs.h>
>>> +#include <linux/seq_file.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/rtnetlink.h>
>>> +#include <linux/nvmem-consumer.h>
>>
>> Is it possible to keep them in order?
>> Do you need all of them?
>> Does it sound like MFD driver + individual drivers?
>
> My understanding of a valid MFD candidate driver is that is partitions a
> shared resource space into individual resources that can all be managed
> by their respective driver. The KemPLD driver is already a MFD driver,
> here, this is more of a superset of all of that and an aggregate
> x86-based module that has a number of on-board peripherals.
>
>>
>>> +struct __packed eeprom_data {
>>> + unsigned short length; /* 0 - 1 */
>>> + unsigned char checksum; /* 2 */
>>> + unsigned char have_gsm_modem; /* 3 */
>>> + unsigned char have_cdma_modem; /* 4 */
>>> + unsigned char have_wifi_modem; /* 5 */
>>> + unsigned char have_rhdd; /* 6 */
>>> + unsigned char have_dvd; /* 7 */
>>> + unsigned char have_tape; /* 8 */
>>> + unsigned char have_humidity_sensor; /* 9 */
>>> + unsigned char have_fiber_channel; /* 10 */
>>> + unsigned char lru_part_number[11]; /* 11 - 21 Box Part Number */
>>> + unsigned char lru_revision[7]; /* 22 - 28 Box Revision */
>>> + unsigned char lru_serial_number[7]; /* 29 - 35 Box Serial Number */
>>> + unsigned char lru_date_of_manufacture[7];
>>> + /* 36 - 42 Box Date of Manufacture */
>>> + unsigned char board_part_number[11];
>>> + /* 43 - 53 Base Board Part Number */
>>> + unsigned char board_revision[7];
>>> + /* 54 - 60 Base Board Revision */
>>> + unsigned char board_serial_number[7];
>>> + /* 61 - 67 Base Board Serial Number */
>>> + unsigned char board_date_of_manufacture[7];
>>> + /* 68 - 74 Base Board Date of Manufacture */
>>> + unsigned char board_updated_date_of_manufacture[7];
>>> + /* 75 - 81 Updated Box Date of Manufacture */
>>> + unsigned char board_updated_revision[7];
>>> + /* 82 - 88 Updated Box Revision */
>>> + unsigned char dummy[7]; /* 89 - 95 spare/filler */
>>> +};
>>
>> Would it be better to use fixed-width types here:
>> u8
>> u16
>>
>>> +enum scu_version { scu1, scu2, scu3, unknown };
>>
>> MANUFACTURER_SCU_VERSION_x
>> ?
>
> OK.
>
>>
>>> +struct scu_data {
>>> + struct device *dev; /* SCU platform device */
>>> + struct net_device *netdev; /* Ethernet device */
>>> + struct platform_device *mdio_dev; /* MDIO device */
>>> + struct platform_device *dsa_dev; /* DSA device */
>>> + struct proc_dir_entry *rave_proc_dir;
>>> + struct mutex write_lock;
>>> + struct platform_device *leds_pdev[3];
>>> + struct i2c_adapter *adapter; /* I2C adapter */
>>> + struct spi_master *master; /* SPI master */
>>> + struct i2c_client *client[10]; /* I2C clients */
>>> + struct spi_device *spidev[1]; /* SPI devices */
>>
>> Comments are good candidates for kernel doc.
>>
>>> + const struct scu_platform_data *pdata;
>>> + bool have_write_magic;
>>> + struct eeprom_data eeprom;
>>> + struct nvmem_device *nvmem;
>>
>>> + bool eeprom_accessible;
>>> + bool eeprom_valid;
>>
>> unsigned int flag1:1;
>> unsigned int flag2:1;
>>
>> ?
>
> Are you concerned with storage, or what motivates the preference for
> bitfields vs. bools?
>
Four bytes less storage, lots of additional code for each access. Not really sure
if I like that idea. In my scope of responsibility I ask for the opposite.
>>
>>
>>> +/* platform data */
>>> +
>>> +static struct gpio_led pca_gpio_leds1[] = {
>>> + { /* bit 0 */
>>> + .name = "scu_status:g:RD",
>>> + .gpio = SCU_RD_LED_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "heartbeat",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 1 */
>>> + .name = "scu_status:a:WLess",
>>> + .gpio = SCU_WLES_LED_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 2 */
>>> + .name = "scu_status:r:LDFail",
>>> + .gpio = SCU_LD_FAIL_LED_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 3 */
>>> + .name = "scu_status:a:SW",
>>> + .gpio = SCU_SW_LED_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + }
>>> +};
>>> +
>>> +static struct gpio_led_platform_data pca_gpio_led_info1 = {
>>> + .leds = pca_gpio_leds1,
>>> + .num_leds = ARRAY_SIZE(pca_gpio_leds1),
>>> +};
>>> +
>>> +static struct gpio_led pca_gpio_leds2[] = {
>>> + { /* bit 0 */
>>> + .name = "SD1:g:active",
>>> + .gpio = SCU_SD_ACTIVE_1_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 1 */
>>> + .name = "SD1:a:error",
>>> + .gpio = SCU_SD_ERROR_1_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 2 */
>>> + .name = "SD2:g:active",
>>> + .gpio = SCU_SD_ACTIVE_2_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 3 */
>>> + .name = "SD2:a:error",
>>> + .gpio = SCU_SD_ERROR_2_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 4 */
>>> + .name = "SD3:g:active",
>>> + .gpio = SCU_SD_ACTIVE_3_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 5 */
>>> + .name = "SD3:a:error",
>>> + .gpio = SCU_SD_ERROR_3_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + }
>>> +};
>>> +
>>> +static struct gpio_led_platform_data pca_gpio_led_info2 = {
>>> + .leds = pca_gpio_leds2,
>>> + .num_leds = ARRAY_SIZE(pca_gpio_leds2),
>>> +};
>>> +
>>> +static struct gpio_led pca_gpio_leds3[] = {
>>> + { /* bit 0 */
>>> + .name = "SD4:g:active",
>>> + .gpio = SCU_SD_ACTIVE_4_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 1 */
>>> + .name = "SD4:a:error",
>>> + .gpio = SCU_SD_ERROR_4_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 2 */
>>> + .name = "SD5:g:active",
>>> + .gpio = SCU_SD_ACTIVE_5_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 3 */
>>> + .name = "SD5:a:error",
>>> + .gpio = SCU_SD_ERROR_5_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 4 */
>>> + .name = "SD6:g:active",
>>> + .gpio = SCU_SD_ACTIVE_6_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + },
>>> + { /* bit 5 */
>>> + .name = "SD6:a:error",
>>> + .gpio = SCU_SD_ERROR_6_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + }
>>> +};
>>
>> Hmm... Can you utilize device tree for that?
>
> Not really an option here
>
Yes, that would be a bit tricky for an x86 system.
>> Or built-in device properties?
>
> Not clear what you mean by that, can you expand?
>
Use device_property_ functions to read the values in the drivers,
and [platform_]device_add_properties() to add them.
See drivers/base/property.c.
Last time I looked that didn't support everything that would be needed
here, but it might work for a subset (if the client drivers support
device properties and not just devicetree properties).
Guenter
>>
>>> +static struct gpio_led_platform_data pca_gpio_led_info3 = {
>>> + .leds = pca_gpio_leds3,
>>> + .num_leds = ARRAY_SIZE(pca_gpio_leds3),
>>> +};
>>> +
>>> +static void pca_leds_register(struct device *parent,
>>> + struct scu_data *data)
>>> +{
>>> + data->leds_pdev[0] =
>>> + platform_device_register_data(parent, "leds-gpio", 1,
>>> + &pca_gpio_led_info1,
>>> + sizeof(pca_gpio_led_info1));
>>> + data->leds_pdev[1] =
>>> + platform_device_register_data(parent, "leds-gpio", 2,
>>> + &pca_gpio_led_info2,
>>> + sizeof(pca_gpio_led_info2));
>>> + data->leds_pdev[2] =
>>> + platform_device_register_data(parent, "leds-gpio", 3,
>>> + &pca_gpio_led_info3,
>>> + sizeof(pca_gpio_led_info3));
>>> +}
>>
>> It really sounds like MFD to me.
>
> It's more of a board description of attached peripherals (all of them),
> more than a multi-function device, the whole module is by nature, "multi
> function" since it has a bunch of different I/Os and on-module peripherals.
>
> Thanks for your feedback!
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-13 16:38 ` [PATCH] platform/x86: Add IMS/ZII SCU driver Andy Shevchenko
2017-01-13 18:22 ` Darren Hart
2017-01-13 23:15 ` Florian Fainelli
@ 2017-01-24 18:45 ` Florian Fainelli
2 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2017-01-24 18:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Platform Driver, cphealy, Guenter Roeck, Darren Hart, open list
On 01/13/2017 08:38 AM, Andy Shevchenko wrote:
> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>>
>> This patch adds support for the IMS (now Zodiac Inflight Innovations)
>> SCU Generation 1/2/3 platform driver. This driver registers all the
>> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
>> GPIO expanders, Kontrom CPLD/I2C master, and more.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
>> ---
>> Darren,
>>
>> This is against your "for-next" branch thanks!
>
> I'm going to review this later, though few of comments.
Waiting for this to happen so I can do a respin with an updated file
location/Kconfig symbol
--
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-13 23:15 ` Florian Fainelli
2017-01-14 5:49 ` Guenter Roeck
@ 2017-01-24 19:39 ` Andy Shevchenko
2017-01-24 20:07 ` Florian Fainelli
1 sibling, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2017-01-24 19:39 UTC (permalink / raw)
To: Florian Fainelli
Cc: Platform Driver, Chris Healy, Guenter Roeck, Darren Hart, open list
On Sat, Jan 14, 2017 at 1:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 01/13/2017 08:38 AM, Andy Shevchenko wrote:
>> On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> This patch adds support for the IMS (now Zodiac Inflight Innovations)
>>> SCU Generation 1/2/3 platform driver. This driver registers all the
>>> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
>>> GPIO expanders, Kontrom CPLD/I2C master, and more.
>> I'm going to review this later, though few of comments.
Sorry, it takes a bit longer. Below my answers.
>> Does it sound like MFD driver + individual drivers?
>
> My understanding of a valid MFD candidate driver is that is partitions a
> shared resource space into individual resources that can all be managed
> by their respective driver. The KemPLD driver is already a MFD driver,
> here, this is more of a superset of all of that and an aggregate
> x86-based module that has a number of on-board peripherals.
So, what is common here to make mandatory to keep all those in one module then?
>>> + bool eeprom_accessible;
>>> + bool eeprom_valid;
>>
>> unsigned int flag1:1;
>> unsigned int flag2:1;
>>
>> ?
>
> Are you concerned with storage, or what motivates the preference for
> bitfields vs. bools?
Just matter of style. Up to you.
>>> + { /* bit 5 */
>>> + .name = "SD6:a:error",
>>> + .gpio = SCU_SD_ERROR_6_GPIO,
>>> + .active_low = 1,
>>> + .default_trigger = "none",
>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>> + }
>>> +};
>>
>> Hmm... Can you utilize device tree for that?
>
> Not really an option here
Why not?
>> Or built-in device properties?
>
> Not clear what you mean by that, can you expand?
include/linux/properties.h (platform data in a form of DT resource provider)
>>> +static void pca_leds_register(struct device *parent,
>>> + struct scu_data *data)
>>> +{
>>> + data->leds_pdev[0] =
>>> + platform_device_register_data(parent, "leds-gpio", 1,
>>> + &pca_gpio_led_info1,
>>> + sizeof(pca_gpio_led_info1));
>>> + data->leds_pdev[1] =
>>> + platform_device_register_data(parent, "leds-gpio", 2,
>>> + &pca_gpio_led_info2,
>>> + sizeof(pca_gpio_led_info2));
>>> + data->leds_pdev[2] =
>>> + platform_device_register_data(parent, "leds-gpio", 3,
>>> + &pca_gpio_led_info3,
>>> + sizeof(pca_gpio_led_info3));
>>> +}
>>
>> It really sounds like MFD to me.
>
> It's more of a board description of attached peripherals (all of them),
> more than a multi-function device, the whole module is by nature, "multi
> function" since it has a bunch of different I/Os and on-module peripherals.
So, there is neither ACPI, nor DT provider for such resources?
We used to have a hell of a time to handle board files for Intel MID
platforms under arch/x86/platform/intel-mid (formerly known as
mrst.c), so, I don't like the idea of board files.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-24 19:39 ` Andy Shevchenko
@ 2017-01-24 20:07 ` Florian Fainelli
2017-01-24 20:18 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2017-01-24 20:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Platform Driver, Chris Healy, Guenter Roeck, Darren Hart, open list
On 01/24/2017 11:39 AM, Andy Shevchenko wrote:
>> Are you concerned with storage, or what motivates the preference for
>> bitfields vs. bools?
>
> Just matter of style. Up to you.
>
>>>> + { /* bit 5 */
>>>> + .name = "SD6:a:error",
>>>> + .gpio = SCU_SD_ERROR_6_GPIO,
>>>> + .active_low = 1,
>>>> + .default_trigger = "none",
>>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>>> + }
>>>> +};
>>>
>>> Hmm... Can you utilize device tree for that?
>>
>> Not really an option here
>
> Why not?
The platform is several years old, it is currently deployed with a boot
loader that does not support passing a Device Tree blob pointer in one
of the special e820 regions, I looked into that before, but I am not
proficient enough with grub's source code to make that happen nor do I
think this is going to be worth the trouble for these specific platforms.
Last I worked on x86 and Device Tree on CE4100 (2013 or so) there was
quite a lot of ad-hoc and not so generic code early FDT/OF
initialization code that had to be provided which AFAIR would hinder the
multiplatform capability of the kernel on x86 if there was a second DT
enabled platform that would show up. I did started that route, and it
did not look pretty, also, I would have to make *all* drivers used by
this platform (kempld and friends) to become DT aware, and make sure
that they do propagate of_node/child of_node correctly, major pain.
Finally, writing a complete Device Tree for this platform is a major
pain because a lot of the peripherals are being a PCI host bridge and
some of them are several levels deep (SPI over I2C expander over CPLD
over...).
>
>>> Or built-in device properties?
>>
>> Not clear what you mean by that, can you expand?
>
> include/linux/properties.h (platform data in a form of DT resource provider)
>
>>>> +static void pca_leds_register(struct device *parent,
>>>> + struct scu_data *data)
>>>> +{
>>>> + data->leds_pdev[0] =
>>>> + platform_device_register_data(parent, "leds-gpio", 1,
>>>> + &pca_gpio_led_info1,
>>>> + sizeof(pca_gpio_led_info1));
>>>> + data->leds_pdev[1] =
>>>> + platform_device_register_data(parent, "leds-gpio", 2,
>>>> + &pca_gpio_led_info2,
>>>> + sizeof(pca_gpio_led_info2));
>>>> + data->leds_pdev[2] =
>>>> + platform_device_register_data(parent, "leds-gpio", 3,
>>>> + &pca_gpio_led_info3,
>>>> + sizeof(pca_gpio_led_info3));
>>>> +}
>>>
>>> It really sounds like MFD to me.
>>
>> It's more of a board description of attached peripherals (all of them),
>> more than a multi-function device, the whole module is by nature, "multi
>> function" since it has a bunch of different I/Os and on-module peripherals.
>
> So, there is neither ACPI, nor DT provider for such resources?
Nope.
> We used to have a hell of a time to handle board files for Intel MID
> platforms under arch/x86/platform/intel-mid (formerly known as
> mrst.c), so, I don't like the idea of board files.
I understand the pain, but we also have to work with the tools we have,
and neither DT nor ACPI are such tools at the moment for this platform.
Thanks
--
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-24 20:07 ` Florian Fainelli
@ 2017-01-24 20:18 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2017-01-24 20:18 UTC (permalink / raw)
To: Florian Fainelli
Cc: Platform Driver, Chris Healy, Guenter Roeck, Darren Hart, open list
On Tue, Jan 24, 2017 at 10:07 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 01/24/2017 11:39 AM, Andy Shevchenko wrote:
>>>>> + { /* bit 5 */
>>>>> + .name = "SD6:a:error",
>>>>> + .gpio = SCU_SD_ERROR_6_GPIO,
>>>>> + .active_low = 1,
>>>>> + .default_trigger = "none",
>>>>> + .default_state = LEDS_GPIO_DEFSTATE_OFF,
>>>>> + }
>>>>> +};
>>>>
>>>> Hmm... Can you utilize device tree for that?
>>>
>>> Not really an option here
>>
>> Why not?
>
> The platform is several years old, it is currently deployed with a boot
> loader that does not support passing a Device Tree blob pointer in one
> of the special e820 regions, I looked into that before, but I am not
> proficient enough with grub's source code to make that happen nor do I
> think this is going to be worth the trouble for these specific platforms.
>
> Last I worked on x86 and Device Tree on CE4100 (2013 or so) there was
> quite a lot of ad-hoc and not so generic code early FDT/OF
> initialization code that had to be provided which AFAIR would hinder the
> multiplatform capability of the kernel on x86 if there was a second DT
> enabled platform that would show up. I did started that route, and it
> did not look pretty, also, I would have to make *all* drivers used by
> this platform (kempld and friends) to become DT aware, and make sure
> that they do propagate of_node/child of_node correctly, major pain.
>
> Finally, writing a complete Device Tree for this platform is a major
> pain because a lot of the peripherals are being a PCI host bridge and
> some of them are several levels deep (SPI over I2C expander over CPLD
> over...).
>
>>
>>>> Or built-in device properties?
>>>
>>> Not clear what you mean by that, can you expand?
>>
>> include/linux/properties.h (platform data in a form of DT resource provider)
Have you give a look at it?
>>>>> +static void pca_leds_register(struct device *parent,
>>>>> + struct scu_data *data)
>>>>> +{
>>>>> + data->leds_pdev[0] =
>>>>> + platform_device_register_data(parent, "leds-gpio", 1,
>>>>> + &pca_gpio_led_info1,
>>>>> + sizeof(pca_gpio_led_info1));
>>>>> + data->leds_pdev[1] =
>>>>> + platform_device_register_data(parent, "leds-gpio", 2,
>>>>> + &pca_gpio_led_info2,
>>>>> + sizeof(pca_gpio_led_info2));
>>>>> + data->leds_pdev[2] =
>>>>> + platform_device_register_data(parent, "leds-gpio", 3,
>>>>> + &pca_gpio_led_info3,
>>>>> + sizeof(pca_gpio_led_info3));
>>>>> +}
>>>>
>>>> It really sounds like MFD to me.
>>>
>>> It's more of a board description of attached peripherals (all of them),
>>> more than a multi-function device, the whole module is by nature, "multi
>>> function" since it has a bunch of different I/Os and on-module peripherals.
>>
>> So, there is neither ACPI, nor DT provider for such resources?
>
> Nope.
See above.
>> We used to have a hell of a time to handle board files for Intel MID
>> platforms under arch/x86/platform/intel-mid (formerly known as
>> mrst.c), so, I don't like the idea of board files.
>
> I understand the pain, but we also have to work with the tools we have,
> and neither DT nor ACPI are such tools at the moment for this platform.
I too understand your point, but what can we do is to do better
approach to this all.
Intel Medfield reminds me of MSIC (which is actually an IP uses SCU
and provides several devices).
Note, that on new SoCs we have just PMIC that basically looks pretty
similar (just in one IP): It's an MFD device that can contain real MFD
devices,
We have in kernel that kind of design:
arch/x86/platform/intel-mid/device_libs/platform_msic*.c (Platform
data for MSIC devices)
drivers/mfd/intel_msic.c (An MFD driver of MSIC devices)
drivers/*/* ... */* (Actual drivers)
That code is quite old. Instead we can do something like
drivers/platform/x86 (It's indeed better place for a such stuff than
drivers/mfd)
that contains MFD driver _with_ builtin device properties instead of
legacy platform data.
_and_
drivers/*/* ... */* (Actual drivers)
that contains actual resource provider agnostic drivers.
How does it sound to you?
P.S. At least if I would write MSIC support right now I would go this way.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] platform/x86: Add IMS/ZII SCU driver
2017-01-11 21:26 [PATCH] platform/x86: Add IMS/ZII SCU driver Florian Fainelli
` (2 preceding siblings ...)
2017-01-13 16:38 ` [PATCH] platform/x86: Add IMS/ZII SCU driver Andy Shevchenko
@ 2017-01-24 21:08 ` Andy Shevchenko
3 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2017-01-24 21:08 UTC (permalink / raw)
To: Florian Fainelli
Cc: Platform Driver, Chris Healy, Guenter Roeck, Darren Hart, open list
On Wed, Jan 11, 2017 at 11:26 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> This patch adds support for the IMS (now Zodiac Inflight Innovations)
> SCU Generation 1/2/3 platform driver. This driver registers all the
> on-module peripherals: Ethernet switches (Broadcom or Marvell), I2C to
> GPIO expanders, Kontrom CPLD/I2C master, and more.
Here additional comments to previously shallow review.
> +config SCU
> + tristate "SCU driver"
> + depends on DMI
> + ---help---
> + Enable SCU driver
Be more verbose here, list platforms it supports, etc.
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
Please, remove this snail address.
> +struct __packed eeprom_data {
> + unsigned short length; /* 0 - 1 */
> + unsigned char checksum; /* 2 */
> + unsigned char have_gsm_modem; /* 3 */
> + unsigned char have_cdma_modem; /* 4 */
> + unsigned char have_wifi_modem; /* 5 */
> + unsigned char have_rhdd; /* 6 */
> + unsigned char have_dvd; /* 7 */
> + unsigned char have_tape; /* 8 */
> + unsigned char have_humidity_sensor; /* 9 */
> + unsigned char have_fiber_channel; /* 10 */
> + unsigned char lru_part_number[11]; /* 11 - 21 Box Part Number */
> + unsigned char lru_revision[7]; /* 22 - 28 Box Revision */
> + unsigned char lru_serial_number[7]; /* 29 - 35 Box Serial Number */
> + unsigned char lru_date_of_manufacture[7];
> + /* 36 - 42 Box Date of Manufacture */
> + unsigned char board_part_number[11];
> + /* 43 - 53 Base Board Part Number */
> + unsigned char board_revision[7];
> + /* 54 - 60 Base Board Revision */
> + unsigned char board_serial_number[7];
> + /* 61 - 67 Base Board Serial Number */
> + unsigned char board_date_of_manufacture[7];
> + /* 68 - 74 Base Board Date of Manufacture */
> + unsigned char board_updated_date_of_manufacture[7];
> + /* 75 - 81 Updated Box Date of Manufacture */
> + unsigned char board_updated_revision[7];
> + /* 82 - 88 Updated Box Revision */
> + unsigned char dummy[7]; /* 89 - 95 spare/filler */
> +};
Perhaps gather all comments to one nice description of EEPROM structure?
> +struct scu_platform_data {
> + const char *board_type;
> + const char *lru_part_number;
> + const char *board_part_number;
> + enum scu_version version;
> + int eeprom_len;
> + struct i2c_board_info *i2c_board_info;
> + int num_i2c_board_info;
> + struct spi_board_info *spi_board_info;
> + int num_spi_board_info;
> + void (*init)(struct scu_data *);
> + void (*remove)(struct scu_data *);
> +};
> +
> +struct scu_data {
> + struct device *dev; /* SCU platform device */
> + struct net_device *netdev; /* Ethernet device */
> + struct platform_device *mdio_dev; /* MDIO device */
> + struct platform_device *dsa_dev; /* DSA device */
> + struct proc_dir_entry *rave_proc_dir;
> + struct mutex write_lock;
> + struct platform_device *leds_pdev[3];
3
> + struct i2c_client *client[10]; /* I2C clients */
10
> + struct spi_device *spidev[1]; /* SPI devices */
1
Is it by design in hardware, or it might be changed in SCU firmware?
Would be nice to have a comment for the fields.
> +/* sysfs */
Kinda useless.
> +static int scu_update_checksum(struct scu_data *data)
> +{
> + unsigned char checksum;
> + int ret;
> +
> + data->eeprom.checksum = 0;
> + checksum = scu_get_checksum((unsigned char *)&data->eeprom,
> + data->pdata->eeprom_len);
> + data->eeprom.checksum = ~checksum + 1;
> + ret = nvmem_device_write(data->nvmem,
> + 0x300 + offsetof(struct eeprom_data, checksum),
Magic.
> + sizeof(data->eeprom.checksum),
> + &data->eeprom.checksum);
> + if (ret <= 0)
> + return ret < 0 ? ret : -EIO;
> + return 0;
if (ret < 0)
return ret;
return ret ? 0 : -EIO;
?
Though I think ret == 0 case would be handled by caller.
> +static ssize_t scu_object_store(struct scu_data *data, int offset,
> + const char *in, char *out, ssize_t len)
> +{
> + char buffer[12] = { 0 };
Do you need initialization for it?
> + strlcpy(buffer, in, sizeof(buffer));
> + /* do not copy newline from input string */
> + cp = strchr(buffer, '\n');
> + if (cp)
> + *cp = '\0';
Perhaps better to move it after you get new len.
> + if (len > sizeof(buffer))
> + len = sizeof(buffer);
> +
> + mutex_lock(&data->write_lock);
> + strncpy(out, buffer, len);
Does it need to be protected?
> +
> + /* Write entire eeprom if it was marked invalid */
> + if (!data->eeprom_valid) {
> + offset = 0;
> + /* Write checksumed and non checksumed data */
> + len = sizeof(data->eeprom);
> + out = (char *)&data->eeprom;
Maybe just do a call with different parameters?
> + }
> +
> + ret = nvmem_device_write(data->nvmem, 0x300 + offset, len, out);
Magic.
> + if (ret <= 0) {
> + data->eeprom_valid = false;
> + if (ret == 0)
> + ret = -EIO;
Since you are using this idiom you might consider a wrapper for this.
It would perhaps hide magic number as well.
> + goto error;
> + }
> + if (offset < data->pdata->eeprom_len) {
> + /*
> + * Write to checksummed area of eeprom
> + * Update checksum
> + */
> + ret = scu_update_checksum(data);
> + if (ret < 0) {
> + data->eeprom_valid = false;
> + goto error;
> + }
> + }
> + data->eeprom_valid = true;
> +error:
error_unlock: ?
> + mutex_unlock(&data->write_lock);
> + return ret;
> +}
> +static umode_t scu_attr_is_visible(struct kobject *kobj, struct attribute *attr,
> + int index)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct scu_data *data = dev_get_drvdata(dev);
> + umode_t mode = attr->mode;
> +
> + /*
> + * If the eeprom has not been processed, disable its attributes.
> + * If it has been processed but is not accessible, disable
> + * write accesses to it.
> + */
> + if (index >= 1 && !data->eeprom_accessible)
> + mode &= 0444;
return mode & 0444; ?
> +
> + if (index >= 4 && data->pdata->version == scu1)
> + return 0;
> +
> + return mode;
> +}
> +/* platform data */
Useless.
> + for (i = 0; i < ARRAY_SIZE(data->leds_pdev); i++)
> + platform_device_unregister(data->leds_pdev[i]);
Sign of MFD.
> +static int scu_gpio_common_setup(unsigned int gpio_base, unsigned int ngpio,
> + u32 mask, u32 is_input, u32 is_active,
> + u32 active_low)
> +{
> + int i;
> + unsigned long flags;
> +
> + for (i = 0; i < ngpio; i++) {
> + if (!(mask & (1 << i)))
> + continue;
for_each_set_bit()
> + flags = GPIOF_EXPORT_DIR_FIXED;
> + if (is_input & (1 << i)) {
> + flags |= GPIOF_DIR_IN;
> + } else {
> + flags |= GPIOF_DIR_OUT;
> + if (is_active & (1 << i))
> + flags |= GPIOF_INIT_HIGH;
> + }
> + if (active_low & (1 << i))
> + flags |= GPIOF_ACTIVE_LOW;
> + gpio_request_one(gpio_base + i, flags, NULL);
> + }
Use BIT() macro
> + return 0;
> +}
I might assume that GPIO library has some helper for that.
> +static void scu_gpio_common_teardown(unsigned int gpio_base, int ngpio,
> + u32 mask)
> +{
> + int i;
> +
> + for (i = 0; i < ngpio; i++) {
> + if (mask & (1 << i)) {
for_each_set_bit()
> + gpio_unexport(gpio_base + i);
> + gpio_free(gpio_base + i);
> + }
> + }
> +}
And something telling me that it would much easier to do this.
Existing helpers / drivers?
> +static int scu_instantiate_i2c(struct scu_data *data, int base,
> + struct i2c_board_info *info, int count)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + data->client[base + i] = i2c_new_device(data->adapter, info);
> + if (!data->client[base + i]) {
> + /*
> + * Unfortunately this call does not tell us
> + * why it failed. Pick the most likely reason.
> + */
> + return -EBUSY;
> + }
Shouldn't i2c core do this for you whenever adapter is probed?
> + info++;
> + }
> + return 0;
> +}
> +static int scu_instantiate_spi(struct scu_data *data,
> + struct spi_board_info *info, int count)
> +{
> + struct spi_master *master;
> + int i;
> +
> + /* SPI bus number matches i2c bus number (set by sc18is602 driver) */
> + master = spi_busnum_to_master(data->adapter->nr);
> + if (!master) {
> + dev_err(data->dev, "Failed to find SPI adapter\n");
> + return -ENODEV;
> + }
> + data->master = master;
> +
> + for (i = 0; i < count; i++) {
> + info->bus_num = master->bus_num;
> + /* ignore errors */
> + data->spidev[i] = spi_new_device(master, info);
> + info++;
> + }
Ditto (spi core).
> + return 0;
> +}
> +/*
> + * This is the callback function when a a specifc at24 eeprom is found.
> + * Its reads out the eeprom contents via the read function passed back in via
> + * struct memory_accessor. It then calls part_number_proc, serial_number_proc,
> + * and dom_proc to populate the procfs entries for each specific field.
> + */
Hmm... Sounds like a stuff on top of EEPROM chip(s).
Can it be separate module?
> +static void populate_unit_info(struct nvmem_device *nvmem,
> + void *context)
> +{
> + const struct scu_platform_data *pdata = &scu_platform_data[unknown];
> + struct scu_data *data = context;
> + unsigned char *ptr;
> + int i, len;
> +
> + data->nvmem = nvmem;
> +
> + ptr = (unsigned char *)&data->eeprom;
> + /* Read Data structure from EEPROM */
> + if (nvmem_device_read(nvmem, 0x300, sizeof(data->eeprom), ptr) <= 0) {
> + dev_err(data->dev, "Failed to read eeprom data\n");
> + goto error;
> + }
> +
> + /* EEPROM is accessible, permit write access to it */
> + data->eeprom_accessible = true;
> +
> + /* Special case - eeprom not programmed */
> + if (data->eeprom.length == 0xffff && data->eeprom.checksum == 0xff) {
> + /* Assume it is SCU3, but report different board type */
> + memset(&data->eeprom, '\0', sizeof(data->eeprom));
> + data->eeprom.length = cpu_to_le16(SCU_EEPROM_LEN_EEPROM);
> + goto unprogrammed;
> + }
> +
> + /* Sanity check */
> + if (le16_to_cpu(data->eeprom.length) != SCU_EEPROM_LEN_EEPROM) {
> + dev_err(data->dev,
> + "Bad eeprom data length: Expected %d, got %d\n",
> + SCU_EEPROM_LEN_EEPROM,
> + le16_to_cpu(data->eeprom.length));
> + goto error;
> + }
> +
> + /* Update platform data based on part number retrieved from EEPROM */
> + for (i = 0; i < ARRAY_SIZE(scu_platform_data); i++) {
> + const struct scu_platform_data *tpdata = &scu_platform_data[i];
> +
> + if (tpdata->lru_part_number == NULL)
> + continue;
> + if (!strncmp(data->eeprom.lru_part_number,
> + tpdata->lru_part_number,
> + strlen(tpdata->lru_part_number))) {
> + pdata = tpdata;
> + break;
> + }
> + }
> +
> +unprogrammed:
> + data->pdata = pdata;
> + /*
> + * We know as much as we will ever find out about the platform.
> + * Perform final platform initialization and instantiate additional
> + * I2C devices as well as SPI devices now, prior to validating
> + * the EEPROM checksum.
> + */
> + if (pdata->init)
> + pdata->init(data);
> +
> + if (pdata->i2c_board_info)
> + scu_instantiate_i2c(data, ARRAY_SIZE(scu_i2c_info_common),
> + pdata->i2c_board_info,
> + pdata->num_i2c_board_info);
> +
> + if (pdata->spi_board_info)
> + scu_instantiate_spi(data, pdata->spi_board_info,
> + pdata->num_spi_board_info);
> +
> + len = data->pdata->eeprom_len;
> +
> + /* Validate checksum */
> + if (scu_get_checksum(ptr, len)) {
> + dev_err(data->dev,
> + "EEPROM data checksum error: expected 0, got 0x%x [len=%d]\n",
> + scu_get_checksum(ptr, len), len);
> + /* TBD: Do we want to clear the eeprom in this case ? */
> + goto error_noclean;
> + }
> +
> + /* Create sysfs attributes based on retrieved platform data */
> + data->eeprom_valid = true;
> + goto done;
> +
> +error:
> + memset(&data->eeprom, '\0', sizeof(data->eeprom));
> + data->eeprom.length = cpu_to_le16(SCU_EEPROM_LEN_EEPROM);
> +error_noclean:
> + data->eeprom_valid = false;
> +done:
> + if (sysfs_create_group(&data->dev->kobj, &scu_eeprom_group))
> + ;
> + if (sysfs_create_bin_file(&data->dev->kobj,
> + &scu_eeprom_test_scratchpad_file))
> + ;
> +}
> +static int scu_probe(struct platform_device *pdev)
> +{
> + struct proc_dir_entry *rave_board_type;
> + struct device *dev = &pdev->dev;
> + struct i2c_adapter *adapter;
> + struct net_device *ndev;
> + struct scu_data *data;
> + int i, ret;
> +
> + scu_request_modules(true);
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, data);
> +
> + data->dev = dev;
> +
> + mutex_init(&data->write_lock);
> +
> + /* look for ethernet device attached to 'e1000e' driver */
> + rtnl_lock();
> + for_each_netdev(&init_net, ndev) {
> + if (ndev->dev.parent && ndev->dev.parent->driver &&
> + !strcmp(ndev->dev.parent->driver->name, "e1000e")) {
> + data->netdev = ndev;
> + break;
> + }
> + }
> + rtnl_unlock();
> +
> + if (!data->netdev)
> + return -EPROBE_DEFER;
Can it be that it never appears?
> +
> + /*
> + * The adapter driver should have been loaded by now.
> + * If not, try again later.
> + */
> + adapter = scu_find_i2c_adapter("i2c-kempld");
> + if (!adapter) {
> + ret = -EPROBE_DEFER;
> + goto error_put_net;
Ditto.
> + .owner = THIS_MODULE,
Do we need this?
> +static struct platform_device *scu_pdev;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-01-24 21:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-11 21:26 [PATCH] platform/x86: Add IMS/ZII SCU driver Florian Fainelli
2017-01-13 10:15 ` kbuild test robot
2017-01-13 10:15 ` [PATCH] platform/x86: fix platform_no_drv_owner.cocci warnings kbuild test robot
2017-01-13 16:38 ` [PATCH] platform/x86: Add IMS/ZII SCU driver Andy Shevchenko
2017-01-13 18:22 ` Darren Hart
2017-01-13 23:11 ` Florian Fainelli
2017-01-13 23:15 ` Florian Fainelli
2017-01-14 5:49 ` Guenter Roeck
2017-01-24 19:39 ` Andy Shevchenko
2017-01-24 20:07 ` Florian Fainelli
2017-01-24 20:18 ` Andy Shevchenko
2017-01-24 18:45 ` Florian Fainelli
2017-01-24 21:08 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).