UP Squared board drivers
Subsystems (5)
cover UP Squared board drivers
[linux-kernel correctly Cc'd on the series now. Sorry for the noise.]
Hi all,
This series adds platform support for AAEON's UP Squared, a x86 SBC based on Apollo Lake [1]. Specifically, it enables control for the board pin headers and for the LEDs, as both of these features are only available behind a FPGA-based platform controller.
This is structured around a MFD driver that provides a regmap interface to that platform controller to each MFD cell.
The series is carved out from an out-of-tree support patchset, initially written by Dan O'Donovan for the original UP Board [2] and then extended through the past year to support new entries in the lineup (UP Board, UP Squared and UP Core) [3].
Here we only submit support for UP Squared to simplify review. Still, the driver is designed so that it can easily gain support for the other boards.
Questions:
* Is MFD the right fit for the platform controller?
The intention here is to encapsulate the custom GPIO-bitbanged control protocol by sharing the regmap between the drivers.
* Is our use of two chained pinctrls (in patch 3/3) valid?
Thank you!
[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/[email protected]
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
Javier Arteaga (3):
mfd: upboard: Add UP2 platform controller driver
leds: upboard: Add LED support
pinctrl: upboard: Add pinctrl and gpio driver
drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 87 +++++ drivers/mfd/Kconfig | 17 + drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 273 ++++++++++++++++ drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 523 ++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 70 ++++ 10 files changed, 996 insertions(+) create mode 100644 drivers/leds/leds-upboard.c create mode 100644 drivers/mfd/upboard.c create mode 100644 drivers/pinctrl/pinctrl-upboard.c create mode 100644 include/linux/mfd/upboard.h
Signature · 1 line
2.17.0
RFC PATCH 1/3 mfd: upboard: Add UP2 platform controller driver
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+
| 40-pin RPi-like header |
+------| (HAT) |
| +------------------------+
+-------+ +--------+
| | | | +------------------------+
| SoC |----| FPGA |-----| Custom UP2 pin header |
| | | | | (EXHAT) |
+-------+ +--------+ +------------------------+
|
+------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
--- drivers/mfd/Kconfig | 17 +++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 253 ++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 65 +++++++++ 4 files changed, 336 insertions(+) create mode 100644 drivers/mfd/upboard.c create mode 100644 include/linux/mfd/upboard.h
drivers/mfd/Kconfig +17 -0
@@ -1812,6 +1812,23 @@ config MFD_STM32_TIMERS for PWM and IIO Timer. This driver allow to share the registers between the others drivers. +config MFD_UPBOARD + tristate "UP Squared" + depends on ACPI + depends on GPIOLIB + select MFD_CORE + select REGMAP + help + If you say yes here you get support for the platform controller + of the UP Squared single-board computer. + + This driver provides common support for accessing the device, + additional drivers must be enabled in order to use the + functionality of the device. + + This driver can also be built as a module. If so, the module + will be called upboard. + menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100
drivers/mfd/Makefile +1 -0
@@ -229,3 +229,4 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o +obj-$(CONFIG_MFD_UPBOARD) += upboard.o
drivers/mfd/upboard.c new file +253 -0
253 lines not on this page · raw message
include/linux/mfd/upboard.h new file +65 -0
65 lines not on this page · raw message
Signature · 1 line
2.17.0
Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver
drivers/mfd/upboard.c line 197
Quoted 2 lines · Javier Arteaga
drivers/mfd/upboard.c new file +2
-0+static const struct acpi_device_id upboard_acpi_match[] = { + {"AANT0F01" , (kernel_ulong_t) &upboard_up2_data },
No space after cast so write it like,
{ "AANT0F01", (kernel_ulong_t)&upboard_up2_data },Quoted 6 lines
drivers/mfd/upboard.c new file +6
-0+ { } +}; +MODULE_DEVICE_TABLE (acpi, upboard_acpi_match); + +static int upboard_probe (struct platform_device *pdev) +{
drivers/mfd/upboard.c line 213
Quoted 10 lines
drivers/mfd/upboard.c new file +10
-0+struct upboard *upboard; +const struct acpi_device_id *id; +const struct upboard_data *upboard_data; +int ret; + + id =acpi_match_device (upboard_acpi_match, &pdev->dev); +if (!id) +return -ENODEV ; + + upboard_data = (const struct upboard_data *) id->driver_data;
Ditto and to other places.
Otherwise looks good to me.
Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver
Hi Mika,
Quoted 11 lines · Mika Westerberg
drivers/mfd/upboard.c line 197
Quoted 2 lines · Javier Arteaga
drivers/mfd/upboard.c new file +2
-0+static const struct acpi_device_id upboard_acpi_match[] = { + {"AANT0F01" , (kernel_ulong_t) &upboard_up2_data },No space after cast so write it like,
{ "AANT0F01", (kernel_ulong_t)&upboard_up2_data },drivers/mfd/upboard.c line 213
Quoted 1 line
drivers/mfd/upboard.c new file +1
-0+ upboard_data = (const struct upboard_data *) id->driver_data;Ditto and to other places.
Thanks! I've fixed it for the next series now.
Actually, checkpatch did not catch this one, and I can't see an explicit mention to this rule in coding-style.rst either. Is this worth sending a patch to the style guide?
Quoted 1 line
Otherwise looks good to me.
Thanks for your review!
Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver
In reply to Javier ArteagaQuoted 20 lines · Javier Arteaga
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake.
drivers/mfd/Kconfig +8
-0+config MFD_UPBOARD + tristate "UP Squared" + depends on ACPI + depends on GPIOLIB + select MFD_CORE + select REGMAP + help + If you say yes here you get support for the platformcontroller
drivers/mfd/Kconfig +3
-0+ of the UP Squared single-board computer. + + This driver provides common support for accessing thedevice,
drivers/mfd/Kconfig +4
-0+ additional drivers must be enabled in order to use the + functionality of the device. + + This driver can also be built as a module. If so, themodule
drivers/mfd/Kconfig config MFD_STM32_TIMERS · line 1830
Quoted 1 line
drivers/mfd/Kconfig +1
-0+ will be called upboard.
"upboard"
Quoted 20 lines
+static int
upboard_read(void *context, unsigned int reg, unsigned int *val)drivers/mfd/upboard.c new file +12
-0+{ +const struct upboard *const upboard = context; +int i; + +gpiod_set_value (upboard->clear_gpio,0 ); +gpiod_set_value (upboard->clear_gpio,1 ); + + reg |=UPBOARD_READ_FLAG ; + +for (i =UPBOARD_ADDRESS_SIZE ; i >=0 ; i--) { +gpiod_set_value (upboard->strobe_gpio,0 ); + gpiod_set_value(upboard->datain_gpio, (reg >> i) &
0x1);drivers/mfd/upboard.c new file +3
-0+gpiod_set_value (upboard->strobe_gpio,1 ); + } +
drivers/mfd/upboard.c line 44
Quoted 10 lines
drivers/mfd/upboard.c new file +10
-0+gpiod_set_value (upboard->strobe_gpio,0 ); + *val =0 ; + +for (i =UPBOARD_REGISTER_SIZE -1 ; i >=0 ; i--) { +gpiod_set_value (upboard->strobe_gpio,1 ); +gpiod_set_value (upboard->strobe_gpio,0 ); + *val |=gpiod_get_value (upboard->dataout_gpio) << i; + } + +gpiod_set_value (upboard->strobe_gpio,1 );
Can't you rewrite this like
for (addr) {
strobe(0)
data(x)
strobe(1)
}
for (register) {
strobe(0)
val = data(x)
strobe(1)
}
val &= BIT(register_size);
strobe(0)
?
Quoted 25 lines
drivers/mfd/upboard.c new file +5
-0+ +return 0 ; +}; + +static int upboard_write(void *context, unsigned int reg, unsignedint val)
drivers/mfd/upboard.c new file +10
-0+{ +const struct upboard *const upboard = context; +int i; + +gpiod_set_value (upboard->clear_gpio,0 ); +gpiod_set_value (upboard->clear_gpio,1 ); + +for (i =UPBOARD_ADDRESS_SIZE ; i >=0 ; i--) { +gpiod_set_value (upboard->strobe_gpio,0 ); + gpiod_set_value(upboard->datain_gpio, (reg >> i) &
0x1);drivers/mfd/upboard.c new file +7
-0+gpiod_set_value (upboard->strobe_gpio,1 ); + } + +gpiod_set_value (upboard->strobe_gpio,0 ); + +for (i =UPBOARD_REGISTER_SIZE -1 ; i >=0 ; i--) { + gpiod_set_value(upboard->datain_gpio, (val >> i) &
0x1);
drivers/mfd/upboard.c line 44
Quoted 5 lines
drivers/mfd/upboard.c new file +5
-0+gpiod_set_value (upboard->strobe_gpio,1 ); +gpiod_set_value (upboard->strobe_gpio,0 ); + } + +gpiod_set_value (upboard->strobe_gpio,1 );
Similar here:
for (addr) {
strobe(0)
data(x)
strobe(1)
}
for (register) {
strobe(0)
data(x)
strobe(1)
}
strobe(0)
strobe(1)
?
drivers/mfd/upboard.c line 47
Quoted 3 lines
drivers/mfd/upboard.c new file +3
-0+ +return 0 ; +};
Moreover these two functions have duplications, i.e.
static ... upboard_clear()
{
clear(0)
clear(1)
}
static ... upboard_set_address()
{
for (addr) {
...
}
}
Additional question is about spi-bitbang and i2c-gpio. Can one of them be utilized here? Why not?
Quoted 12 lines
drivers/mfd/upboard.c new file +10
-0+struct upboard_data { +const struct regmap_config *regmapconf; +const struct mfd_cell *cells; +size_t ncells; +}; +static int upboard_init_gpio (struct upboard *upboard) +{ +struct gpio_desc *enable_gpio; + + enable_gpio = devm_gpiod_get(upboard->dev, "enable",
GPIOD_OUT_LOW);
drivers/mfd/upboard.c line 152
Quoted 4 lines
drivers/mfd/upboard.c new file +3
-0+if (IS_ERR (enable_gpio)) +return PTR_ERR (enable_gpio); +gpiod_set_value (enable_gpio,1 );
When do you disable it? Why not?
drivers/mfd/upboard.c line 160
Quoted 9 lines
drivers/mfd/upboard.c new file +7
-0+return 0 ; +} + +static int upboard_check_supported (struct upboard *upboard) +{ +const unsigned int AAEON_MANUFACTURER_ID =0x01 ; +const unsigned int SUPPORTED_FW_MAJOR =0x0 ;
Why to hide here instead of putting at the top of file as defined constants?
drivers/mfd/upboard.c line 184
Quoted 9 lines
drivers/mfd/upboard.c new file +7
-0+unsigned int platform_id, manufacturer_id; +unsigned int firmware_id, build, major, minor, patch; +int ret; + build = (firmware_id >>12 ) &0xf ; + major = (firmware_id >>8 ) &0xf ; + minor = (firmware_id >>4 ) &0xf ; + patch = firmware_id &0xf ;
For style purposes you can use
(firmware >> 0) & 0xf here
drivers/mfd/upboard.c line 213
Quoted 12 lines
drivers/mfd/upboard.c new file +11
-0+static int upboard_probe (struct platform_device *pdev) +{ +struct upboard *upboard; +const struct acpi_device_id *id; +const struct upboard_data *upboard_data; +int ret; + id =acpi_match_device (upboard_acpi_match, &pdev->dev); +if (!id) +return -ENODEV ; + + upboard_data = (const struct upboard_data *) id->driver_data;
Use new API for that.
Quoted 1 line
+MODULE_LICENSE("GPL");
License mismatch.
include/linux/mfd/upboard.h line 15
Quoted 4 lines
include/linux/mfd/upboard.h new file +3
-0+#define UPBOARD_ADDRESS_SIZE 7 +#define UPBOARD_REGISTER_SIZE 16 +#define UPBOARD_READ_FLAG BIT(UPBOARD_ADDRESS_SIZE)
It's not clear why this one is defined in this way. Comment is needed.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver
Hi Andy,
First off, many thanks for your thorough review! Replies inline.
Quoted 13 lines · Andy Shevchenko
Quoted 13 lines
drivers/mfd/Kconfig +8
-0+config MFD_UPBOARD + tristate "UP Squared" + depends on ACPI + depends on GPIOLIB + select MFD_CORE + select REGMAP + help + If you say yes here you get support for the platformcontroller
drivers/mfd/Kconfig +3
-0+ of the UP Squared single-board computer. + + This driver provides common support for accessing thedevice,
Quoted 8 lines
drivers/mfd/Kconfig config MFD_STM32_TIMERS · line 1830
Quoted 6 lines
drivers/mfd/Kconfig +4
-0+ additional drivers must be enabled in order to use the + functionality of the device. + + This driver can also be built as a module. If so, themodule
drivers/mfd/Kconfig +1
-0+ will be called upboard."upboard"
The module name in quotes reads better to me too, but the majority of Kconfig entries do it this way, looks like:
linux $ rg 'module will be called [^"]' | wc -l
1275
linux $ rg 'module will be called "' | wc -l
5
Quoted 50 lines
Quoted 2 lines
+static int
upboard_read(void *context, unsigned int reg, unsigned int *val)Can't you rewrite this like
for (addr) {
strobe(0)
data(x)
strobe(1)
}for (register) {
strobe(0)
val = data(x)
strobe(1)
}val &=
BIT(register_size);
strobe(0)?
Quoted 2 lines
+static int
upboard_write(void *context, unsigned int reg, unsigned int val)Similar here:
for (addr) {
strobe(0)
data(x)
strobe(1)
}for (register) {
strobe(0)
data(x)
strobe(1)
}strobe(0)
strobe(1)?
Moreover these two functions have duplications, i.e.
static ...
upboard_clear()
{
clear(0)
clear(1)
}
Quoted 6 lines
static ...
upboard_set_address()
{
for (addr) {
...
}
}
I'll look into making these R/W functions more compact.
Quoted 2 lines
Additional question is about spi-bitbang and i2c-gpio. Can one of them be utilized here? Why not?
i2c-gpio would be closest, but unfortunately this isn't quite I2C:
- two in/out GPIOs instead of a single SDA line,
- R/W sequence start is signaled from yet _another_ line,
- ACK implicit in last rising edge of the address instead of an ACK pulse,
- etc...
Probably should explain this in a comment too.
Quoted 9 lines
Quoted 8 lines
drivers/mfd/upboard.c new file +5
-0+static int upboard_init_gpio (struct upboard *upboard) +{ +struct gpio_desc *enable_gpio; + + enable_gpio = devm_gpiod_get(upboard->dev, "enable",
GPIOD_OUT_LOW);drivers/mfd/upboard.c new file +2
-0+if (IS_ERR (enable_gpio)) +return PTR_ERR (enable_gpio);
Quoted 3 lines
drivers/mfd/upboard.c line 152
Quoted 1 line
drivers/mfd/upboard.c new file +1
-0+gpiod_set_value (enable_gpio,1 );When do you disable it? Why not?
enable = 0/1 sets all FPGA pins for FPGA-routed lines Hi-Z/active. It's probably safer to set enable = 0 on unload.
I'll go over this again (and add comments in any case).
Quoted 8 lines
drivers/mfd/upboard.c line 160
Quoted 5 lines
drivers/mfd/upboard.c new file +4
-0+static int upboard_check_supported (struct upboard *upboard) +{ +const unsigned int AAEON_MANUFACTURER_ID =0x01 ; +const unsigned int SUPPORTED_FW_MAJOR =0x0 ;Why to hide here instead of putting at the top of file as defined constants?
No strong reason. I'll move them to the top.
Quoted 8 lines
drivers/mfd/upboard.c line 184
Quoted 5 lines
drivers/mfd/upboard.c new file +4
-0+ build = (firmware_id >>12 ) &0xf ; + major = (firmware_id >>8 ) &0xf ; + minor = (firmware_id >>4 ) &0xf ; + patch = firmware_id &0xf ;For style purposes you can use
(firmware >> 0) &0xfhere
Sure, why not.
Quoted 7 lines
Quoted 6 lines
drivers/mfd/upboard.c new file +6
-0+static int upboard_probe (struct platform_device *pdev) +{ +struct upboard *upboard; +const struct acpi_device_id *id; +const struct upboard_data *upboard_data; +int ret;
Quoted 7 lines
drivers/mfd/upboard.c line 213
Quoted 5 lines
drivers/mfd/upboard.c new file +5
-0+ id =acpi_match_device (upboard_acpi_match, &pdev->dev); +if (!id) +return -ENODEV ; + + upboard_data = (const struct upboard_data *) id->driver_data;Use new API for that.
Will do.
Quoted 3 lines
Quoted 1 line
+MODULE_LICENSE("GPL");
License mismatch.
True, it should read "GPL v2". I'll update these.
Quoted 7 lines
include/linux/mfd/upboard.h line 15
Quoted 4 lines
include/linux/mfd/upboard.h new file +3
-0+#define UPBOARD_ADDRESS_SIZE 7 +#define UPBOARD_REGISTER_SIZE 16 +#define UPBOARD_READ_FLAG BIT(UPBOARD_ADDRESS_SIZE)It's not clear why this one is defined in this way. Comment is needed.
It means that the RW flag comes after the last bit of the address, like in I2C. I'll likely drop this #define and make this clearer next iteration.
RFC PATCH 2/3 leds: upboard: Add LED support
In reply to Javier ArteagaAllow userspace to use the on-board LEDs as "upboard:<color>:".
--- drivers/leds/Kconfig | 10 +++++ drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 87 +++++++++++++++++++++++++++++++++++++ drivers/mfd/upboard.c | 19 ++++++++ include/linux/mfd/upboard.h | 5 +++ 5 files changed, 122 insertions(+) create mode 100644 drivers/leds/leds-upboard.c
drivers/leds/Kconfig +10 -0
@@ -722,6 +722,16 @@ config LEDS_NIC78BX To compile this driver as a module, choose M here: the module will be called leds-nic78bx. +config LEDS_UPBOARD + tristate "LED support for the UP Squared" + depends on LEDS_CLASS + depends on MFD_UPBOARD + help + This option enables support for the LEDs on the UP Squared board. + + This driver can also be built as a module. If so, the module + will be called leds-upboard. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig"
drivers/leds/Makefile +1 -0
@@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o +obj-$(CONFIG_LEDS_UPBOARD) += leds-upboard.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
drivers/leds/leds-upboard.c new file +87 -0
87 lines not on this page · raw message
drivers/mfd/upboard.c +19 -0
@@ -79,6 +79,14 @@ struct upboard_data {size_t ncells; }; +#define UPBOARD_LED_CELL (led_data, n) \ + { \ + .name = "upboard-led", \ + .id = (n), \ + .platform_data = &led_data[(n)], \ + .pdata_size = sizeof(*(led_data)), \ + } +/* UP Squared */ static const struct regmap_range upboard_up2_readable_ranges[] = {
@@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = { .wr_table = &upboard_up2_writable_table, }; +static struct upboard_led_data upboard_up2_led_data[] = { + { .id =0 , .color ="blue" }, + { .id =1 , .color ="yellow" }, + { .id =2 , .color ="green" }, + { .id =3 , .color ="red" }, +}; +static const struct mfd_cell upboard_up2_mfd_cells[] = { +UPBOARD_LED_CELL (upboard_up2_led_data,0 ), +UPBOARD_LED_CELL (upboard_up2_led_data,1 ), +UPBOARD_LED_CELL (upboard_up2_led_data,2 ), +UPBOARD_LED_CELL (upboard_up2_led_data,3 ), };static const struct upboard_data upboard_up2_data = {
include/linux/mfd/upboard.h +5 -0
@@ -62,4 +62,9 @@ struct upboard { struct gpio_desc *dataout_gpio; }; +struct upboard_led_data { +unsigned int id; +const char *color; +}; +#endif /* __LINUX_MFD_UPBOARD_H */
Signature · 1 line
2.17.0
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
drivers/leds/leds-upboard.c line 30
Quoted 13 lines · Javier Arteaga
Allow userspace to use the on-board LEDs as "upboard:<color>:".
Signed-off-by Javier Arteaga <[email protected]>drivers/leds/leds-upboard.c new file +9
-0+static enum led_brightness upboard_led_brightness_get (struct led_classdev *cdev) +{ + struct upboard_led *led = container_of(cdev, struct upboard_led, cdev); +int brightness =0 ; + +regmap_field_read (led->field, &brightness); + +return brightness; +};
I'm slightly confused here. Is each led controlled by single bit?
drivers/leds/leds-upboard.c line 38
Quoted 7 lines
drivers/leds/leds-upboard.c new file +7
-0+static void upboard_led_brightness_set (struct led_classdev *cdev, +enum led_brightness brightness) +{ + struct upboard_led *led = container_of(cdev, struct upboard_led, cdev); + +regmap_field_write (led->field, brightness !=LED_OFF ); +};
What is going on with ";" at end of function? We don't do that.
If it is single bit, max_brightness should be one, and != LED_OFF test should not be needed.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Hi Pavel,
Quoted 5 lines · Pavel Machek
Quoted 3 lines · Javier Arteaga
Allow userspace to use the on-board LEDs as "upboard:<color>:".
Signed-off-by Javier Arteaga <[email protected]>
Quoted 11 lines
drivers/leds/leds-upboard.c line 30
Quoted 9 lines
drivers/leds/leds-upboard.c new file +9
-0+static enum led_brightness upboard_led_brightness_get (struct led_classdev *cdev) +{ + struct upboard_led *led = container_of(cdev, struct upboard_led, cdev); +int brightness =0 ; + +regmap_field_read (led->field, &brightness); + +return brightness; +};I'm slightly confused here. Is each led controlled by single bit?
Yes. Each regmap field is one bit wide.
Quoted 1 line
What is going on with ";" at end of function? We don't do that.
Huh, I totally missed this. It's all throughout the patchset too, and it's going away in v2 of course.
Quoted 2 lines
If it is single bit,
max_brightnessshould be one, and !=LED_OFFtest should not be needed.
Got it. Will do for v2.
Thanks for your time!
Pavel Machek
Acked-byRe: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Hi!
Quoted 4 lines
Quoted 2 lines
If it is single bit,
max_brightnessshould be one, and !=LED_OFFtest should not be needed.Got it. Will do for v2.
Good, you can add
for the v2.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
In reply to Javier ArteagaQuoted 4 lines · Javier Arteaga
Allow userspace to use the on-board LEDs as "upboard:<color>:".
(quoted diff) +1
-0+ struct upboard_led *led = container_of(cdev, struct
upboard_led, cdev);
#define to_upboard_led(cdev) container_of(cdev, struct upboard_led, cdev)
... led = to_upboard_led(cdev);
Quoted 2 lines
(quoted diff) +1
-0+ struct upboard_led *led = container_of(cdev, struct
upboard_led, cdev);
Ditto.
Quoted 1 line
+static int
__initupboard_led_probe(struct platform_device *pdev)
Are you sure about __init here?
Quoted 2 lines
(quoted diff) +1
-0+ struct upboard_led_data * const pdata = pdev-Quoted 1 line
dev.platform_data;
Don't use direct dereference to platform_data.
drivers/leds/leds-upboard.c line 54
Quoted 6 lines
drivers/leds/leds-upboard.c new file +6
-0+if (!pdev->dev.parent) +return -EINVAL ; + + upboard =dev_get_drvdata (pdev->dev.parent); +if (!upboard || !pdata) +return -EINVAL ;
Wouldn't be better to supply regmap as part of platform data? It will allow be flexible of parent device.
Quoted 1 line
+MODULE_LICENSE("GPL");
License mismatch.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Quoted 9 lines · Andy Shevchenko
Quoted 4 lines · Javier Arteaga
Allow userspace to use the on-board LEDs as "upboard:<color>:".
(quoted diff) +1
-0+ struct upboard_led *led = container_of(cdev, struct
upboard_led, cdev);#define
to_upboard_led(cdev)container_of(cdev, struct upboard_led, cdev)
Quoted 6 lines
... led =
to_upboard_led(cdev);Quoted 2 lines
(quoted diff) +1
-0+ struct upboard_led *led = container_of(cdev, struct
upboard_led, cdev);Ditto.
Will do - thanks!
Quoted 3 lines
Quoted 1 line
+static int
__initupboard_led_probe(struct platform_device *pdev)Are you sure about
__inithere?
Not 100% now :)
My understanding was that in this context, __init allows this probe() to be dropped from memory after module load.
What am I doing wrong there?
Quoted 4 lines
Quoted 2 lines
(quoted diff) +1
-0+ struct upboard_led_data * const pdata = pdev-Quoted 1 line
dev.platform_data;
Don't use direct dereference to
platform_data.
Sorry, I don't understand this one. What's the alternative?
Quoted 9 lines
drivers/leds/leds-upboard.c line 54
Quoted 6 lines
drivers/leds/leds-upboard.c new file +6
-0+if (!pdev->dev.parent) +return -EINVAL ; + + upboard =dev_get_drvdata (pdev->dev.parent); +if (!upboard || !pdata) +return -EINVAL ;Wouldn't be better to supply regmap as part of platform data? It will allow be flexible of parent device.
I'll play around with that.
Quoted 3 lines
Quoted 1 line
+MODULE_LICENSE("GPL");
License mismatch.
Will fix.
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Quoted 7 lines · Javier Arteaga
Quoted 5 lines · Andy Shevchenko
Quoted 1 line · Javier Arteaga
+static int
__initupboard_led_probe(struct platform_device *pdev)Are you sure about
__inithere?
Quoted 7 lines
Not 100% now :)
My understanding was that in this context,
__initallows thisprobe()to
be dropped from memory after module load.What am I doing wrong there?
Just give another thought about it. The keyword(s) here is(are): time to live of the objects in question. It's good to get knowing what unbind- bind means (for built-in drivers).
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
(quoted diff) +1
-0+ struct upboard_led_data * const pdata = pdev-Quoted 1 line
dev.platform_data;
Don't use direct dereference to
platform_data.Sorry, I don't understand this one. What's the alternative?
See other drivers how they do that stuff. Hint: check inline functions in include/linux/device.h.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Thanks Andy for these pointers :)
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Javier Arteaga
My understanding was that in this context,
__initallows thisprobe()to
be dropped from memory after module load.What am I doing wrong there?
Just give another thought about it. The keyword(s) here is(are): time to live of the objects in question. It's good to get knowing what unbind- bind means (for built-in drivers).
So this is the bit that I _believed_ applied to the platform drivers for these MFD-registered devices (from driver-model/platform.txt):
Or, in common situations where the device is known not to be hot-pluggable, the probe() routine can live in an init section to reduce the driver's runtime memory footprint:
int platform_driver_probe(struct platform_driver *drv,
int (*probe)(struct platform_device *))
I'm thinking my misunderstanding probably stems from assuming that these leds/pinctrl drivers will always find all devices registered at init time. Can't say I've validated that assumption - I just didn't see anything obviously blowing up in my tests so far :)
I'll keep reading and test out a few more things so I fully understand. Until then, I've taken out __init annotations from next version.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Don't use direct dereference to
platform_data.Sorry, I don't understand this one. What's the alternative?
See other drivers how they do that stuff. Hint: check inline functions in
include/linux/device.h.
I wasn't looking at the right other drivers :) I'll use the dev_get_platdata() wrapper going forwards.
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Quoted 34 lines · Javier Arteaga
Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Javier Arteaga
My understanding was that in this context,
__initallows thisprobe()
to
be dropped from memory after module load.Quoted 7 lines
Quoted 1 line
What am I doing wrong there?
Just give another thought about it. The keyword(s) here is(are):
time to
live of the objects in question. It's good to get knowing what unbind-
bind means (for built-in drivers).So this is the bit that I _believed_ applied to the platform drivers for
these MFD-registered devices (fromdriver-model/platform.txt):Or, in common situations where the device is known not to be hot-
pluggable,
theprobe()routine can live in an init section to reduce the
driver's
runtime memory footprint:int platform_driver_probe(struct
platform_driver*drv,
int (*probe)(structplatform_device*))I'm thinking my misunderstanding probably stems from assuming that these
leds/pinctrl drivers will always find all devices registered at init time. Can't say I've validated that assumption - I just didn't see anything obviously blowing up in my tests so far :)
Quoted 3 lines
I'll keep reading and test out a few more things so I fully understand.
Until then, I've taken out__initannotations from next version.
Just do one small test. Try to unbind the (built-in) driver and bind it back. Would it work? Would kernel survive this?
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
In reply to Javier ArteagaQuoted 9 lines · Javier Arteaga
Allow userspace to use the on-board LEDs as "upboard:<color>:".
Signed-off-by Javier Arteaga <[email protected]>--- drivers/leds/Kconfig | 10 +++++ drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 87 +++++++++++++++++++++++++++++++++++++drivers/mfd/upboard.c | 19 ++++++++
This change needs to be placed into a separate patch.
Quoted 3 lines
include/linux/mfd/upboard.h | 5 +++ 5 files changed, 122 insertions(+) create mode 100644 drivers/leds/leds-upboard.c
[...]
Quoted 6 lines
drivers/mfd/upboard.c +0
-0@@ -79,6 +79,14 @@ struct upboard_data {size_t ncells;
drivers/mfd/upboard.c struct upboard_data { · line 89
Quoted 10 lines
drivers/mfd/upboard.c +8
-0}; +#define UPBOARD_LED_CELL (led_data, n) \ + { \ + .name = "upboard-led", \ + .id = (n), \ + .platform_data = &led_data[(n)], \ + .pdata_size = sizeof(*(led_data)), \ + } +
There is a subsystem-level MACRO currently being reviewed on the list.
Just use the full format in your structs for now.
drivers/mfd/upboard.c static const struct regmap_config upboard_up2_regmap_config = { · line 132
Quoted 13 lines
drivers/mfd/upboard.c +6
-0/* UP Squared */ static const struct regmap_range upboard_up2_readable_ranges[] = {@@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = { .wr_table = &upboard_up2_writable_table, }; +static struct upboard_led_data upboard_up2_led_data[] = { + { .id =0 , .color ="blue" }, + { .id =1 , .color ="yellow" }, + { .id =2 , .color ="green" }, + { .id =3 , .color ="red" }, +};
How is this data used?
Does it ever change, from board to board?
Quoted 13 lines
drivers/mfd/upboard.c +4
-0static const struct mfd_cell upboard_up2_mfd_cells[] = { +UPBOARD_LED_CELL (upboard_up2_led_data,0 ), +UPBOARD_LED_CELL (upboard_up2_led_data,1 ), +UPBOARD_LED_CELL (upboard_up2_led_data,2 ), +UPBOARD_LED_CELL (upboard_up2_led_data,3 ), };static const struct upboard_data upboard_up2_data = {include/linux/mfd/upboard.h +0
-0
include/linux/mfd/upboard.h struct upboard { · line 68
Quoted 7 lines
include/linux/mfd/upboard.h +4
-0@@ -62,4 +62,9 @@ struct upboard {struct gpio_desc *dataout_gpio; }; +struct upboard_led_data { +unsigned int id; +const char *color; +};
If this is going to stick around, which I'm not sure it should, you need to document it (using kerneldoc format), since 'id' is quite ambiguous.
Quoted 1 line
include/linux/mfd/upboard.h +0
-0#endif /* __LINUX_MFD_UPBOARD_H */
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
Hi Lee,
Quoted 3 lines · Lee Jones
Quoted 1 line
drivers/mfd/upboard.c | 19 ++++++++This change needs to be placed into a separate patch.
OK. I'll do ("add driver" -> "add cell") pairs of patches then.
Quoted 11 lines
drivers/mfd/upboard.c struct upboard_data { · line 89
Quoted 8 lines
drivers/mfd/upboard.c +8
-0+#define UPBOARD_LED_CELL (led_data, n) \ + { \ + .name = "upboard-led", \ + .id = (n), \ + .platform_data = &led_data[(n)], \ + .pdata_size = sizeof(*(led_data)), \ + } +There is a subsystem-level MACRO currently being reviewed on the list.
Quoted 1 line
Just use the full format in your structs for now.
Will do.
Quoted 16 lines
drivers/mfd/upboard.c static const struct regmap_config upboard_up2_regmap_config = { · line 132
Quoted 13 lines
/* UP Squared */
static const struct
regmap_rangeupboard_up2_readable_ranges[] = {drivers/mfd/upboard.c +6
-0@@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = { .wr_table = &upboard_up2_writable_table, }; +static struct upboard_led_data upboard_up2_led_data[] = { + { .id =0 , .color ="blue" }, + { .id =1 , .color ="yellow" }, + { .id =2 , .color ="green" }, + { .id =3 , .color ="red" }, +};How is this data used?
Quoted 1 line
Does it ever change, from board to board?
This provides indexes into the LED control register, so the leds driver knows which regmap bits to flip, and maps them to color names, so it can name the led devices accordingly. The mapping does change for each board (UP1 has 3 LEDs, UP Core depends on the carrier board).
Quoted 8 lines
include/linux/mfd/upboard.h struct upboard { · line 68
Quoted 4 lines
include/linux/mfd/upboard.h +4
-0+struct upboard_led_data { +unsigned int id; +const char *color; +};If this is going to stick around, which I'm not sure it should, you need to document it (using kerneldoc format), since 'id' is quite ambiguous.
True, it's not very clear. Would it help here to also pass the regmap explicitly as platform_data (instead of leds-upboard getting to the regmap through parent driver drvdata)?
Thanks for your review!
Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support
On Thu, 26 Apr 2018, Javier Arteaga wrote:
[...]
Quoted 14 lines
Quoted 14 lines
drivers/mfd/upboard.c static const struct regmap_config upboard_up2_regmap_config = { · line 132
Quoted 11 lines
static const struct
regmap_rangeupboard_up2_readable_ranges[] = {drivers/mfd/upboard.c +6
-0@@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = { .wr_table = &upboard_up2_writable_table, }; +static struct upboard_led_data upboard_up2_led_data[] = { + { .id =0 , .color ="blue" }, + { .id =1 , .color ="yellow" }, + { .id =2 , .color ="green" }, + { .id =3 , .color ="red" }, +};How is this data used?
Quoted 6 lines
Quoted 1 line
Does it ever change, from board to board?
This provides indexes into the LED control register, so the leds driver knows which regmap bits to flip, and maps them to color names, so it can name the led devices accordingly. The mapping does change for each board (UP1 has 3 LEDs, UP Core depends on the carrier board).
I think this information should live in the driver which consumes it.
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
RFC PATCH 3/3 pinctrl: upboard: Add UP2 pinctrl and gpio driver
In reply to Javier ArteagaThe UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)
To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
---
For reference, here's the relevant ASL from the UP2 platform controller. GPO0..GPO3 are the INT3452 GPIO community controllers on Apollo Lake.
Device (PCTL)
{
Name (_HID, "AANT0F01")
Name (_DDN, "UP Squared FPGA-based Pin Controller")
[...]
Name (_CRS, ResourceTemplate ()
{
/*
* FPGA SoC GPIO Pins 0-47
* These GPIOs are ordered relative to the corresponding
* bit position in the FPGA pin direction "DIR" register
*/
GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO0", 0) {43}
GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO0", 0) {42}
GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO0", 0) {44}
[...]
GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionNone, "\\_SB.GPO1", 0) {44}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () { "external-gpios", Package() { ^PCTL, 0, 0, 0,
^PCTL, 1, 0, 0,
^PCTL, 2, 0, 0,
[...]
^PCTL, 47, 0, 0 }},
Package () { "enable-gpio", Package() { ^PCTL, 48, 0, 0 }},
Package () { "strobe-gpio", Package() { ^PCTL, 49, 0, 0 }},
Package () { "datain-gpio", Package() { ^PCTL, 50, 0, 0 }},
Package () { "clear-gpio", Package() { ^PCTL, 51, 0, 0 }},
Package () { "reset-gpio", Package() { ^PCTL, 52, 0, 0 }},
Package () { "dataout-gpio", Package() { ^PCTL, 53, 0, 0 }},
}
})drivers/mfd/upboard.c | 1 + drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 523 ++++++++++++++++++++++++++++++ 4 files changed, 538 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-upboard.c
drivers/mfd/upboard.c +1 -0
@@ -132,6 +132,7 @@ static struct upboard_led_data upboard_up2_led_data[] = { };static const struct mfd_cell upboard_up2_mfd_cells[] = { + { .name ="upboard-pinctrl" },UPBOARD_LED_CELL (upboard_up2_led_data,0 ),UPBOARD_LED_CELL (upboard_up2_led_data,1 ),UPBOARD_LED_CELL (upboard_up2_led_data,2 ),
drivers/pinctrl/Kconfig +13 -0
@@ -337,6 +337,19 @@ config PINCTRL_OCELOT select GENERIC_PINMUX_FUNCTIONS select REGMAP_MMIO +config PINCTRL_UPBOARD + tristate "UP Squared pinctrl and GPIO driver" + depends on ACPI + depends on MFD_UPBOARD + select PINMUX + help + Pinctrl driver for the pin headers on the UP Squared board. It + handles pin control for lines routed through the on-board FPGA and + propagates changes to the SoC pinctrl to keep them in sync. + + This driver can also be built as a module. If so, the module will be + called pinctrl-upboard. + source "drivers/pinctrl/aspeed/Kconfig" source "drivers/pinctrl/bcm/Kconfig" source "drivers/pinctrl/berlin/Kconfig"
drivers/pinctrl/Makefile +1 -0
@@ -42,6 +42,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o +obj-$(CONFIG_PINCTRL_UPBOARD) += pinctrl-upboard.o obj-$(CONFIG_ARCH_ASPEED) += aspeed/ obj-y += bcm/
drivers/pinctrl/pinctrl-upboard.c new file +523 -0
523 lines not on this page · raw message
Signature · 1 line
2.17.0
Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Quoted 26 lines · Javier Arteaga
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
on-board devices (ADC, FPGA IP blocks...).These lines are routed through an on-board FPGA. The platform controller
in its stock firmware provides register fields to change:- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
Quoted 2 lines
For reference, here's the relevant ASL from the UP2 platform controller.
It should be in Documentation file or in commit message.
drivers/mfd/upboard.c static struct upboard_led_data upboard_up2_led_data[] = { · line 135
Quoted 2 lines
static const struct
mfd_cellupboard_up2_mfd_cells[] = {drivers/mfd/upboard.c +1
-0+ { .name = "upboard-pinctrl" },
I guess it should be 3 lines.
drivers/mfd/upboard.c static struct upboard_led_data upboard_up2_led_data[] = { · line 138
Quoted 3 lines
drivers/mfd/upboard.c +0
-0UPBOARD_LED_CELL (upboard_up2_led_data,0 ),UPBOARD_LED_CELL (upboard_up2_led_data,1 ), UPBOARD_LED_CELL(upboard_up2_led_data, 2),
...and honestly I would not use this macro and put 4 cells explicitly here.
drivers/pinctrl/pinctrl-upboard.c line 222
Quoted 13 lines
(quoted diff) +2
-0+static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range*range,
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+ unsigned int pin) +{ + const struct pin_desc * const pd = pin_desc_get(pctldev,pin);
drivers/pinctrl/pinctrl-upboard.c new file +5
-0+const struct upboard_pin *p; +int ret; + +if (!pd) +return -EINVAL ;
When it's possible?
drivers/pinctrl/pinctrl-upboard.c line 249
Quoted 7 lines
drivers/pinctrl/pinctrl-upboard.c new file +5
-0+ p = pd->drv_data; +return 0 ; +}; +if (!pd) +return -EINVAL ;
Ditto.
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip);
Do define and use to_upboard_pinctrl().
drivers/pinctrl/pinctrl-upboard.c line 295
Quoted 3 lines
(quoted diff) +1
-0+ if (offset + 1 > pctrl->nsoc_gpios || !pctrl-Quoted 1 line
soc_gpios[offset])
drivers/pinctrl/pinctrl-upboard.c new file +1
-0+return ERR_PTR (-ENODEV );
When this is a case?
Quoted 6 lines
+static int
upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)(quoted diff) +2
-0+{ + struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc,offset);
+
Split above to definition and assignment pieces. Put assignment immediately before condition.
Quoted 15 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+if (IS_ERR (desc)) +return PTR_ERR (desc); + +return gpiod_get_direction (desc); +} +static struct regmap_field * __init upboard_field_alloc(struct device*dev,
(quoted diff) +1
-0+ struct regmap*regmap,
(quoted diff) +1
-0+ unsigned intbase,
(quoted diff) +1
-0+ unsigned int
Quoted 1 line
number)
You should really understand what __init means and when it's appropriate to use it.
drivers/pinctrl/pinctrl-upboard.c line 427
Quoted 3 lines
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+static int __init upboard_pinctrl_probe (struct platform_device *pdev) +{ +struct acpi_device *const adev =ACPI_COMPANION (&pdev->dev);
Huh, const in that place? Why?
drivers/pinctrl/pinctrl-upboard.c line 443
Quoted 6 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+if (!pdev->dev.parent) +return -EINVAL ; + + upboard =dev_get_drvdata (pdev->dev.parent); +if (!upboard) +return -EINVAL ;
Same comment as per LED driver.
drivers/pinctrl/pinctrl-upboard.c line 446
Quoted 2 lines
drivers/pinctrl/pinctrl-upboard.c new file +2
-0+if (strcmp (acpi_device_hid (adev),"AANT0F01" )) +return -ENODEV ;
Huh?
drivers/pinctrl/pinctrl-upboard.c line 480
Quoted 1 line
drivers/pinctrl/pinctrl-upboard.c new file +1
-0+ ((struct pinctrl_pin_desc *)pd)->drv_data = pin;
What is that?! I mean ugly casting.
drivers/pinctrl/pinctrl-upboard.c line 523
Quoted 5 lines
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+ } +} +MODULE_LICENSE ("GPL" );
License mismatch.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Quoted 4 lines · Andy Shevchenko
Quoted 2 lines
For reference, here's the relevant ASL from the UP2 platform controller.
It should be in Documentation file or in commit message.
Perfect, I just wasn't sure whether dumping ASL this way would be acceptable in commit history. In that case, I think I prefer the commit body over keeping a Documentation/ file elsewhere that's easy to miss.
Quoted 6 lines
drivers/mfd/upboard.c static struct upboard_led_data upboard_up2_led_data[] = { · line 135
Quoted 2 lines
static const struct
mfd_cellupboard_up2_mfd_cells[] = {drivers/mfd/upboard.c +1
-0+ { .name = "upboard-pinctrl" },I guess it should be 3 lines.
Quoted 6 lines
drivers/mfd/upboard.c static struct upboard_led_data upboard_up2_led_data[] = { · line 138
Quoted 3 lines
drivers/mfd/upboard.c +0
-0UPBOARD_LED_CELL (upboard_up2_led_data,0 ),UPBOARD_LED_CELL (upboard_up2_led_data,1 ), UPBOARD_LED_CELL(upboard_up2_led_data, 2),...and honestly I would not use this macro and put 4 cells explicitly here.
The idea was to avoid repeating .platform_data + .pdata_size over and over as there's a few LEDs per board, but explicit is good too.
May just go with your suggestion as that seems to be more popular than macros in other mfd drivers.
Quoted 11 lines
Quoted 10 lines
(quoted diff) +2
-0+static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range*range,
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+ unsigned int pin) +{ + const struct pin_desc * const pd = pin_desc_get(pctldev,pin);
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+const struct upboard_pin *p; +int ret; +
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!pd) + return -EINVAL;When it's possible?
Just reread pin_request() from pinctrl core. You're right, it isn't. I'll take out the check.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!pd) + return -EINVAL;Ditto.
As above.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip);Do define and use
to_upboard_pinctrl().
Will do.
Quoted 5 lines
drivers/pinctrl/pinctrl-upboard.c line 295
Quoted 3 lines
(quoted diff) +1
-0+ if (offset + 1 > pctrl->nsoc_gpios || !pctrl-Quoted 1 line
soc_gpios[offset])
drivers/pinctrl/pinctrl-upboard.c new file +1
-0+return ERR_PTR (-ENODEV );When this is a case?
Another unnecessary check if gpiolib already guarantees valid inputs.
Quoted 10 lines
Quoted 6 lines
+static int
upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)(quoted diff) +2
-0+{ + struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc,offset);
+Split above to definition and assignment pieces. Put assignment immediately before condition.
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (IS_ERR(desc)) + return PTR_ERR(desc);
I'll do that.
Quoted 11 lines
Quoted 8 lines
+static struct
regmap_field*__initupboard_field_alloc(struct device *dev,(quoted diff) +1
-0+ struct regmap*regmap,
(quoted diff) +1
-0+ unsigned intbase,
(quoted diff) +1
-0+ unsigned intnumber)
You should really understand what
__initmeans and when it's appropriate to use it.
As in the other email: the intention was to drop probe() and funcs only used on module init, but I appear to have misunderstood something here.
Quoted 5 lines
drivers/pinctrl/pinctrl-upboard.c line 427
Quoted 3 lines
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+static int __init upboard_pinctrl_probe (struct platform_device *pdev) +{ +struct acpi_device *const adev =ACPI_COMPANION (&pdev->dev);Huh, const in that place? Why?
You're right, it isn't a usual pattern. Dropped.
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c line 443
Quoted 6 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+if (!pdev->dev.parent) +return -EINVAL ; + + upboard =dev_get_drvdata (pdev->dev.parent); +if (!upboard) +return -EINVAL ;Same comment as per LED driver.
I'll address that too.
Quoted 4 lines
drivers/pinctrl/pinctrl-upboard.c line 446
Quoted 2 lines
drivers/pinctrl/pinctrl-upboard.c new file +2
-0+if (strcmp (acpi_device_hid (adev),"AANT0F01" )) +return -ENODEV ;Huh?
Ugh, this is left over from a bit of code that selected the right pinctrl_desc* for each UP HID. Of course, it doesn't make sense now.
I'll take that out.
Quoted 3 lines
drivers/pinctrl/pinctrl-upboard.c line 480
Quoted 1 line
drivers/pinctrl/pinctrl-upboard.c new file +1
-0+ ((struct pinctrl_pin_desc *)pd)->drv_data = pin;What is that?! I mean ugly casting.
I agree, it's an eyesore. The intention was to drop const from pinctrl_desc->pins as we're replacing the pins' drv_data on init.
It does look like I'm going at this the wrong way though. I'll take another stab at it (pointers welcome of course).
Quoted 3 lines
Quoted 1 line
+MODULE_LICENSE("GPL");
License mismatch.
Will fix.
Thanks again for your time Andy! I really appreciate your help :)
Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
In reply to Javier ArteagaQuoted 23 lines · Javier Arteaga
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
Signed-off-by Javier Arteaga <[email protected]>---
[...]
Quoted 13 lines
drivers/mfd/upboard.c | 1 + drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 523 ++++++++++++++++++++++++++++++ 4 files changed, 538 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-upboard.cdrivers/mfd/upboard.c +0
-0
drivers/mfd/upboard.c static struct upboard_led_data upboard_up2_led_data[] = { · line 138
Quoted 7 lines
drivers/mfd/upboard.c +1
-0@@ -132,6 +132,7 @@ static struct upboard_led_data upboard_up2_led_data[] = { };static const struct mfd_cell upboard_up2_mfd_cells[] = { + { .name ="upboard-pinctrl" },UPBOARD_LED_CELL (upboard_up2_led_data,0 ),UPBOARD_LED_CELL (upboard_up2_led_data,1 ),UPBOARD_LED_CELL (upboard_up2_led_data,2 ),
Please made this a separate patch.
There aren't any build dependencies between the files.
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Quoted 9 lines · Lee Jones
drivers/mfd/upboard.c static struct upboard_led_data upboard_up2_led_data[] = { · line 138
Quoted 5 lines
static const struct
mfd_cellupboard_up2_mfd_cells[] = {drivers/mfd/upboard.c +1
-0+ { .name = "upboard-pinctrl" },UPBOARD_LED_CELL (upboard_up2_led_data,0 ),UPBOARD_LED_CELL (upboard_up2_led_data,1 ), UPBOARD_LED_CELL(upboard_up2_led_data, 2),Please made this a separate patch.
There aren't any build dependencies between the files.
Will do.
I have one further question about MFD in this patch too - should I keep passing regmap into the driver via dev_get_drvdata(pdev->dev.parent), or is explicit platform_data preferable?
Andy suggested platform_data allows more flexibility on the parent device side (although I can't see upboard-pinctrl being used other than as a child of the upboard driver).
I went with parent drvdata simply because that's what I found in other MFD drivers and material [1].
Thank you!
[1]: http://events17.linuxfoundation.org/sites/events/files/slides/belloni-mfd-regmap-syscon_0.pdf
Quoted 21 lines · Javier Arteaga
[linux-kernel correctly Cc'd on the series now. Sorry for the noise.]
Hi all,
This series adds platform support for AAEON's UP Squared, a x86 SBC based on Apollo Lake [1]. Specifically, it enables control for the board pin headers and for the LEDs, as both of these features are only available behind a FPGA-based platform controller.
This is structured around a MFD driver that provides a regmap interface to that platform controller to each MFD cell.
The series is carved out from an out-of-tree support patchset, initially written by Dan O'Donovan for the original UP Board [2] and then extended through the past year to support new entries in the lineup (UP Board, UP Squared and UP Core) [3].
Here we only submit support for UP Squared to simplify review. Still, the driver is designed so that it can easily gain support for the other boards.
Quoted 7 lines
Questions:
* Is MFD the right fit for the platform controller?
The intention here is to encapsulate the custom GPIO-bitbanged control protocol by sharing the regmap between the drivers.* Is our use of two chained pinctrls (in patch 3/3) valid?
There are few minor comments but other than that, to me this looks like a reasonable approach.
v2 cover UP Squared board drivers
In reply to Javier ArteagaHi all,
This series adds platform support for AAEON's UP Squared, a x86 SBC based on Apollo Lake [1]. Specifically, it enables control for the board pin headers and for the LEDs, as both of these features are only available behind a FPGA-based platform controller.
This is structured around a MFD driver that provides a regmap interface to that platform controller to each MFD cell.
The series is carved out from an out-of-tree support patchset, initially written for the original UP Board [2] and then extended through the past year to support new entries in the lineup (UP Board, UP Squared and UP Core) [3].
Here we only submit support for UP Squared to simplify review. Still, the driver is designed so that it can easily gain support for the other boards.
V1 was originally submitted by Javier Arteaga in April 2018 as an RFC [4]. This V2 aims to address the feedback gratefully received from that.
[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/[email protected]
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
[4]: https://lore.kernel.org/lkml/[email protected]/
V2:
* Rewrote regmap r/w handlers to avoid duplication and improve readability
* Simplified data passing between MFD parent and child device drivers
* Removed incorrect __init attribute from probe functions
* Moved MFD cell definitions into same patch as MFD driver and simplified them
* Promoted some dev_dbg instances to dev_err for probe error cases
* Decoupled MFD cell reference from ACPI driver data
* Miscellanous coding style improvements
* Fixed license mismatches
Javier Arteaga (3):
mfd: upboard: Add UP2 platform controller driver
leds: upboard: Add LED support
pinctrl: upboard: Add UP2 pinctrl and gpio driver
drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 104 ++++++++ drivers/mfd/Kconfig | 17 ++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 344 +++++++++++++++++++++++++ drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 519 ++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++ 10 files changed, 1054 insertions(+) create mode 100644 drivers/leds/leds-upboard.c create mode 100644 drivers/mfd/upboard.c create mode 100644 drivers/pinctrl/pinctrl-upboard.c create mode 100644 include/linux/mfd/upboard.h
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
PATCH v2 1/3 mfd: upboard: Add UP2 platform controller driver
From: Javier Arteaga <[email protected]>
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+
| 40-pin RPi-like header |
+------| (HAT) |
| +------------------------+
+-------+ +--------+
| | | | +------------------------+
| SoC |----| FPGA |-----| Custom UP2 pin header |
| | | | | (EXHAT) |
+-------+ +--------+ +------------------------+
|
+------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
--- drivers/mfd/Kconfig | 17 +++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 344 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++++ 4 files changed, 406 insertions(+) create mode 100644 drivers/mfd/upboard.c create mode 100644 include/linux/mfd/upboard.h
drivers/mfd/Kconfig +17 -0
@@ -1855,6 +1855,23 @@ config MFD_STM32_TIMERS for PWM and IIO Timer. This driver allow to share the registers between the others drivers. +config MFD_UPBOARD + tristate "UP Squared" + depends on ACPI + depends on GPIOLIB + select MFD_CORE + select REGMAP + help + If you say yes here you get support for the platform controller + of the UP Squared single-board computer. + + This driver provides common support for accessing the device, + additional drivers must be enabled in order to use the + functionality of the device. + + This driver can also be built as a module. If so, the module + will be called "upboard". + menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100
drivers/mfd/Makefile +1 -0
@@ -241,3 +241,4 @@ obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o +obj-$(CONFIG_MFD_UPBOARD) += upboard.o
drivers/mfd/upboard.c new file +344 -0
344 lines not on this page · raw message
include/linux/mfd/upboard.h new file +44 -0
44 lines not on this page · raw message
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver
Quoted 28 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+ | 40-pin RPi-like header | +------| (HAT) | | +------------------------+ +-------+ +--------+ | | | | +------------------------+ | SoC |----| FPGA |-----| Custom UP2 pin header | | | | | | (EXHAT) | +-------+ +--------+ +------------------------+ | +------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
Quoted 7 lines
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
Can we see a link to or an excerpt of ACPI table for this device?
drivers/mfd/upboard.c line 61
Quoted 4 lines
drivers/mfd/upboard.c new file +4
-0+#define set_clear (u, x) gpiod_set_value((u)->clear_gpio, (x)) +#define set_strobe (u, x) gpiod_set_value((u)->strobe_gpio, (x)) +#define set_datain (u, x) gpiod_set_value((u)->datain_gpio, (x)) +#define get_dataout (u) gpiod_get_value((u)->dataout_gpio)
I think these macros don't bring much value. (Up to you and Lee to decide)
drivers/mfd/upboard.c line 94
Quoted 11 lines
drivers/mfd/upboard.c new file +10
-0+static void __reg_io_write (const struct upboard_ddata *const ddata, +unsigned int size,unsigned int val) +{ +int i; + +/* +* DATAIN is latched on each rising edge of the STROBE signal. +* Data (register address or value) is sent MSb first. +*/ +for (i = size -1 ; i >=0 ; i--) {
while (size--)
Quoted 8 lines
drivers/mfd/upboard.c new file +8
-0+set_strobe (ddata,0 ); +set_datain (ddata, (val >> i) &0x1 ); +set_strobe (ddata,1 ); + } +} + +static void __reg_io_read (const struct upboard_ddata *const ddata, +unsigned int size,unsigned int *val)
drivers/mfd/upboard.c line 111
Quoted 10 lines
drivers/mfd/upboard.c new file +9
-0+{ +int i; + +/* +* DATAOUT is latched on on each rising edge of the STROBE signal. +* Data (register value) is received MSb first. +*/ + *val =0 ; +for (i = size -1 ; i >=0 ; i--) {
Ditto.
drivers/mfd/upboard.c line 240
Quoted 13 lines
drivers/mfd/upboard.c new file +11
-0+set_strobe (ddata,0 ); +set_strobe (ddata,1 ); + *val |=get_dataout (ddata) << i; + } +} +static int upboard_check_supported (struct device *dev,struct regmap *regmap) +{ +uint8_t manufacturer_id, build, major, minor, patch; +unsigned int platform_id, firmware_id; +int ret; + manufacturer_id = platform_id &0xff ;
Redundant & 0xff part.
Quoted 8 lines
drivers/mfd/upboard.c new file +7
-0+if (manufacturer_id !=AAEON_MANUFACTURER_ID ) { +dev_err (dev, +"unsupported FPGA firmware from manufacturer 0x%02x" , + manufacturer_id); +return -ENODEV ; + } +static int upboard_match_device (struct device *dev)
drivers/mfd/upboard.c line 282
Quoted 10 lines
drivers/mfd/upboard.c new file +9
-0+{ +struct upboard_ddata *ddata =dev_get_drvdata (dev); +const struct acpi_device_id *id; + + id =acpi_match_device (upboard_acpi_match, dev); +if (!id) +return -ENODEV ; + +switch (id->driver_data) {
device_get_match_data() IIRC the name of the call.
Quoted 9 lines
drivers/mfd/upboard.c new file +9
-0+case UPBOARD_ID_UP2 : + ddata->regmapconf = &upboard_up2_regmap_config; + ddata->cells = upboard_up2_mfd_cells; + ddata->ncells =ARRAY_SIZE (upboard_up2_mfd_cells); +break ; +default : +dev_err (dev,"unsupported ID %lu\n" , id->driver_data); +return -EINVAL ; + }
drivers/mfd/upboard.c line 322
Quoted 10 lines
drivers/mfd/upboard.c new file +9
-0+ +return 0 ; +} + ret =upboard_init_gpio (dev); +if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err (dev,"failed to init GPIOs: %d" , ret); +return ret; + }
I don't know if probe_err() helper is going to be a part of v4.21 (which this series targets), it would be good to use it.
Quoted 1 line
+}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver
Quoted 37 lines · Andy Shevchenko
Quoted 28 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+ | 40-pin RPi-like header | +------| (HAT) | | +------------------------+ +-------+ +--------+ | | | | +------------------------+ | SoC |----| FPGA |-----| Custom UP2 pin header | | | | | | (EXHAT) | +-------+ +--------+ +------------------------+ | +------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
Quoted 7 lines
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
Quoted 8 lines
Can we see a link to or an excerpt of ACPI table for this device?
drivers/mfd/upboard.c line 61
Quoted 4 lines
drivers/mfd/upboard.c new file +4
-0+#define set_clear (u, x) gpiod_set_value((u)->clear_gpio, (x)) +#define set_strobe (u, x) gpiod_set_value((u)->strobe_gpio, (x)) +#define set_datain (u, x) gpiod_set_value((u)->datain_gpio, (x)) +#define get_dataout (u) gpiod_get_value((u)->dataout_gpio)I think these macros don't bring much value. (Up to you and Lee to decide)
I agree.
In fact I think they only serve to obfuscate instead of clarify.
[...]
Quoted 12 lines
Quoted 11 lines
drivers/mfd/upboard.c new file +10
-0+set_strobe (ddata,0 ); +set_strobe (ddata,1 ); + *val |=get_dataout (ddata) << i; + } +} +static int upboard_check_supported (struct device *dev,struct regmap *regmap) +{ +uint8_t manufacturer_id, build, major, minor, patch; +unsigned int platform_id, firmware_id; +int ret;
Quoted 3 lines
Quoted 1 line
+
manufacturer_id=platform_id&0xff;Redundant &
0xffpart.
Maybe not required, but lets the reader know it's intended.
[...]
Quoted 9 lines
drivers/mfd/upboard.c line 322
Quoted 6 lines
drivers/mfd/upboard.c new file +6
-0+ ret =upboard_init_gpio (dev); +if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err (dev,"failed to init GPIOs: %d" , ret); +return ret; + }I don't know if
probe_err()helper is going to be a part of v4.21 (which this series targets), it would be good to use it.
Interesting. What does it do?
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver
Quoted 10 lines · Lee Jones
Quoted 9 lines · Andy Shevchenko
Quoted 6 lines · Dan O'Donovan
drivers/mfd/upboard.c new file +6
-0+ ret =upboard_init_gpio (dev); +if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err (dev,"failed to init GPIOs: %d" , ret); +return ret; + }
Quoted 4 lines
Quoted 2 lines
I don't know if
probe_err()helper is going to be a part of v4.21 (which this series targets), it would be good to use it.Interesting. What does it do?
The code will tell better than I
https://lore.kernel.org/lkml/[email protected]/
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/3] mfd: upboard: Add UP2 platform controller driver
In reply to Andy ShevchenkoQuoted 28 lines · Andy Shevchenko
Quoted 27 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+ | 40-pin RPi-like header | +------| (HAT) | | +------------------------+ +-------+ +--------+ | | | | +------------------------+ | SoC |----| FPGA |-----| Custom UP2 pin header | | | | | | (EXHAT) | +-------+ +--------+ +------------------------+ | +------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
Quoted 8 lines
Quoted 7 lines
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
Can we see a link to or an excerpt of ACPI table for this device?
Sure: https://gist.github.com/Dan-Emutex/6382c25f4b8b8cdd80e6056889cdf48b
I'll add this to the cover letter for v3 as well.
Quoted 6 lines
drivers/mfd/upboard.c line 61
Quoted 5 lines
drivers/mfd/upboard.c new file +4
-0+#define set_clear (u, x) gpiod_set_value((u)->clear_gpio, (x)) +#define set_strobe (u, x) gpiod_set_value((u)->strobe_gpio, (x)) +#define set_datain (u, x) gpiod_set_value((u)->datain_gpio, (x)) +#define get_dataout (u) gpiod_get_value((u)->dataout_gpio)I think these macros don't bring much value. (Up to you and Lee to decide)
These were added in response to one of your review comments on the original patch-set ;) I think they do slightly improve the code readability so I'll leave them there if there are no objections.
Quoted 13 lines
drivers/mfd/upboard.c line 94
Quoted 12 lines
drivers/mfd/upboard.c new file +10
-0+static void __reg_io_write (const struct upboard_ddata *const ddata, +unsigned int size,unsigned int val) +{ +int i; + +/* +* DATAIN is latched on each rising edge of the STROBE signal. +* Data (register address or value) is sent MSb first. +*/ +for (i = size -1 ; i >=0 ; i--) {while (size--)
Yep, much neater. I'll change that, thanks! Ditto for the other comments on this file.+ ret = upboard_init_gpio(dev);
Quoted 7 lines
drivers/mfd/upboard.c line 322
Quoted 5 lines
drivers/mfd/upboard.c new file +5
-0+if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err (dev,"failed to init GPIOs: %d" , ret); +return ret; + }I don't know if
probe_err()helper is going to be a part of v4.21 (which this series targets), it would be good to use it.
I left this out for now in case it causes any awkwardness with merging. Hope that's ok.
PATCH v2 2/3 leds: upboard: Add LED support
In reply to Dan O'DonovanFrom: Javier Arteaga <[email protected]>
Allow userspace to use the on-board LEDs as "upboard:<color>:".
--- drivers/leds/Kconfig | 10 +++++ drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 104 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 drivers/leds/leds-upboard.c
drivers/leds/Kconfig +10 -0
@@ -756,6 +756,16 @@ config LEDS_NIC78BX To compile this driver as a module, choose M here: the module will be called leds-nic78bx. +config LEDS_UPBOARD + tristate "LED support for the UP Squared" + depends on LEDS_CLASS + depends on MFD_UPBOARD + help + This option enables support for the LEDs on the UP Squared board. + + This driver can also be built as a module. If so, the module + will be called leds-upboard. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig"
drivers/leds/Makefile +1 -0
@@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o +obj-$(CONFIG_LEDS_UPBOARD) += leds-upboard.o # LED SPI Drivers obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
drivers/leds/leds-upboard.c new file +104 -0
104 lines not on this page · raw message
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 7 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
Allow userspace to use the on-board LEDs as "upboard:<color>:".
drivers/leds/leds-upboard.c line 16
Quoted 7 lines
drivers/leds/leds-upboard.c new file +7
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h>
For better maintenance keep this ordered.
drivers/leds/leds-upboard.c line 65
Quoted 3 lines
drivers/leds/leds-upboard.c new file +3
-0+ adev =ACPI_COMPANION (dev); +if (!adev ||strcmp (acpi_device_hid (adev),"AANT0F01" )) +return -ENODEV ;
Why do you need this part?
drivers/leds/leds-upboard.c line 71
Quoted 5 lines
drivers/leds/leds-upboard.c new file +4
-0+if (led_index >=ARRAY_SIZE (upboard_led_names)) +return -EINVAL ; +if (!dev->parent) +return -EINVAL ;
...I think this check will cover cases when driver is being probed standalone. Otherwise it's caller's responsibility not to instantiate it if platform doesn't have this LED feature.
Quoted 3 lines
drivers/leds/leds-upboard.c new file +3
-0+ regmap =dev_get_regmap (dev->parent,NULL ); +if (!regmap) +return -EINVAL ;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 7 lines · Dan O'Donovan
Quoted 4 lines
From: Javier Arteaga <[email protected]>
Allow userspace to use the on-board LEDs as "upboard:<color>:".
Quoted 9 lines
drivers/leds/leds-upboard.c line 16
Quoted 7 lines
drivers/leds/leds-upboard.c new file +7
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h> For better maintenance keep this ordered.
This is good as-is.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v2 2/3] leds: upboard: Add LED support
In reply to Dan O'DonovanHi Javier,
Thank you for the patch.
I have few trivial issues below, please take a look.
Quoted 59 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
Allow userspace to use the on-board LEDs as "upboard:<color>:".
Acked-by Pavel Machek <[email protected]>Signed-off-by Javier Arteaga <[email protected]>Signed-off-by Dan O'Donovan <[email protected]>--- drivers/leds/Kconfig | 10 +++++ drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 104 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 drivers/leds/leds-upboard.cdrivers/leds/Kconfig +10
-0@@ -756,6 +756,16 @@ config LEDS_NIC78BX To compile this driver as a module, choose M here: the module will be called leds-nic78bx. +config LEDS_UPBOARD + tristate "LED support for the UP Squared" + depends on LEDS_CLASS + depends on MFD_UPBOARD + help + This option enables support for the LEDs on the UP Squared board. + + This driver can also be built as a module. If so, the module + will be called leds-upboard. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig"drivers/leds/Makefile +1
-0@@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o +obj-$(CONFIG_LEDS_UPBOARD) += leds-upboard.o # LED SPI Drivers obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.odrivers/leds/leds-upboard.c new file +6
-0@@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// UP Board LED driver +// +// Copyright (c) 2018, Emutex Ltd. +//
drivers/leds/leds-upboard.c line 16
Quoted 10 lines
drivers/leds/leds-upboard.c new file +10
-0+// Author: Javier Arteaga <[email protected]> +// + +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h>
The last include should go first to keep alphabetical order.
Quoted 11 lines
drivers/leds/leds-upboard.c new file +11
-0+ +#define to_upboard_led (cdev) container_of(cdev, struct upboard_led, cdev) + +static const char *const upboard_led_names[] = { +"upboard:blue:" , +"upboard:yellow:" , +"upboard:green:" , +"upboard:red:" , +}; + +struct upboard_led {
drivers/leds/leds-upboard.c line 37
Quoted 10 lines
drivers/leds/leds-upboard.c new file +10
-0+struct regmap_field *field; +struct led_classdev cdev; +}; + +static enum led_brightness upboard_led_brightness_get (struct led_classdev *cdev) +{ +struct upboard_led *led =to_upboard_led (cdev); +int brightness =0 ; + +regmap_field_read (led->field, &brightness);
Please check the return value.
drivers/leds/leds-upboard.c line 47
Quoted 9 lines
drivers/leds/leds-upboard.c new file +9
-0+return brightness; +} + +static void upboard_led_brightness_set (struct led_classdev *cdev, +enum led_brightness brightness) +{ +struct upboard_led *led =to_upboard_led (cdev); + +regmap_field_write (led->field, brightness);
Ditto.
Quoted 28 lines
drivers/leds/leds-upboard.c new file +28
-0+} + +static int upboard_led_probe (struct platform_device *pdev) +{ +unsigned int led_index = pdev->id; +struct device *dev = &pdev->dev; +struct acpi_device *adev; +struct upboard_led *led; +struct regmap *regmap; +struct reg_field conf = { + .reg =UPBOARD_REG_FUNC_EN0 , + .lsb = led_index, + .msb = led_index, + }; + + adev =ACPI_COMPANION (dev); +if (!adev ||strcmp (acpi_device_hid (adev),"AANT0F01" )) +return -ENODEV ; + +if (led_index >=ARRAY_SIZE (upboard_led_names)) +return -EINVAL ; + +if (!dev->parent) +return -EINVAL ; + + regmap =dev_get_regmap (dev->parent,NULL ); +if (!regmap) +return -EINVAL ;
drivers/leds/leds-upboard.c line 85
Quoted 10 lines
drivers/leds/leds-upboard.c new file +10
-0+ + led =devm_kzalloc (dev,sizeof (*led),GFP_KERNEL ); +if (!led) +return -ENOMEM ; + + led->field =devm_regmap_field_alloc (dev, regmap, conf); +if (IS_ERR (led->field)) +return PTR_ERR (led->field); + + led->cdev.max_brightness =1 ;
s/1/LED_ON/
Quoted 20 lines
drivers/leds/leds-upboard.c new file +19
-0+ led->cdev.brightness_get = upboard_led_brightness_get; + led->cdev.brightness_set = upboard_led_brightness_set; + led->cdev.name = upboard_led_names[led_index]; + +return devm_led_classdev_register (dev, &led->cdev); +} + +static struct platform_driver upboard_led_driver = { + .driver = { + .name ="upboard-led" , + }, +}; + +module_platform_driver_probe (upboard_led_driver, upboard_led_probe); + +MODULE_ALIAS ("platform:upboard-led" ); +MODULE_AUTHOR ("Javier Arteaga <[email protected]>" ); +MODULE_DESCRIPTION ("UP Board LED driver" ); +MODULE_LICENSE ("GPL v2" );
Signature · 2 lines
Best regards, Jacek Anaszewski
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Hi!
Quoted 7 lines
drivers/leds/leds-upboard.c line 85
Quoted 5 lines
drivers/leds/leds-upboard.c new file +5
-0+ led->field =devm_regmap_field_alloc (dev, regmap, conf); +if (IS_ERR (led->field)) +return PTR_ERR (led->field); + + led->cdev.max_brightness =1 ;s/1/LED_ON/
Actually, I prefer constant 1 here, as it makes it immediately obvious this supports just 0/1.
Yes, LED_ON is also 1, but I had to grep the header files for that... (I thought it was 255).
Best regards,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Jacek Anaszewski
Acked-by Pavel MachekRe: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 10 lines · Pavel Machek
Hi!
Quoted 7 lines
drivers/leds/leds-upboard.c line 85
Quoted 5 lines
drivers/leds/leds-upboard.c new file +5
-0+ led->field =devm_regmap_field_alloc (dev, regmap, conf); +if (IS_ERR (led->field)) +return PTR_ERR (led->field); + + led->cdev.max_brightness =1 ;s/1/LED_ON/
Quoted 5 lines
Actually, I prefer constant 1 here, as it makes it immediately obvious this supports just 0/1.
Yes,
LED_ONis also 1, but I had to grep the header files for that... (I thought it was 255).
If we have the enum for that, let's use it.
Here's the commit message of the patch adding LED_ON - it should be somehow familiar to you - see the ack.
commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384
Author: Andi Shyti <[email protected]> Date: Thu Jan 5 11:34:12 2017 +0900
leds: add LED_ON brightness as boolean value
Some devices do not handle the led brightness or simply don't care about it. Conceptually said devices want to just switch on or off the led. It is useless in this case to have a 255 range of brightness, while just having an LED_ON and LED_OFF improves the boolean meaning of the led status.
Signature · 2 lines
Best regards, Jacek Anaszewski
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 14 lines · Jacek Anaszewski
Quoted 13 lines · Pavel Machek
Hi!
Quoted 7 lines
drivers/leds/leds-upboard.c line 85
Quoted 5 lines
drivers/leds/leds-upboard.c new file +5
-0+ led->field =devm_regmap_field_alloc (dev, regmap, conf); +if (IS_ERR (led->field)) +return PTR_ERR (led->field); + + led->cdev.max_brightness =1 ;s/1/LED_ON/
Actually, I prefer constant 1 here, as it makes it immediately obvious this supports just 0/1.
Quoted 6 lines
Quoted 2 lines
Yes,
LED_ONis also 1, but I had to grep the header files for that... (I thought it was 255).If we have the enum for that, let's use it.
Here's the commit message of the patch addingLED_ON- it should be somehow familiar to you - see the ack.
Well .. brightness = LED_ON; is good usage. max_brightness = LED_ON is IMO less readable than max_brightness = 1.
Looking at situation again... Having LED_ON and LED_FULL, with some leds having max brightness of 1023, so LED_FULL is not really full brightness any more... Maybe it is time to get rid of the enum, and make it plain int. It does not really enumarate anything, and it does not help readability, either.
Pavel
Quoted 17 lines
commit
4e552c8cb5bc9137e67e035bab8df6dddbca7384Author: Andi Shyti <[email protected]> Date: Thu Jan 5 11:34:12 2017 +0900leds: add
LED_ONbrightness as boolean valueSome devices do not handle the led brightness or simply don't care about it. Conceptually said devices want to just switch on or off the led. It is useless in this case to have a 255 range of brightness, while just having an
LED_ONandLED_OFFimproves the boolean meaning of the led status.Signed-off-by Andi Shyti <[email protected]>Acked-by Pavel Machek <[email protected]>Signed-off-by Jacek Anaszewski <[email protected]>
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 22 lines · Pavel Machek
Quoted 14 lines · Jacek Anaszewski
Quoted 13 lines · Pavel Machek
Hi!
Quoted 7 lines
drivers/leds/leds-upboard.c line 85
Quoted 5 lines
drivers/leds/leds-upboard.c new file +5
-0+ led->field =devm_regmap_field_alloc (dev, regmap, conf); +if (IS_ERR (led->field)) +return PTR_ERR (led->field); + + led->cdev.max_brightness =1 ;s/1/LED_ON/
Actually, I prefer constant 1 here, as it makes it immediately obvious this supports just 0/1.
Quoted 6 lines
Quoted 2 lines
Yes,
LED_ONis also 1, but I had to grep the header files for that... (I thought it was 255).If we have the enum for that, let's use it.
Here's the commit message of the patch addingLED_ON- it should be somehow familiar to you - see the ack.
Quoted 8 lines
Well .. brightness =
LED_ON; is good usage.max_brightness=LED_ONis IMO less readable thanmax_brightness= 1.Looking at situation again... Having
LED_ONandLED_FULL, with some leds having max brightness of 1023, soLED_FULLis not really full brightness any more... Maybe it is time to get rid of the enum, and make it plain int. It does not really enumarate anything, and it does not help readability, either.
I agree that it introduces confusion.
Yet, there are many out of LED subsystem files to update:
find -name "*.c" -o -name "*.h" | xargs grep "enum led_brightness" | awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l
returns 87.
Quoted 18 lines
Quoted 17 lines
commit
4e552c8cb5bc9137e67e035bab8df6dddbca7384Author: Andi Shyti <[email protected]> Date: Thu Jan 5 11:34:12 2017 +0900leds: add
LED_ONbrightness as boolean valueSome devices do not handle the led brightness or simply don't care about it. Conceptually said devices want to just switch on or off the led. It is useless in this case to have a 255 range of brightness, while just having an
LED_ONandLED_OFFimproves the boolean meaning of the led status.Signed-off-by Andi Shyti <[email protected]>Acked-by Pavel Machek <[email protected]>Signed-off-by Jacek Anaszewski <[email protected]>
Signature · 2 lines
Best regards, Jacek Anaszewski
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 6 lines · Jacek Anaszewski
Yet, there are many out of LED subsystem files to update:
find -name "*.c" -o -name "*.h" | xargs grep "enum
led_brightness" | awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -lreturns 87.
Side note:
git grep -n 'enum led_brightness' | ...
a bit more effitient.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Jacek Anaszewski
Yet, there are many out of LED subsystem files to update:
find -name "*.c" -o -name "*.h" | xargs grep "enum
led_brightness" | awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l
Quoted 7 lines
Quoted 1 line
returns 87.
Side note:
git grep -n 'enum
led_brightness' | ...a bit more effitient.
Right, thanks for the hint.
Signature · 2 lines
Best regards, Jacek Anaszewski
Re: [PATCH v2 2/3] leds: upboard: Add LED support
In reply to Jacek AnaszewskiQuoted 6 lines · Jacek Anaszewski
drivers/leds/leds-upboard.c
Quoted 6 lines
drivers/leds/leds-upboard.c new file +0
-0@@ -0,0 +1,104 @@
[]
Quoted 9 lines
drivers/leds/leds-upboard.c line 16
Quoted 7 lines
drivers/leds/leds-upboard.c new file +7
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h> The last include should go first to keep alphabetical order.
There is no accepted single kernel style for #include file ordering.
drivers/leds does not use a single style nor is this particular variant documented anywhere to my knowledge.
Until such a time when either a local preferred style document or a treewide preferred style exists, please stop asking people to modify #include ordering for various styles like reverse christmas tree by length, alphabetic ordering, or other individual styles.
My preferred style would always have kernel.h first as that may help with precompiled headers and overall kernel compilation time one day.
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 18 lines · Joe Perches
Quoted 6 lines · Jacek Anaszewski
drivers/leds/leds-upboard.c
Quoted 6 lines
drivers/leds/leds-upboard.c new file +0
-0@@ -0,0 +1,104 @@[]
Quoted 9 lines
drivers/leds/leds-upboard.c line 16
Quoted 7 lines
drivers/leds/leds-upboard.c new file +7
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h> The last include should go first to keep alphabetical order.
Quoted 2 lines
There is no accepted single kernel style for #include file ordering.
Well, I myself have been asked several times for sorting includes, that's why I keep requiring it for the LED subsystem too.
When it's by the occasion of a new driver submission, it costs virtually nothing. And it allows for avoiding any prospective noise on the list due to the submissions like "Order includes".
As a first shot the following returns 77:
git log | grep -i "include.*alphabetical" | wc -l
Aside of that, in case of this particular patch the intention seemingly was to have includes ordered lexicographically, since only the last item didn't fit for this pattern.
Quoted 2 lines
drivers/ledsdoes not use a single style nor is this particular variant documented anywhere to my knowledge.
Unspecified kind of includes sorting is mentioned in the Documentation/process/coding-style.rst, line 637.
Quoted 8 lines
Until such a time when either a local preferred style document or a treewide preferred style exists, please stop asking people to modify #include ordering for various styles like reverse christmas tree by length, alphabetic ordering, or other individual style My preferred style would always have
kernel.hfirst as that may help with precompiled headers and overall kernel compilation time one day.
If that will happen we'll see massive rearrangement of includes anyway.
But OK, I've skimmed through other subsystems and core kernel files and realized that this is indeed often not preserved.
So, provided there are no other strong arguments in favor of sorting, I will give up this nitpicking from now on.
Signature · 2 lines
Best regards, Jacek Anaszewski
Re: [PATCH v2 2/3] leds: upboard: Add LED support
In reply to Joe PerchesQuoted 18 lines · Joe Perches
Quoted 6 lines · Jacek Anaszewski
drivers/leds/leds-upboard.c
Quoted 6 lines
drivers/leds/leds-upboard.c new file +0
-0@@ -0,0 +1,104 @@[]
Quoted 9 lines
drivers/leds/leds-upboard.c line 16
Quoted 7 lines
drivers/leds/leds-upboard.c new file +7
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h> The last include should go first to keep alphabetical order.
Quoted 2 lines
There is no accepted single kernel style for #include file ordering.
There is a rule of (subtly) better maintenance.
If you need to add / remove some header later in a (long) list of unordered list, it would be error prone.
Just run make includecheck and see the result.
I personally fixed some header duplications and removal of init.h in unsorted lists, which have been missed by some reasons.
Quoted 2 lines
drivers/ledsdoes not use a single style nor is this particular variant documented anywhere to my knowledge.
Neither does kernel in general.
But kernel is evolving and styles also. When you do such statement consider to divide by a time period when certain code was pushed to upstream.
Quoted 5 lines
Until such a time when either a local preferred style document or a treewide preferred style exists, please stop asking people to modify #include ordering for various styles like reverse christmas tree by length, alphabetic ordering, or other individual styles.
Why? It makes a sense to ask for new code (and even for patches against old one in some cases).
Quoted 3 lines
My preferred style would always have
kernel.hfirst as that may help with precompiled headers and overall kernel compilation time one day.
How ordering would screw this up?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] leds: upboard: Add LED support
Quoted 8 lines · Andy Shevchenko
Quoted 5 lines · Joe Perches
Until such a time when either a local preferred style document or a treewide preferred style exists, please stop asking people to modify #include ordering for various styles like reverse christmas tree by length, alphabetic ordering, or other individual styles.
Why? It makes a sense to ask for new code (and even for patches against old one in some cases).
It's just a nit and frequently impossible to require as ordering dependencies between include files do exist.
Quoted 5 lines
Quoted 3 lines
My preferred style would always have
kernel.hfirst as that may help with precompiled headers and overall kernel compilation time one day.How ordering would screw this up?
gcc has many limits on the use of precompiled headers.
https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html
Precompiled headers are often shared by multiple compilation units and precompilation can be stopped after a specific header.
PATCH v2 3/3 pinctrl: upboard: Add UP2 pinctrl and gpio driver
In reply to Dan O'DonovanFrom: Javier Arteaga <[email protected]>
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)
To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
--- drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 519 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 533 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-upboard.c
drivers/pinctrl/Kconfig +13 -0
@@ -338,6 +338,19 @@ config PINCTRL_OCELOT select GENERIC_PINMUX_FUNCTIONS select REGMAP_MMIO +config PINCTRL_UPBOARD + tristate "UP Squared pinctrl and GPIO driver" + depends on ACPI + depends on MFD_UPBOARD + select PINMUX + help + Pinctrl driver for the pin headers on the UP Squared board. It + handles pin control for lines routed through the on-board FPGA and + propagates changes to the SoC pinctrl to keep them in sync. + + This driver can also be built as a module. If so, the module will be + called pinctrl-upboard. + source "drivers/pinctrl/actions/Kconfig" source "drivers/pinctrl/aspeed/Kconfig" source "drivers/pinctrl/bcm/Kconfig"
drivers/pinctrl/Makefile +1 -0
@@ -42,6 +42,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o +obj-$(CONFIG_PINCTRL_UPBOARD) += pinctrl-upboard.o obj-y += actions/ obj-$(CONFIG_ARCH_ASPEED) += aspeed/
drivers/pinctrl/pinctrl-upboard.c new file +519 -0
519 lines not on this page · raw message
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Quoted 18 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Quoted 5 lines
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
To Linus: please, don't consider this as anyhow part of Intel pin control infrastructure. Thus, if you are okay with the driver (personally I don't see any major issues with the code, though it might be required some clarification on design level, e.g. ACPI relationship) I have no objection.
drivers/pinctrl/pinctrl-upboard.c line 51
Quoted 2 lines
drivers/pinctrl/pinctrl-upboard.c new file +2
-0+#define UPBOARD_BIT_TO_PIN (r, bit) \ + ((r) * UPBOARD_REGISTER_SIZE + (bit))
One line?
Quoted 16 lines
drivers/pinctrl/pinctrl-upboard.c new file +16
-0+static int upboard_get_functions_count (struct pinctrl_dev *pctldev) +{ +return 0 ; +} + +static int upboard_get_function_groups (struct pinctrl_dev *pctldev, +unsigned int selector, +const char *const **groups, +unsigned int *num_groups) +{ + *groups =NULL ; + *num_groups =0 ; +return 0 ; +} + +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
drivers/pinctrl/pinctrl-upboard.c line 210
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +10
-0+ unsigned int selector) +{ +return NULL ; +} + +static int upboard_set_mux (struct pinctrl_dev *pctldev,unsigned int function, +unsigned int group) +{ +return 0 ; +}
Hmm... Do you need those stubs? Same Q for other stubs in the file.
drivers/pinctrl/pinctrl-upboard.c line 221
Quoted 11 lines
drivers/pinctrl/pinctrl-upboard.c new file +10
-0+static int upboard_gpio_request_enable (struct pinctrl_dev *pctldev, +struct pinctrl_gpio_range *range, +unsigned int pin) +{ +const struct pin_desc *const pd =pin_desc_get (pctldev, pin); +const struct upboard_pin *p; +int ret; + +if (!pd) +return -EINVAL ;
When it possible to happen?
Same Q for the rest same excerpts.
Quoted 14 lines
drivers/pinctrl/pinctrl-upboard.c new file +14
-0+ p = pd->drv_data; + +/* if this pin has an associated function bit, disable it first */ +if (p->func_en) { + ret =regmap_field_write (p->func_en,0 ); +if (ret) +return ret; + } + +if (p->gpio_en) { + ret =regmap_field_write (p->gpio_en,1 ); +if (ret) +return ret; + }
drivers/pinctrl/pinctrl-upboard.c line 291
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +8
-0+ +return 0 ; +} +static struct gpio_desc *upboard_offset_to_soc_gpio (struct gpio_chip *gc, +unsigned int offset) +{ +struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip);
One line?
drivers/pinctrl/pinctrl-upboard.c line 294
Quoted 3 lines
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+ +if (offset +1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset]) +return ERR_PTR (-ENODEV );
offset >= ?
Is it even possible?
drivers/pinctrl/pinctrl-upboard.c line 302
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c new file +8
-0+ +return pctrl->soc_gpios[offset]; +} + +static int upboard_gpio_request (struct gpio_chip *gc,unsigned int offset) +{ +struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip);
One line?
drivers/pinctrl/pinctrl-upboard.c line 321
Quoted 6 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+} + +static void upboard_gpio_free (struct gpio_chip *gc,unsigned int offset) +{ +struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip);
Ditto.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset]) + return;
offset >= ?
Is it even possible?
Quoted 6 lines
drivers/pinctrl/pinctrl-upboard.c new file +5
-0+} +static int upboard_pinctrl_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct pinctrl_desc *pctldesc;
drivers/pinctrl/pinctrl-upboard.c line 437
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +9
-0+struct upboard_pinctrl *pctrl; +struct upboard_pin *pins; +struct acpi_device *adev; +struct regmap *regmap; +unsigned int i; +int ret; + adev =ACPI_COMPANION (dev); +if (!adev ||strcmp (acpi_device_hid (adev),"AANT0F01" )) +return -ENODEV ;
Same Q as per LED driver.
drivers/pinctrl/pinctrl-upboard.c line 457
Quoted 5 lines
drivers/pinctrl/pinctrl-upboard.c new file +4
-0+for (i =0 ; i < pctldesc->npins; i++) { +struct upboard_pin *pin = &pins[i]; +const struct pinctrl_pin_desc *pd = &pctldesc->pins[i]; + pin->func_en =NULL ;
Useless.
Quoted 11 lines
drivers/pinctrl/pinctrl-upboard.c new file +11
-0+if (pd->drv_data) { +struct reg_field *field = pd->drv_data; + + pin->func_en =devm_regmap_field_alloc (dev, regmap, + *field); +if (IS_ERR (pin->func_en)) +return PTR_ERR (pin->func_en); + } + + pin->gpio_en =upboard_field_alloc (dev, regmap, +UPBOARD_REG_GPIO_EN0 , i);
drivers/pinctrl/pinctrl-upboard.c line 477
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +9
-0+if (IS_ERR (pin->gpio_en)) +return PTR_ERR (pin->gpio_en); + + pin->gpio_dir =upboard_field_alloc (dev, regmap, +UPBOARD_REG_GPIO_DIR0 , i); +if (IS_ERR (pin->gpio_dir)) +return PTR_ERR (pin->gpio_dir); + + ((struct pinctrl_pin_desc *)pd)->drv_data = pin;
I'm not sure I understand the purpose of this casting.
drivers/pinctrl/pinctrl-upboard.c line 493
Quoted 7 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+ } + pctrl->soc_gpios =devm_kzalloc (dev, + pctrl->nsoc_gpios *sizeof (*pctrl->soc_gpios), +GFP_KERNEL ); +if (!pctrl->soc_gpios) +return -ENOMEM ;
kzalloc -> kcalloc
Quoted 1 line
+}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Thanks for your review feedback, Andy! I'll send a v3 shortly with those changes you suggested. I've added some comments inline below.
Quoted 21 lines · Andy Shevchenko
Quoted 19 lines · Dan O'Donovan
drivers/pinctrl/pinctrl-upboard.c new file +19
-0+static int upboard_get_functions_count (struct pinctrl_dev *pctldev) +{ +return 0 ; +} + +static int upboard_get_function_groups (struct pinctrl_dev *pctldev, +unsigned int selector, +const char *const **groups, +unsigned int *num_groups) +{ + *groups =NULL ; + *num_groups =0 ; +return 0 ; +} + +static const char *upboard_get_function_name (struct pinctrl_dev *pctldev, +unsigned int selector) +{ +return NULL ;
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c line 210
Quoted 7 lines
drivers/pinctrl/pinctrl-upboard.c new file +7
-0+} + +static int upboard_set_mux (struct pinctrl_dev *pctldev,unsigned int function, +unsigned int group) +{ +return 0 ; +}Hmm... Do you need those stubs? Same Q for other stubs in the file.
It looks like they're required by pinctrl core, which returns an error if they're not provided.
Quoted 4 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-upboard.c new file +4
-0+static int upboard_gpio_request_enable (struct pinctrl_dev *pctldev, +struct pinctrl_gpio_range *range, +unsigned int pin) +{
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c line 221
Quoted 6 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+const struct pin_desc *const pd =pin_desc_get (pctldev, pin); +const struct upboard_pin *p; +int ret; + +if (!pd) +return -EINVAL ;When it possible to happen?
Same Q for the rest same excerpts.
Agreed, it shouldn't be possible. I will remove these checks.
Quoted 5 lines
drivers/pinctrl/pinctrl-upboard.c line 294
Quoted 3 lines
drivers/pinctrl/pinctrl-upboard.c new file +3
-0+ +if (offset +1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset]) +return ERR_PTR (-ENODEV );offset >= ?
Is it even possible?
Agreed, it shouldn't be possible. I will remove these checks.
Quoted 6 lines
Quoted 6 lines
drivers/pinctrl/pinctrl-upboard.c new file +6
-0+static int upboard_pinctrl_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct pinctrl_desc *pctldesc; +struct upboard_pinctrl *pctrl; +struct upboard_pin *pins;
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c line 437
Quoted 7 lines
drivers/pinctrl/pinctrl-upboard.c new file +7
-0+struct acpi_device *adev; +struct regmap *regmap; +unsigned int i; +int ret; + adev =ACPI_COMPANION (dev); +if (!adev ||strcmp (acpi_device_hid (adev),"AANT0F01" )) +return -ENODEV ;Same Q as per LED driver.
I agree. I will remove this check, both here and in the LED driver.
Quoted 13 lines
Quoted 13 lines
drivers/pinctrl/pinctrl-upboard.c new file +13
-0+if (pd->drv_data) { +struct reg_field *field = pd->drv_data; + + pin->func_en =devm_regmap_field_alloc (dev, regmap, + *field); +if (IS_ERR (pin->func_en)) +return PTR_ERR (pin->func_en); + } + + pin->gpio_en =upboard_field_alloc (dev, regmap, +UPBOARD_REG_GPIO_EN0 , i); +if (IS_ERR (pin->gpio_en)) +return PTR_ERR (pin->gpio_en);
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c line 477
Quoted 7 lines
drivers/pinctrl/pinctrl-upboard.c new file +7
-0+ + pin->gpio_dir =upboard_field_alloc (dev, regmap, +UPBOARD_REG_GPIO_DIR0 , i); +if (IS_ERR (pin->gpio_dir)) +return PTR_ERR (pin->gpio_dir); + + ((struct pinctrl_pin_desc *)pd)->drv_data = pin;I'm not sure I understand the purpose of this casting.
When the pd pointer is retrieved from struct pinctrl_desc, it has a const constraint. The purpose of the cast is to bypass that constraint for this use case, because this code is allocating and setting drv_data dynamically here at run-time rather than at compile-time.
Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
In reply to Dan O'DonovanQuoted 4 lines · Dan O'Donovan
From: Javier Arteaga <[email protected]>
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT).
Which makes me want to have Eric Anholt's review on this patch so as to secure basic interoperability with that header.
Quoted 3 lines
Both expose assorted
functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
OK
Look at how RPi define names for their GPIO lines in the DTS file:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/bcm2835-rpi-b.dts
Please follow this pattern with your patch.
As you probably do not have device tree or anything similar for ACPI to name the lines (correct me if I'm wrong) you can use the .names array in struct gpio_chip for hardcoding the proper line names.
lsgpio should give the same line names as it does on the corresponding RPi header IMO.
drivers/pinctrl/Kconfig config PINCTRL_OCELOT · line 345
Quoted 5 lines
drivers/pinctrl/Kconfig +5
-0+config PINCTRL_UPBOARD + tristate "UP Squared pinctrl and GPIO driver" + depends on ACPI + depends on MFD_UPBOARD + select PINMUX
select GPIOLIB
as you're using it.
Quoted 16 lines
drivers/pinctrl/pinctrl-upboard.c new file +16
-0+static int upboard_get_functions_count (struct pinctrl_dev *pctldev) +{ +return 0 ; +} + +static int upboard_get_function_groups (struct pinctrl_dev *pctldev, +unsigned int selector, +const char *const **groups, +unsigned int *num_groups) +{ + *groups =NULL ; + *num_groups =0 ; +return 0 ; +} + +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
drivers/pinctrl/pinctrl-upboard.c line 210
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +10
-0+ unsigned int selector) +{ +return NULL ; +} + +static int upboard_set_mux (struct pinctrl_dev *pctldev,unsigned int function, +unsigned int group) +{ +return 0 ; +}
This just looks weird.
There seems to be code to disable pins and turn them into GPIOs in upboard_gpio_request_enable() but no way to switch them back to the original function, is that how it works?
I guess it is fine if that is how it's supposed to be used. But won't some grumpy users come around and complain about this one day?
We can fix it when it happens though.
Quoted 4 lines
drivers/pinctrl/pinctrl-upboard.c new file +4
-0+static int upboard_gpio_request (struct gpio_chip *gc,unsigned int offset) +{ +struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip);
drivers/pinctrl/pinctrl-upboard.c line 312
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +10
-0+struct gpio_desc *desc; +int ret; + + ret =pinctrl_gpio_request (gc->base + offset); +if (ret) +return ret; + + desc =devm_gpiod_get_index (gc->parent,"external" , offset,GPIOD_ASIS ); +if (IS_ERR (desc)) +return PTR_ERR (desc);
No please don't do this. The consumers should request the gpio, not the driver.
drivers/pinctrl/pinctrl-upboard.c line 326
Quoted 9 lines
drivers/pinctrl/pinctrl-upboard.c new file +9
-0+static void upboard_gpio_free (struct gpio_chip *gc,unsigned int offset) +{ +struct upboard_pinctrl *pctrl = + container_of(gc, struct upboard_pinctrl, chip); + +if (offset +1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset]) +return ; + +devm_gpiod_put (gc->parent, pctrl->soc_gpios[offset]);
Dito.
drivers/pinctrl/pinctrl-upboard.c line 340
Quoted 9 lines
drivers/pinctrl/pinctrl-upboard.c new file +9
-0+static int upboard_gpio_get_direction (struct gpio_chip *gc,unsigned int offset) +{ +struct gpio_desc *desc =upboard_offset_to_soc_gpio (gc, offset); + +if (IS_ERR (desc)) +return PTR_ERR (desc); + +return gpiod_get_direction (desc); +}
This is just confusing me even more...
If you need pinctrl_gpio_get_direction() then it should be added to the API in <linux/pinctrl/consumer.h>.
Quoted 4 lines
drivers/pinctrl/pinctrl-upboard.c new file +4
-0+static int upboard_gpio_direction_output (struct gpio_chip *gc, +unsigned int offset,int value) +{ +struct gpio_desc *desc =upboard_offset_to_soc_gpio (gc, offset);
drivers/pinctrl/pinctrl-upboard.c line 371
Quoted 10 lines
drivers/pinctrl/pinctrl-upboard.c new file +10
-0+int ret; + +if (IS_ERR (desc)) +return PTR_ERR (desc); + + ret =pinctrl_gpio_direction_output (gc->base + offset); +if (ret) +return ret; + +return gpiod_direction_output (desc, value);
No this looks confusing too.
drivers/pinctrl/pinctrl-upboard.c line 381
Quoted 8 lines
drivers/pinctrl/pinctrl-upboard.c new file +8
-0+static int upboard_gpio_get_value (struct gpio_chip *gc,unsigned int offset) +{ +struct gpio_desc *desc =upboard_offset_to_soc_gpio (gc, offset); + +if (IS_ERR (desc)) +return PTR_ERR (desc); + +return gpiod_get_value (desc);
I don't get this masking one GPIO chip behind another GPIO chip. It looks really weird.
What we usually have is a GPIO chip in front of a pin controller utilizing
extern int pinctrl_gpio_request(unsigned gpio);
extern void pinctrl_gpio_free(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio); extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);
these things for the GPIO chip to talk to the pin control back-end.
This driver seems to use a GPIO chip in front of a GPIO chip and a pin controller too (or something like that) and that makes me very uneasy.
I need a clear picture of the internal architectur of the GPIO parts of this driver, why the GPIO accessors are calling back into the GPIO layer etc. It looks very unorthodox to me, and I get very confused.
Yours.
Linus Walleij
Quoted 15 lines · Dan O'Donovan
Hi all,
This series adds platform support for AAEON's UP Squared, a x86 SBC based on Apollo Lake [1]. Specifically, it enables control for the board pin headers and for the LEDs, as both of these features are only available behind a FPGA-based platform controller.
This is structured around a MFD driver that provides a regmap interface to that platform controller to each MFD cell.
The series is carved out from an out-of-tree support patchset, initially written for the original UP Board [2] and then extended through the past year to support new entries in the lineup (UP Board, UP Squared and UP Core) [3].
Quoted 6 lines
Here we only submit support for UP Squared to simplify review. Still, the driver is designed so that it can easily gain support for the other boards.
V1 was originally submitted by Javier Arteaga in April 2018 as an RFC [4]. This V2 aims to address the feedback gratefully received from that.
Are you attending ELCE in Edinburgh?
Quoted 44 lines
[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/[email protected]
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
[4]: https://lore.kernel.org/lkml/[email protected]/V2:
* Rewrote regmap r/w handlers to avoid duplication and improve readability
* Simplified data passing between MFD parent and child device drivers
* Removed incorrect__initattribute from probe functions
* Moved MFD cell definitions into same patch as MFD driver and simplified them
* Promoted somedev_dbginstances todev_errfor probe error cases
* Decoupled MFD cell reference from ACPI driver data
* Miscellanous coding style improvements
* Fixed license mismatchesJavier Arteaga (3):
mfd: upboard: Add UP2 platform controller driver
leds: upboard: Add LED support
pinctrl: upboard: Add UP2 pinctrl and gpio driverdrivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 104 ++++++++ drivers/mfd/Kconfig | 17 ++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 344 +++++++++++++++++++++++++ drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 519 ++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++ 10 files changed, 1054 insertions(+) create mode 100644 drivers/leds/leds-upboard.c create mode 100644 drivers/mfd/upboard.c create mode 100644 drivers/pinctrl/pinctrl-upboard.c create mode 100644 include/linux/mfd/upboard.hSignature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
Signature · 2 lines
With Best Regards, Andy Shevchenko
v3 cover UP Squared board drivers
In reply to Dan O'DonovanHi all,
This series adds platform support for AAEON's UP Squared, a x86 SBC based on Apollo Lake [1]. Specifically, it enables control for the board pin headers and for the LEDs, as both of these features are only available behind a FPGA-based platform controller.
This is structured around a MFD driver that provides a regmap interface to that platform controller to each MFD cell.
The series is carved out from an out-of-tree support patchset, initially written for the original UP Board [2] and then extended through the past year to support new entries in the lineup (UP Board, UP Squared and UP Core) [3].
Here we only submit support for UP Squared to simplify review. Still, the driver is designed so that it can easily gain support for the other boards.
V1 was originally submitted by Javier Arteaga in April 2018 as an RFC [4].
The relevant ACPI SSDT from UP Squared can be found here: [5]
[1]: http://www.up-board.org/upsquared/specifications-up2/
[2]: https://lkml.kernel.org/r/[email protected]
[3]: https://github.com/emutex/ubilinux-kernel/commits/upboard-4.9
[4]: https://lore.kernel.org/lkml/[email protected]/
[5]: https://gist.github.com/Dan-Emutex/6382c25f4b8b8cdd80e6056889cdf48b
V3:
* Javier's address is no longer valid. Replaced From: with my own address.
* Added error checks in LEDs driver, suggested by Jacek Anaszewski
* Added minor tidy-ups in MFD and pinctrl drivers, suggested by Andy Shevchenko
* Added link to relevant ACPI extract above [5]
V2:
* Rewrote regmap r/w handlers to avoid duplication and improve readability
* Simplified data passing between MFD parent and child device drivers
* Removed incorrect __init attribute from probe functions
* Moved MFD cell definitions into same patch as MFD driver and simplified them
* Promoted some dev_dbg instances to dev_err for probe error cases
* Decoupled MFD cell reference from ACPI driver data
* Miscellanous coding style improvements
* Fixed license mismatches
Dan O'Donovan (3):
mfd: upboard: Add UP2 platform controller driver
leds: upboard: Add LED support
pinctrl: upboard: Add UP2 pinctrl and gpio driver
drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 103 ++++++++ drivers/mfd/Kconfig | 17 ++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 336 ++++++++++++++++++++++++++ drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 496 ++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++ 10 files changed, 1022 insertions(+) create mode 100644 drivers/leds/leds-upboard.c create mode 100644 drivers/mfd/upboard.c create mode 100644 drivers/pinctrl/pinctrl-upboard.c create mode 100644 include/linux/mfd/upboard.h
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
PATCH v3 1/3 mfd: upboard: Add UP2 platform controller driver
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+
| 40-pin RPi-like header |
+------| (HAT) |
| +------------------------+
+-------+ +--------+
| | | | +------------------------+
| SoC |----| FPGA |-----| Custom UP2 pin header |
| | | | | (EXHAT) |
+-------+ +--------+ +------------------------+
|
+------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
--- drivers/mfd/Kconfig | 17 +++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 336 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++++ 4 files changed, 398 insertions(+) create mode 100644 drivers/mfd/upboard.c create mode 100644 include/linux/mfd/upboard.h
drivers/mfd/Kconfig +17 -0
@@ -1871,6 +1871,23 @@ config MFD_STM32_TIMERS for PWM and IIO Timer. This driver allow to share the registers between the others drivers. +config MFD_UPBOARD + tristate "UP Squared" + depends on ACPI + depends on GPIOLIB + select MFD_CORE + select REGMAP + help + If you say yes here you get support for the platform controller + of the UP Squared single-board computer. + + This driver provides common support for accessing the device, + additional drivers must be enabled in order to use the + functionality of the device. + + This driver can also be built as a module. If so, the module + will be called "upboard". + menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100
drivers/mfd/Makefile +1 -0
@@ -242,3 +242,4 @@ obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o +obj-$(CONFIG_MFD_UPBOARD) += upboard.o
drivers/mfd/upboard.c new file +336 -0
336 lines not on this page · raw message
include/linux/mfd/upboard.h new file +44 -0
44 lines not on this page · raw message
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver
Quoted 35 lines · Dan O'Donovan
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+ | 40-pin RPi-like header | +------| (HAT) | | +------------------------+ +-------+ +--------+ | | | | +------------------------+ | SoC |----| FPGA |-----| Custom UP2 pin header | | | | | | (EXHAT) | +-------+ +--------+ +------------------------+ | +------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
Acked-by Linus Walleij <[email protected]>Signed-off-by Dan O'Donovan <[email protected]>
Quoted 8 lines
--- drivers/mfd/Kconfig | 17 +++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 336 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++++ 4 files changed, 398 insertions(+) create mode 100644 drivers/mfd/upboard.c create mode 100644 include/linux/mfd/upboard.h
Looks like you've ignored the review comments here too.
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver
Quoted 45 lines · Lee Jones
Quoted 35 lines · Dan O'Donovan
UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It features a MAX 10 FPGA that routes lines from both SoC and on-board devices to two I/O headers:
+------------------------+ | 40-pin RPi-like header | +------| (HAT) | | +------------------------+ +-------+ +--------+ | | | | +------------------------+ | SoC |----| FPGA |-----| Custom UP2 pin header | | | | | | (EXHAT) | +-------+ +--------+ +------------------------+ | +------* On-board devices: LED, VLS...This is intended to enable vendor-specific applications to customize I/O header pinout, as well as include low-latency functionality. It also performs voltage level translation between the SoC (1.8V) and HAT header (3.3V).
Out of the box, this block implements a platform controller with a GPIO-bitbanged control interface. It's enumerated by ACPI and provides registers to control:
- Configuration of all FPGA-routed header lines. These can be driven SoC-to-header, header-to-SoC or set in high impedance.
- On-board LEDs and enable lines for other platform devices.
Add core support for this platform controller as a MFD device, exposing these registers as a regmap.
Acked-by Linus Walleij <[email protected]>Signed-off-by Dan O'Donovan <[email protected]>Quoted 8 lines
--- drivers/mfd/Kconfig | 17 +++ drivers/mfd/Makefile | 1 + drivers/mfd/upboard.c | 336 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/upboard.h | 44 ++++++ 4 files changed, 398 insertions(+) create mode 100644 drivers/mfd/upboard.c create mode 100644 include/linux/mfd/upboard.h
Quoted 1 line
Looks like you've ignored the review comments here too.
Hi Lee. Looks like I didn't receive your original email, most likely because of a crappy spam filtering service we were using here up until recently. I've found your comments in online archives now and will address them in the next revision of the patches. Sorry for the inconvenience. :(
Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver
In reply to Dan O'DonovanHi!
drivers/mfd/upboard.c line 72
Quoted 10 lines
drivers/mfd/upboard.c new file +10
-0+static void __reg_io_start (const struct upboard_ddata *const ddata) +{ +/* +* CLEAR signal must be pulsed low before any register access. +* This resets internal counters in the controller and marks +* the start of a new register access. +*/ +set_clear (ddata,0 ); +set_clear (ddata,1 ); +}
We normally reserve __ prefix for different stuff. This is not using it appropriately.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v3 1/3] mfd: upboard: Add UP2 platform controller driver
Quoted 14 lines · Pavel Machek
drivers/mfd/upboard.c line 72
Quoted 11 lines
drivers/mfd/upboard.c new file +10
-0+static void __reg_io_start (const struct upboard_ddata *const ddata) +{ +/* +* CLEAR signal must be pulsed low before any register access. +* This resets internal counters in the controller and marks +* the start of a new register access. +*/ +set_clear (ddata,0 ); +set_clear (ddata,1 ); +}We normally reserve __ prefix for different stuff. This is not using it appropriately.
+1
I actively dislike __underscore_notation as it seems to be completely arbitrary and ambiguous. I think even do_the_stuff_reg_io_start() is better.
But please come up with something to the point and descriptive! :)
Yours,
Linus Walleij
PATCH v3 2/3 leds: upboard: Add LED support
In reply to Dan O'DonovanAllow userspace to use the on-board LEDs as "upboard:<color>:".
--- drivers/leds/Kconfig | 10 +++++ drivers/leds/Makefile | 1 + drivers/leds/leds-upboard.c | 103 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+) create mode 100644 drivers/leds/leds-upboard.c
drivers/leds/Kconfig +10 -0
@@ -766,6 +766,16 @@ config LEDS_NIC78BX To compile this driver as a module, choose M here: the module will be called leds-nic78bx. +config LEDS_UPBOARD + tristate "LED support for the UP Squared" + depends on LEDS_CLASS + depends on MFD_UPBOARD + help + This option enables support for the LEDs on the UP Squared board. + + This driver can also be built as a module. If so, the module + will be called leds-upboard. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig"
drivers/leds/Makefile +1 -0
@@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o +obj-$(CONFIG_LEDS_UPBOARD) += leds-upboard.o # LED SPI Drivers obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
drivers/leds/leds-upboard.c new file +103 -0
103 lines not on this page · raw message
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
PATCH v3 3/3 pinctrl: upboard: Add UP2 pinctrl and gpio driver
In reply to Dan O'DonovanThe UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)
To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
--- drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-upboard.c | 496 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 510 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-upboard.c
drivers/pinctrl/Kconfig +13 -0
@@ -350,6 +350,19 @@ config PINCTRL_OCELOT select GENERIC_PINMUX_FUNCTIONS select REGMAP_MMIO +config PINCTRL_UPBOARD + tristate "UP Squared pinctrl and GPIO driver" + depends on ACPI + depends on MFD_UPBOARD + select PINMUX + help + Pinctrl driver for the pin headers on the UP Squared board. It + handles pin control for lines routed through the on-board FPGA and + propagates changes to the SoC pinctrl to keep them in sync. + + This driver can also be built as a module. If so, the module will be + called pinctrl-upboard. + source "drivers/pinctrl/actions/Kconfig" source "drivers/pinctrl/aspeed/Kconfig" source "drivers/pinctrl/bcm/Kconfig"
drivers/pinctrl/Makefile +1 -0
@@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o +obj-$(CONFIG_PINCTRL_UPBOARD) += pinctrl-upboard.o obj-y += actions/ obj-$(CONFIG_ARCH_ASPEED) += aspeed/
drivers/pinctrl/pinctrl-upboard.c new file +496 -0
496 lines not on this page · raw message
Signature · 5 lines
2.7.4 ------ This email has been scanned for spam and malware by The Email Laundry.
Re: [PATCH v3 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Hi Dan,
Quoted 15 lines · Dan O'Donovan
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Quoted 7 lines
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
Signed-off-by Dan O'Donovan <[email protected]>
It appears you missed my review comments so please read them and reply or respin the patch accordingly.
Yours,
Linus Walleij
Re: [PATCH v3 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
Quoted 25 lines · Linus Walleij
Hi Dan,
Quoted 21 lines · Dan O'Donovan
The UP2 board features a Raspberry Pi compatible pin header (HAT) and a board-specific expansion connector (EXHAT). Both expose assorted functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other on-board devices (ADC, FPGA IP blocks...).
These lines are routed through an on-board FPGA. The platform controller in its stock firmware provides register fields to change:
- Line enable (FPGA pins enabled / high impedance)
- Line direction (SoC driven / FPGA driven)To enable using SoC GPIOs on the pin header, this arrangement requires both configuring the platform controller, and updating the SoC pad registers in sync.
Add a frontend pinctrl/GPIO driver that registers a new set of GPIO lines for the header pins. When these are requested, the driver propagates this request to the backend SoC pinctrl/GPIO driver by grabbing a GPIO descriptor for the matching SoC GPIO line. The needed mapping for this is retrieved via ACPI properties.
Quoted 3 lines
Quoted 1 line
Signed-off-by Dan O'Donovan <[email protected]>It appears you missed my review comments so please read them and reply or respin the patch accordingly.
Hi Linus. Unfortunately I did miss your comments completely. Somehow, they weren't in my Inbox, but I found them in the online archives now. Thanks for letting me know. I'll reply to those soon.
Quoted 3 lines
Yours,
Linus Walleij