platform/x86: Add IMS/ZII SCU driver
PATCH 1/1 platform/x86: Add IMS/ZII SCU driver
From: Guenter Roeck <[email protected]>
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.
---
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
drivers/platform/x86/Kconfig +6 -0
@@ -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
drivers/platform/x86/Makefile +2 -0
@@ -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
drivers/platform/x86/scu.c new file +1626 -0
1,626 lines not on this page · raw message
Signature · 1 line
2.9.3
kbuild test robot
build bot[PATCH] platform/x86: fix platform_no_drv_owner.cocci warnings
drivers/platform/x86/scu.c:1541:3-8: No need to set .owner here. The core will do it.
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
---
scu.c | 1 - 1 file changed, 1 deletion(-)
drivers/platform/x86/scu.c +0 -1
@@ -1538,7 +1538,6 @@ static struct platform_driver scu_driver .remove = __exit_p(scu_remove), .driver = { .name = "scu", - .owner = THIS_MODULE, }, };
kbuild test robot
build botcoccinelle warnings: (new ones prefixed by >>)
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
Quoted 7 lines · Florian Fainelli
From: Guenter Roeck <[email protected]>
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.
Quoted 7 lines
Signed-off-by Guenter Roeck <[email protected]>Signed-off-by Florian Fainelli <[email protected]>---
Darren,This is against your "for-next" branch thanks!
I'm going to review this later, though few of comments.
Quoted 1 line
+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).
Quoted 1 line
+obj-$(CONFIG_SCU) += scu.o
For file name as well.
Quoted 14 lines
drivers/platform/x86/scu.c new file +14
-0+#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>
drivers/platform/x86/scu.c line 45
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+#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?
Quoted 20 lines
drivers/platform/x86/scu.c new file +20
-0+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 */
drivers/platform/x86/scu.c line 103
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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
Quoted 1 line
+enum
scu_version{ scu1, scu2, scu3, unknown };
MANUFACTURER_SCU_VERSION_x
?
drivers/platform/x86/scu.c line 134
Quoted 12 lines
drivers/platform/x86/scu.c new file +12
-0+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.
drivers/platform/x86/scu.c line 140
Quoted 7 lines
drivers/platform/x86/scu.c new file +6
-0+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;
?
Quoted 122 lines
drivers/platform/x86/scu.c new file +122
-0+/* 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" ,
drivers/platform/x86/scu.c line 769
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+ .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?
Quoted 11 lines
drivers/platform/x86/scu.c new file +11
-0+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,
drivers/platform/x86/scu.c line 791
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+ 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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 20 lines · Andy Shevchenko
Quoted 7 lines · Florian Fainelli
From: Guenter Roeck <[email protected]>
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.
Quoted 7 lines
Signed-off-by Guenter Roeck <[email protected]>Signed-off-by Florian Fainelli <[email protected]>---
Darren,This is against your "for-next" branch thanks!
I'm going to review this later, though few of comments.
Quoted 1 line
+config SCU
Quoted 8 lines
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?
Signature · 2 lines
Darren Hart Intel Open Source Technology Center
Quoted 5 lines · Darren Hart
Quoted 4 lines
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).
Quoted 7 lines
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?
Signature · 1 line
Florian
Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Florian Fainelli
From: Guenter Roeck <[email protected]>
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 <[email protected]>Signed-off-by Florian Fainelli <[email protected]>---
Darren,
Quoted 7 lines
Quoted 1 line
This is against your "for-next" branch thanks!
I'm going to review this later, though few of comments.
Quoted 1 line
+config SCU
No, no. We have enough mess with Intel's SCU/PMIC here, not add more.
OK OK.
Quoted 32 lines
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).
Quoted 1 line
+obj-$(CONFIG_SCU) += scu.o
For file name as well.
Quoted 20 lines
drivers/platform/x86/scu.c new file +20
-0+#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>
Quoted 8 lines
drivers/platform/x86/scu.c line 45
Quoted 4 lines
drivers/platform/x86/scu.c new file +4
-0+#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.
Quoted 36 lines
Quoted 21 lines
drivers/platform/x86/scu.c new file +20
-0+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 */ drivers/platform/x86/scu.c line 103
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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
Quoted 4 lines
Quoted 1 line
+enum
scu_version{ scu1, scu2, scu3, unknown };
MANUFACTURER_SCU_VERSION_x
?
OK.
Quoted 21 lines
drivers/platform/x86/scu.c line 134
Quoted 13 lines
drivers/platform/x86/scu.c new file +12
-0+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.
Quoted 4 lines
drivers/platform/x86/scu.c new file +4
-0+const struct scu_platform_data *pdata; +bool have_write_magic; +struct eeprom_data eeprom; +struct nvmem_device *nvmem;
Quoted 7 lines
drivers/platform/x86/scu.c line 140
Quoted 2 lines
drivers/platform/x86/scu.c new file +2
-0+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?
Quoted 128 lines
Quoted 128 lines
drivers/platform/x86/scu.c new file +126
-0+/* 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" ,
Quoted 8 lines
drivers/platform/x86/scu.c line 769
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+ .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
Quoted 1 line
Or built-in device properties?
Not clear what you mean by that, can you expand?
Quoted 16 lines
Quoted 16 lines
drivers/platform/x86/scu.c new file +15
-0+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,
Quoted 8 lines
drivers/platform/x86/scu.c line 791
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+ 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!
Signature · 1 line
Florian
Quoted 139 lines · Florian Fainelli
Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Florian Fainelli
From: Guenter Roeck <[email protected]>
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 <[email protected]>Signed-off-by Florian Fainelli <[email protected]>---
Darren,Quoted 7 lines
Quoted 1 line
This is against your "for-next" branch thanks!
I'm going to review this later, though few of comments.
Quoted 1 line
+config SCU
No, no. We have enough mess with Intel's SCU/PMIC here, not add more.
OK OK.
Quoted 32 lines
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).
Quoted 1 line
+obj-$(CONFIG_SCU) += scu.o
For file name as well.
Quoted 20 lines
drivers/platform/x86/scu.c new file +20
-0+#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> Quoted 8 lines
drivers/platform/x86/scu.c line 45
Quoted 4 lines
drivers/platform/x86/scu.c new file +4
-0+#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.
Quoted 36 lines
Quoted 21 lines
drivers/platform/x86/scu.c new file +20
-0+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 */ drivers/platform/x86/scu.c line 103
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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
u16Quoted 4 lines
Quoted 1 line
+enum
scu_version{ scu1, scu2, scu3, unknown };
MANUFACTURER_SCU_VERSION_x
?OK.
Quoted 24 lines
drivers/platform/x86/scu.c line 134
Quoted 13 lines
drivers/platform/x86/scu.c new file +12
-0+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.
Quoted 7 lines
drivers/platform/x86/scu.c new file +6
-0+const struct scu_platform_data *pdata; +bool have_write_magic; +struct eeprom_data eeprom; +struct nvmem_device *nvmem; +bool eeprom_accessible; +bool eeprom_valid;
Quoted 8 lines
Quoted 4 lines
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.
Quoted 137 lines
Quoted 128 lines
Quoted 128 lines
drivers/platform/x86/scu.c new file +126
-0+/* 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" ,Quoted 8 lines
drivers/platform/x86/scu.c line 769
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+ .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?
Quoted 2 lines
Not really an option here
Yes, that would be a bit tricky for an x86 system.
Quoted 4 lines
Quoted 1 line
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
Quoted 31 lines
Quoted 16 lines
Quoted 16 lines
drivers/platform/x86/scu.c new file +15
-0+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,Quoted 8 lines
drivers/platform/x86/scu.c line 791
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+ 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!
Quoted 8 lines · Florian Fainelli
Quoted 7 lines · Andy Shevchenko
Quoted 4 lines · Florian Fainelli
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.
Quoted 7 lines
Quoted 1 line
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?
Quoted 10 lines
Quoted 7 lines
drivers/platform/x86/scu.c line 140
Quoted 2 lines
drivers/platform/x86/scu.c new file +2
-0+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.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/platform/x86/scu.c new file +8
-0+ {/* bit 5 */ + .name = "SD6:a:error", + .gpio = SCU_SD_ERROR_6_GPIO, + .active_low = 1, + .default_trigger = "none", + .default_state = LEDS_GPIO_DEFSTATE_OFF, + } +};
Quoted 3 lines
Quoted 1 line
Hmm... Can you utilize device tree for that?
Not really an option here
Why not?
Quoted 3 lines
Quoted 1 line
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)
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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,Quoted 8 lines
drivers/platform/x86/scu.c line 791
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+ 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.
Quoted 3 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 14 lines · Andy Shevchenko
Quoted 2 lines
Are you concerned with storage, or what motivates the preference for bitfields vs. bools?
Just matter of style. Up to you.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/platform/x86/scu.c new file +8
-0+ {/* bit 5 */ + .name = "SD6:a:error", + .gpio = SCU_SD_ERROR_6_GPIO, + .active_low = 1, + .default_trigger = "none", + .default_state = LEDS_GPIO_DEFSTATE_OFF, + } +};
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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...).
Quoted 26 lines
Quoted 4 lines
Quoted 2 lines
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)Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+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,drivers/platform/x86/scu.c line 791
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+ 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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 3 lines
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 asmrst.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
Signature · 1 line
Florian
Quoted 32 lines · Florian Fainelli
Quoted 9 lines · Andy Shevchenko
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/platform/x86/scu.c new file +8
-0+ {/* bit 5 */ + .name = "SD6:a:error", + .gpio = SCU_SD_ERROR_6_GPIO, + .active_low = 1, + .default_trigger = "none", + .default_state = LEDS_GPIO_DEFSTATE_OFF, + } +};Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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_nodecorrectly, major pain.
Quoted 11 lines
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...).
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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?
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+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,drivers/platform/x86/scu.c line 791
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+ 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.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 3 lines
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 asmrst.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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Florian Fainelli
From: Guenter Roeck <[email protected]>
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 <[email protected]>Signed-off-by Florian Fainelli <[email protected]>
Quoted 6 lines
Quoted 4 lines
---
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
Quoted 6 lines · Florian Fainelli
From: Guenter Roeck <[email protected]>
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.
drivers/platform/x86/Kconfig config SURFACE3_WMI · line 382
Quoted 6 lines
drivers/platform/x86/Kconfig +5
-0+config SCU + tristate "SCU driver" + depends on DMI + ---help--- + Enable SCU driver
Be more verbose here, list platforms it supports, etc.
Quoted 3 lines
+ * 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.
Quoted 20 lines
drivers/platform/x86/scu.c new file +20
-0+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 */
drivers/platform/x86/scu.c line 103
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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?
Quoted 13 lines
drivers/platform/x86/scu.c new file +13
-0+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 *); +};
drivers/platform/x86/scu.c line 130
Quoted 10 lines
drivers/platform/x86/scu.c new file +9
-0+ +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
Quoted 1 line
+ struct
i2c_client*client[10]; /* I2C clients */
10
Quoted 1 line
+ 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.
Quoted 1 line
+/* sysfs */
Kinda useless.
drivers/platform/x86/scu.c line 178
Quoted 12 lines
drivers/platform/x86/scu.c new file +11
-0+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.
drivers/platform/x86/scu.c line 183
Quoted 7 lines
drivers/platform/x86/scu.c new file +5
-0+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.
drivers/platform/x86/scu.c line 227
Quoted 4 lines
drivers/platform/x86/scu.c new file +4
-0+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?
drivers/platform/x86/scu.c line 238
Quoted 5 lines
drivers/platform/x86/scu.c new file +5
-0+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.
drivers/platform/x86/scu.c line 244
Quoted 6 lines
drivers/platform/x86/scu.c new file +5
-0+if (len >sizeof (buffer)) + len =sizeof (buffer); + +mutex_lock (&data->write_lock); +strncpy (out, buffer, len);
Does it need to be protected?
drivers/platform/x86/scu.c line 251
Quoted 8 lines
drivers/platform/x86/scu.c new file +7
-0+ +/* 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?
drivers/platform/x86/scu.c line 254
Quoted 4 lines
drivers/platform/x86/scu.c new file +3
-0+ } + + ret =nvmem_device_write (data->nvmem,0x300 + offset, len, out);
Magic.
drivers/platform/x86/scu.c line 258
Quoted 4 lines
drivers/platform/x86/scu.c new file +4
-0+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.
Quoted 6 lines
drivers/platform/x86/scu.c new file +6
-0+ goto error; + } +if (offset < data->pdata->eeprom_len) { +/* +* Write to checksummed area of eeprom +* Update checksum
drivers/platform/x86/scu.c line 273
Quoted 10 lines
drivers/platform/x86/scu.c new file +9
-0+*/ + ret =scu_update_checksum (data); +if (ret <0 ) { + data->eeprom_valid = false; + goto error; + } + } + data->eeprom_valid = true; +error:
error_unlock: ?
Quoted 9 lines
drivers/platform/x86/scu.c new file +8
-0+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);
drivers/platform/x86/scu.c line 569
Quoted 10 lines
drivers/platform/x86/scu.c new file +9
-0+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; ?
drivers/platform/x86/scu.c line 638
Quoted 8 lines
drivers/platform/x86/scu.c new file +7
-0+ +if (index >=4 && data->pdata->version == scu1) +return 0 ; + +return mode; +} +/* platform data */
Useless.
drivers/platform/x86/scu.c line 798
Quoted 2 lines
drivers/platform/x86/scu.c new file +2
-0+for (i =0 ; i <ARRAY_SIZE (data->leds_pdev); i++) +platform_device_unregister (data->leds_pdev[i]);
Sign of MFD.
drivers/platform/x86/scu.c line 865
Quoted 11 lines
drivers/platform/x86/scu.c new file +10
-0+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()
drivers/platform/x86/scu.c line 877
Quoted 12 lines
drivers/platform/x86/scu.c new file +12
-0+ 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
Review
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
I might assume that GPIO library has some helper for that.
drivers/platform/x86/scu.c line 927
Quoted 7 lines
drivers/platform/x86/scu.c new file +7
-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)) {
for_each_set_bit()
drivers/platform/x86/scu.c line 932
Quoted 5 lines
drivers/platform/x86/scu.c new file +5
-0+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?
Quoted 5 lines
drivers/platform/x86/scu.c new file +5
-0+static int scu_instantiate_i2c (struct scu_data *data,int base, +struct i2c_board_info *info,int count) +{ +int i; +
drivers/platform/x86/scu.c line 1176
Quoted 10 lines
drivers/platform/x86/scu.c new file +9
-0+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?
Quoted 15 lines
drivers/platform/x86/scu.c new file +14
-0+ 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" );
drivers/platform/x86/scu.c line 1201
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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).
drivers/platform/x86/scu.c line 1232
Quoted 9 lines
drivers/platform/x86/scu.c new file +8
-0+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?
Quoted 124 lines
drivers/platform/x86/scu.c new file +122
-0+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) {
drivers/platform/x86/scu.c line 1452
Quoted 10 lines
drivers/platform/x86/scu.c new file +10
-0+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?
drivers/platform/x86/scu.c line 1461
Quoted 9 lines
drivers/platform/x86/scu.c new file +9
-0+ +/* +* 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.
Quoted 1 line
+ .owner =
THIS_MODULE,
Do we need this?
Quoted 1 line
+static struct
platform_device*scu_pdev;
Signature · 2 lines
With Best Regards, Andy Shevchenko