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@xxxxxxxxx> wrote:From: Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>--- Darren, This is against your "for-next" branch thanks!I'm going to review this later, though few of comments.+config SCUNo, 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.oFor 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!
