Re: [PATCH] platform/x86: Add IMS/ZII SCU driver

From: Guenter Roeck
Date: Sat Jan 14 2017 - 00:49:32 EST

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 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!