Introduce the Surface Extensible Boot Loader driver for the Surface Duo.
Exposes information about the driver to user space via sysfs.
Signed-off-by: Jarrett Schultz <[email protected]>
---
Changes in v2:
- Per Maximilian, added patch 2: propagated ACPI dependency from the
directory as a whole to each individual driver
- For the yaml documentation:
* Removed json-schema dependence
* Elaborated on description of driver
* Updated example
- Changed target KernelVersion in sysfs documentation
- Updated MAINTAINER changes to be properly applied across patches
- For the driver itself,
* Added types.h inclusion and removed unused inclusions
* Minor updates to code and acronym style
* Remove __packed attribute on driver struct
* Use .dev_groups for sysfs
- Added more in-depth description of driver in Kconfig
- Modified dts to reference a newly added section in sm8150.dtsi
---
Jarrett Schultz (5):
dt-bindings: platform: microsoft: Document surface xbl
platform: surface: Propogate ACPI Dependency
platform: surface: Add surface xbl
arm64: dts: qcom: sm8150: Add imem section
arm64: dts: qcom: surface-duo: Add surface xbl
.../ABI/testing/sysfs-platform-surface-xbl | 78 +++++++
.../platform/microsoft/surface-xbl.yaml | 57 +++++
MAINTAINERS | 9 +
.../dts/qcom/sm8150-microsoft-surface-duo.dts | 10 +
arch/arm64/boot/dts/qcom/sm8150.dtsi | 8 +
drivers/platform/surface/Kconfig | 24 +-
drivers/platform/surface/Makefile | 1 +
drivers/platform/surface/surface-xbl.c | 215 ++++++++++++++++++
8 files changed, 401 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
create mode 100644 drivers/platform/surface/surface-xbl.c
--
2.25.1
Introduce the imem section in preparation for the surface xbl driver.
Signed-off-by: Jarrett Schultz <[email protected]>
---
Changes in v2:
- Created to properly reference the xbl section inside of imem
---
arch/arm64/boot/dts/qcom/sm8150.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index ef0232c2cf45..1da327cd49ae 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -1176,6 +1176,14 @@ gpi_dma1: dma-controller@a00000 {
status = "disabled";
};
+ imem: imem@146bf000 {
+ compatible = "simple-mfd";
+ reg = <0x0 0x146bf000 0x0 0x1000>;
+ ranges = <0x0 0x0 0x146bf000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ };
+
qupv3_id_1: geniqup@ac0000 {
compatible = "qcom,geni-se-qup";
reg = <0x0 0x00ac0000 0x0 0x6000>;
--
2.25.1
From: Jarrett Schultz <[email protected]>
Since the Surface XBL Driver does not depend on ACPI, the
platform/surface directory as a whole no longer depends on ACPI. With
respect to this, the ACPI dependency is moved into each config that
depends on ACPI individually.
Signed-off-by: Jarrett Schultz <[email protected]>
---
Changes in v2:
- Created to propagate ACPI dependency
---
drivers/platform/surface/Kconfig | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index 3105f651614f..0d3970e1d144 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -5,7 +5,6 @@
menuconfig SURFACE_PLATFORMS
bool "Microsoft Surface Platform-Specific Device Drivers"
- depends on ACPI
default y
help
Say Y here to get to see options for platform-specific device drivers
@@ -18,6 +17,7 @@ if SURFACE_PLATFORMS
config SURFACE3_WMI
tristate "Surface 3 WMI Driver"
+ depends on ACPI
depends on ACPI_WMI
depends on DMI
depends on INPUT
@@ -30,12 +30,14 @@ config SURFACE3_WMI
config SURFACE_3_BUTTON
tristate "Power/home/volume buttons driver for Microsoft Surface 3 tablet"
+ depends on ACPI
depends on KEYBOARD_GPIO && I2C
help
This driver handles the power/home/volume buttons on the Microsoft Surface 3 tablet.
config SURFACE_3_POWER_OPREGION
tristate "Surface 3 battery platform operation region support"
+ depends on ACPI
depends on I2C
help
This driver provides support for ACPI operation
@@ -43,6 +45,7 @@ config SURFACE_3_POWER_OPREGION
config SURFACE_ACPI_NOTIFY
tristate "Surface ACPI Notify Driver"
+ depends on ACPI
depends on SURFACE_AGGREGATOR
help
Surface ACPI Notify (SAN) driver for Microsoft Surface devices.
@@ -62,6 +65,7 @@ config SURFACE_ACPI_NOTIFY
config SURFACE_AGGREGATOR_CDEV
tristate "Surface System Aggregator Module User-Space Interface"
+ depends on ACPI
depends on SURFACE_AGGREGATOR
help
Provides a misc-device interface to the Surface System Aggregator
@@ -79,6 +83,7 @@ config SURFACE_AGGREGATOR_CDEV
config SURFACE_AGGREGATOR_REGISTRY
tristate "Surface System Aggregator Module Device Registry"
+ depends on ACPI
depends on SURFACE_AGGREGATOR
depends on SURFACE_AGGREGATOR_BUS
help
@@ -106,6 +111,7 @@ config SURFACE_AGGREGATOR_REGISTRY
config SURFACE_DTX
tristate "Surface DTX (Detachment System) Driver"
+ depends on ACPI
depends on SURFACE_AGGREGATOR
depends on INPUT
help
@@ -126,6 +132,7 @@ config SURFACE_DTX
config SURFACE_GPE
tristate "Surface GPE/Lid Support Driver"
+ depends on ACPI
depends on DMI
help
This driver marks the GPEs related to the ACPI lid device found on
@@ -135,6 +142,7 @@ config SURFACE_GPE
config SURFACE_HOTPLUG
tristate "Surface Hot-Plug Driver"
+ depends on ACPI
depends on GPIOLIB
help
Driver for out-of-band hot-plug event signaling on Microsoft Surface
@@ -154,6 +162,7 @@ config SURFACE_HOTPLUG
config SURFACE_PLATFORM_PROFILE
tristate "Surface Platform Profile Driver"
+ depends on ACPI
depends on SURFACE_AGGREGATOR_REGISTRY
select ACPI_PLATFORM_PROFILE
help
@@ -176,6 +185,7 @@ config SURFACE_PLATFORM_PROFILE
config SURFACE_PRO3_BUTTON
tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
+ depends on ACPI
depends on INPUT
help
This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
--
2.25.1
Introduce device tree source for the surface xbl driver.
Signed-off-by: Jarrett Schultz <[email protected]>
---
Changes in v2:
- Updated to reference an offset inside of imem
---
.../boot/dts/qcom/sm8150-microsoft-surface-duo.dts | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts b/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
index 736da9af44e0..25a4ee05ee2b 100644
--- a/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
+++ b/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
@@ -429,6 +429,16 @@ &i2c19 {
/* MAX34417 @ 0x1e */
};
+&imem {
+ status = "okay";
+
+ xbl@a94 {
+ compatible = "microsoft,sm8150-surface-duo-xbl";
+ reg = <0xa94 0x100>;
+ status = "okay";
+ };
+};
+
&pon {
pwrkey {
status = "okay";
--
2.25.1
Introduce support for the Extensible Boot Loader driver found on the
Surface Duo. Makes device information available to users via sysfs.
Signed-off-by: Jarrett Schultz <[email protected]>
---
Changes in v2:
- Added types.h inclusion and removed unused inclusions
- Minor updates to code and acronym style
- Remove __packed attribute on driver struct
- Use .dev_groups for sysfs
- Added more in-depth description of driver in Kconfig
- Changed target KernelVersion in sysfs documentation
---
.../ABI/testing/sysfs-platform-surface-xbl | 78 +++++++
MAINTAINERS | 2 +
drivers/platform/surface/Kconfig | 12 +
drivers/platform/surface/Makefile | 1 +
drivers/platform/surface/surface-xbl.c | 215 ++++++++++++++++++
5 files changed, 308 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
create mode 100644 drivers/platform/surface/surface-xbl.c
diff --git a/Documentation/ABI/testing/sysfs-platform-surface-xbl b/Documentation/ABI/testing/sysfs-platform-surface-xbl
new file mode 100644
index 000000000000..2ae047b884d3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-surface-xbl
@@ -0,0 +1,78 @@
+What: /sys/devices/platform/146bfa94.xbl/battery_present
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns whether the battery is present. Valid
+ values are:
+ 0 - battery absent
+ 1 - battery present
+
+What: /sys/devices/platform/146bfa94.xbl/board_id
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns the board id.
+
+What: /sys/devices/platform/146bfa94.xbl/hw_init_retries
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns retries attempted to initialize the
+ discrete hardware circuit.
+
+What: /sys/devices/platform/146bfa94.xbl/is_act_mode
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns whether ACT mode is enabled. Valid values
+ are:
+ 0 - ACT disabled
+ 1 - ACT enabled
+
+ ACT mode is used to run checks and put the device to shipmode
+ at factory.
+
+What: /sys/devices/platform/146bfa94.xbl/is_customer_mode
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns whether the device is in manufacturing
+ mode. Valid values are:
+ 0 - Not in manufacturing mode
+ 1 - In manufacturing mode
+
+What: /sys/devices/platform/146bfa94.xbl/ocp_error_location
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns 0 or which power rail has the OCP error.
+ Valid values are:
+ Bit(s) Meaning
+ 15 More than one OCP error occurred
+ 14-12 PMIC
+ 11-7 SMPS
+ 6-2 LDO
+ 1-0 BOB
+
+What: /sys/devices/platform/146bfa94.xbl/pmic_reset_reason
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns the reason for the reset. Valid values
+ are:
+ 0 - no reason lol
+ 9 - Battery driver triggered
+
+What: /sys/devices/platform/146bfa94.xbl/touch_fw_version
+Date: October 2021
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read only. It returns the version of the firmware.
diff --git a/MAINTAINERS b/MAINTAINERS
index 8643546f8fab..d08b68d626f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12428,7 +12428,9 @@ M: Jarrett Schultz <[email protected]>
L: [email protected]
L: [email protected]
S: Supported
+F: Documentation/ABI/testing/sysfs-platform-surface-xbl
F: Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
+F: drivers/platform/surface/surface-xbl.c
MICROSOFT SURFACE GPE LID SUPPORT DRIVER
M: Maximilian Luz <[email protected]>
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index 0d3970e1d144..3a1ced269d96 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -190,6 +190,18 @@ config SURFACE_PRO3_BUTTON
help
This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
+config SURFACE_XBL
+ tristate "Surface XBL Driver"
+ depends on ARM64 || COMPILE_TEST
+ depends on OF
+ help
+ If you say 'Y' to this option, support will be included for the
+ Surface Extensible Boot Loader (XBL) Driver. This driver exposes
+ information about the device through sysfs.
+
+ This driver can also be built as a module. If so, the module
+ will be called surface-xbl.
+
source "drivers/platform/surface/aggregator/Kconfig"
endif # SURFACE_PLATFORMS
diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
index 32889482de55..0946266a8a73 100644
--- a/drivers/platform/surface/Makefile
+++ b/drivers/platform/surface/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o
obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o
obj-$(CONFIG_SURFACE_PLATFORM_PROFILE) += surface_platform_profile.o
obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
+obj-$(CONFIG_SURFACE_XBL) += surface-xbl.o
diff --git a/drivers/platform/surface/surface-xbl.c b/drivers/platform/surface/surface-xbl.c
new file mode 100644
index 000000000000..9ecec4e55a4d
--- /dev/null
+++ b/drivers/platform/surface/surface-xbl.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Surface eXtensible Boot Loader (XBL)
+ *
+ * Copyright (C) 2021 Microsoft Corporation
+ * Author: Jarrett Schultz <[email protected]>
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#define SURFACE_XBL_MAX_VERSION_LEN 16
+#define SURFACE_XBL_BOARD_ID 0
+#define SURFACE_XBL_BATTERY_PRESENT 1
+#define SURFACE_XBL_HW_INIT_RETRIES 2
+#define SURFACE_XBL_IS_CUSTOMER_MODE 3
+#define SURFACE_XBL_IS_ACT_MODE 4
+#define SURFACE_XBL_PMIC_RESET_REASON 5
+#define SURFACE_XBL_TOUCH_FW_VERSION 6
+#define SURFACE_XBL_OCP_ERROR_LOCATION \
+ (SURFACE_XBL_TOUCH_FW_VERSION + \
+ SURFACE_XBL_MAX_VERSION_LEN)
+
+struct surface_xbl {
+ struct device *dev;
+ void __iomem *regs;
+
+ u8 board_id;
+ u8 battery_present;
+ u8 hw_init_retries;
+ u8 is_customer_mode;
+ u8 is_act_mode;
+ u8 pmic_reset_reason;
+ char touch_fw_version[SURFACE_XBL_MAX_VERSION_LEN];
+ u16 ocp_error_location;
+};
+
+static ssize_t
+board_id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->board_id);
+}
+static DEVICE_ATTR_RO(board_id);
+
+static ssize_t
+battery_present_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->battery_present);
+}
+static DEVICE_ATTR_RO(battery_present);
+
+static ssize_t
+hw_init_retries_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->hw_init_retries);
+}
+static DEVICE_ATTR_RO(hw_init_retries);
+
+static ssize_t
+is_customer_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->is_customer_mode);
+}
+static DEVICE_ATTR_RO(is_customer_mode);
+
+static ssize_t
+is_act_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->is_act_mode);
+}
+static DEVICE_ATTR_RO(is_act_mode);
+
+static ssize_t
+pmic_reset_reason_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->pmic_reset_reason);
+}
+static DEVICE_ATTR_RO(pmic_reset_reason);
+
+static ssize_t
+touch_fw_version_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "0x%s\n", sxbl->touch_fw_version);
+}
+static DEVICE_ATTR_RO(touch_fw_version);
+
+static ssize_t
+ocp_error_location_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct surface_xbl *sxbl = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", sxbl->ocp_error_location);
+}
+static DEVICE_ATTR_RO(ocp_error_location);
+
+static struct attribute *xbl_attrs[] = {
+ &dev_attr_board_id.attr,
+ &dev_attr_battery_present.attr,
+ &dev_attr_hw_init_retries.attr,
+ &dev_attr_is_customer_mode.attr,
+ &dev_attr_is_act_mode.attr,
+ &dev_attr_pmic_reset_reason.attr,
+ &dev_attr_touch_fw_version.attr,
+ &dev_attr_ocp_error_location.attr,
+ NULL
+};
+
+static const struct attribute_group xbl_attr_group = {
+ .attrs = xbl_attrs,
+};
+
+const struct attribute_group *xbl_sysfs_groups[] = {
+ &xbl_attr_group,
+ NULL
+};
+
+static u8 surface_xbl_readb(void __iomem *base, u32 offset)
+{
+ return readb(base + offset);
+}
+
+static u16 surface_xbl_readw(void __iomem *base, u32 offset)
+{
+ return readw(base + offset);
+}
+
+static int surface_xbl_probe(struct platform_device *pdev)
+{
+ struct surface_xbl *sxbl;
+ struct device *dev;
+ void __iomem *regs;
+ int index;
+
+ dev = &pdev->dev;
+ sxbl = devm_kzalloc(dev, sizeof(*sxbl), GFP_KERNEL);
+ if (!sxbl)
+ return -ENOMEM;
+
+ sxbl->dev = dev;
+
+ regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+
+ sxbl->regs = regs;
+
+ platform_set_drvdata(pdev, sxbl);
+
+ sxbl->board_id = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_BOARD_ID);
+ sxbl->battery_present = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_BATTERY_PRESENT);
+ sxbl->hw_init_retries = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_HW_INIT_RETRIES);
+ sxbl->is_customer_mode = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_IS_CUSTOMER_MODE);
+ sxbl->is_act_mode = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_IS_ACT_MODE);
+ sxbl->pmic_reset_reason = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_PMIC_RESET_REASON);
+
+ for (index = 0; index < SURFACE_XBL_MAX_VERSION_LEN; index++)
+ sxbl->touch_fw_version[index] = surface_xbl_readb(sxbl->regs,
+ SURFACE_XBL_TOUCH_FW_VERSION + index);
+
+ sxbl->ocp_error_location = surface_xbl_readw(sxbl->regs,
+ SURFACE_XBL_OCP_ERROR_LOCATION);
+
+ return 0;
+}
+
+static int surface_xbl_remove(struct platform_device *pdev)
+{
+ return 0;
+}
+
+static const struct of_device_id surface_xbl_of_match[] = {
+ {
+ .compatible = "microsoft,sm8150-surface-duo-xbl"
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
+
+static struct platform_driver surface_xbl_driver = {
+ .driver = {
+ .name = "surface-xbl",
+ .of_match_table = surface_xbl_of_match,
+ .dev_groups = xbl_sysfs_groups
+ },
+ .probe = surface_xbl_probe,
+ .remove = surface_xbl_remove
+};
+module_platform_driver(surface_xbl_driver);
+
+MODULE_AUTHOR("Jarrett Schultz <[email protected]>");
+MODULE_DESCRIPTION("Surface Extensible Bootloader");
+MODULE_LICENSE("GPL");
--
2.25.1
On Mon 08 Nov 08:44 PST 2021, Jarrett Schultz wrote:
> Introduce support for the Extensible Boot Loader driver found on the
> Surface Duo. Makes device information available to users via sysfs.
>
> Signed-off-by: Jarrett Schultz <[email protected]>
>
> ---
>
> Changes in v2:
> - Added types.h inclusion and removed unused inclusions
> - Minor updates to code and acronym style
> - Remove __packed attribute on driver struct
> - Use .dev_groups for sysfs
> - Added more in-depth description of driver in Kconfig
> - Changed target KernelVersion in sysfs documentation
>
> ---
>
> .../ABI/testing/sysfs-platform-surface-xbl | 78 +++++++
> MAINTAINERS | 2 +
> drivers/platform/surface/Kconfig | 12 +
> drivers/platform/surface/Makefile | 1 +
> drivers/platform/surface/surface-xbl.c | 215 ++++++++++++++++++
> 5 files changed, 308 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
> create mode 100644 drivers/platform/surface/surface-xbl.c
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-surface-xbl b/Documentation/ABI/testing/sysfs-platform-surface-xbl
> new file mode 100644
> index 000000000000..2ae047b884d3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-surface-xbl
> @@ -0,0 +1,78 @@
> +What: /sys/devices/platform/146bfa94.xbl/battery_present
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns whether the battery is present. Valid
> + values are:
> + 0 - battery absent
> + 1 - battery present
Would this information not be available from some battery driver, under
/sys/class/power_supply?
> +
> +What: /sys/devices/platform/146bfa94.xbl/board_id
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns the board id.
Is this a Microsoft-specific board id, or does it relate to the Qualcomm
socinfo property with the same name?
> +
> +What: /sys/devices/platform/146bfa94.xbl/hw_init_retries
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns retries attempted to initialize the
> + discrete hardware circuit.
Which description hardware circuit?
> +
> +What: /sys/devices/platform/146bfa94.xbl/is_act_mode
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns whether ACT mode is enabled. Valid values
> + are:
> + 0 - ACT disabled
> + 1 - ACT enabled
> +
> + ACT mode is used to run checks and put the device to shipmode
> + at factory.
> +
> +What: /sys/devices/platform/146bfa94.xbl/is_customer_mode
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns whether the device is in manufacturing
> + mode. Valid values are:
> + 0 - Not in manufacturing mode
> + 1 - In manufacturing mode
> +
> +What: /sys/devices/platform/146bfa94.xbl/ocp_error_location
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns 0 or which power rail has the OCP error.
Sounds like the reason is singular, so why is this a bitmask?
> + Valid values are:
> + Bit(s) Meaning
> + 15 More than one OCP error occurred
> + 14-12 PMIC
> + 11-7 SMPS
> + 6-2 LDO
> + 1-0 BOB
> +
> +What: /sys/devices/platform/146bfa94.xbl/pmic_reset_reason
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns the reason for the reset. Valid values
> + are:
Is this different from the PMIC reset reason that we read from the PMIC?
Could we make sure to expose this generically for all Qualcomm PMICs?
> + 0 - no reason lol
> + 9 - Battery driver triggered
> +
> +What: /sys/devices/platform/146bfa94.xbl/touch_fw_version
> +Date: October 2021
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read only. It returns the version of the firmware.
Why isn't this exposed by the touchscreen driver instead?
Generally I wonder how you're consuming this information in userspace.
Is it only for debugging purposes, i.e. would debugfs be a better place?
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8643546f8fab..d08b68d626f6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12428,7 +12428,9 @@ M: Jarrett Schultz <[email protected]>
> L: [email protected]
> L: [email protected]
> S: Supported
> +F: Documentation/ABI/testing/sysfs-platform-surface-xbl
> F: Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> +F: drivers/platform/surface/surface-xbl.c
>
> MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> M: Maximilian Luz <[email protected]>
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 0d3970e1d144..3a1ced269d96 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -190,6 +190,18 @@ config SURFACE_PRO3_BUTTON
> help
> This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
>
> +config SURFACE_XBL
> + tristate "Surface XBL Driver"
> + depends on ARM64 || COMPILE_TEST
> + depends on OF
> + help
> + If you say 'Y' to this option, support will be included for the
> + Surface Extensible Boot Loader (XBL) Driver. This driver exposes
> + information about the device through sysfs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called surface-xbl.
> +
> source "drivers/platform/surface/aggregator/Kconfig"
>
> endif # SURFACE_PLATFORMS
> diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
> index 32889482de55..0946266a8a73 100644
> --- a/drivers/platform/surface/Makefile
> +++ b/drivers/platform/surface/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o
> obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o
> obj-$(CONFIG_SURFACE_PLATFORM_PROFILE) += surface_platform_profile.o
> obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
> +obj-$(CONFIG_SURFACE_XBL) += surface-xbl.o
All other files in this directory are named with an underscore, would be
nice to carry on with such convention.
> diff --git a/drivers/platform/surface/surface-xbl.c b/drivers/platform/surface/surface-xbl.c
> new file mode 100644
> index 000000000000..9ecec4e55a4d
> --- /dev/null
> +++ b/drivers/platform/surface/surface-xbl.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Surface eXtensible Boot Loader (XBL)
> + *
> + * Copyright (C) 2021 Microsoft Corporation
> + * Author: Jarrett Schultz <[email protected]>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#define SURFACE_XBL_MAX_VERSION_LEN 16
> +#define SURFACE_XBL_BOARD_ID 0
> +#define SURFACE_XBL_BATTERY_PRESENT 1
> +#define SURFACE_XBL_HW_INIT_RETRIES 2
> +#define SURFACE_XBL_IS_CUSTOMER_MODE 3
> +#define SURFACE_XBL_IS_ACT_MODE 4
> +#define SURFACE_XBL_PMIC_RESET_REASON 5
> +#define SURFACE_XBL_TOUCH_FW_VERSION 6
> +#define SURFACE_XBL_OCP_ERROR_LOCATION \
> + (SURFACE_XBL_TOUCH_FW_VERSION + \
> + SURFACE_XBL_MAX_VERSION_LEN)
> +
> +struct surface_xbl {
> + struct device *dev;
> + void __iomem *regs;
> +
> + u8 board_id;
> + u8 battery_present;
> + u8 hw_init_retries;
> + u8 is_customer_mode;
> + u8 is_act_mode;
> + u8 pmic_reset_reason;
> + char touch_fw_version[SURFACE_XBL_MAX_VERSION_LEN];
> + u16 ocp_error_location;
> +};
> +
> +static ssize_t
> +board_id_show(struct device *dev, struct device_attribute *attr, char *buf)
I think it would be nice to avoid some duplication by putting all these
integer ones in a single show(), see e.g. soc_info_show()
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->board_id);
> +}
> +static DEVICE_ATTR_RO(board_id);
> +
> +static ssize_t
> +battery_present_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->battery_present);
> +}
> +static DEVICE_ATTR_RO(battery_present);
> +
> +static ssize_t
> +hw_init_retries_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->hw_init_retries);
> +}
> +static DEVICE_ATTR_RO(hw_init_retries);
> +
> +static ssize_t
> +is_customer_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->is_customer_mode);
> +}
> +static DEVICE_ATTR_RO(is_customer_mode);
> +
> +static ssize_t
> +is_act_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->is_act_mode);
> +}
> +static DEVICE_ATTR_RO(is_act_mode);
> +
> +static ssize_t
> +pmic_reset_reason_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->pmic_reset_reason);
> +}
> +static DEVICE_ATTR_RO(pmic_reset_reason);
> +
> +static ssize_t
> +touch_fw_version_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "0x%s\n", sxbl->touch_fw_version);
> +}
> +static DEVICE_ATTR_RO(touch_fw_version);
> +
> +static ssize_t
> +ocp_error_location_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", sxbl->ocp_error_location);
> +}
> +static DEVICE_ATTR_RO(ocp_error_location);
> +
> +static struct attribute *xbl_attrs[] = {
> + &dev_attr_board_id.attr,
> + &dev_attr_battery_present.attr,
> + &dev_attr_hw_init_retries.attr,
> + &dev_attr_is_customer_mode.attr,
> + &dev_attr_is_act_mode.attr,
> + &dev_attr_pmic_reset_reason.attr,
> + &dev_attr_touch_fw_version.attr,
> + &dev_attr_ocp_error_location.attr,
> + NULL
> +};
> +
> +static const struct attribute_group xbl_attr_group = {
> + .attrs = xbl_attrs,
> +};
> +
> +const struct attribute_group *xbl_sysfs_groups[] = {
> + &xbl_attr_group,
> + NULL
> +};
> +
> +static u8 surface_xbl_readb(void __iomem *base, u32 offset)
> +{
> + return readb(base + offset);
Instead of having these helpers I think you should just call readb(base
+ offset) directly below.
The shorter function name (readb vs surface_xbl_readb) means that you
don't even need to line wrap those lines.
> +}
> +
> +static u16 surface_xbl_readw(void __iomem *base, u32 offset)
> +{
> + return readw(base + offset);
> +}
> +
> +static int surface_xbl_probe(struct platform_device *pdev)
> +{
> + struct surface_xbl *sxbl;
> + struct device *dev;
> + void __iomem *regs;
> + int index;
> +
> + dev = &pdev->dev;
> + sxbl = devm_kzalloc(dev, sizeof(*sxbl), GFP_KERNEL);
This is the only use of &pdev->dev, so put that here and drop "dev" from
sxbl (and the stack).
> + if (!sxbl)
> + return -ENOMEM;
> +
> + sxbl->dev = dev;
> +
> + regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + sxbl->regs = regs;
Seems only reason you stash "regs" in sxbl is so that you can pass
sxbl->regs in below function calls. I.e. it's a local variable and you
can omit it from struct surface_xbl...
> +
> + platform_set_drvdata(pdev, sxbl);
> +
> + sxbl->board_id = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_BOARD_ID);
> + sxbl->battery_present = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_BATTERY_PRESENT);
> + sxbl->hw_init_retries = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_HW_INIT_RETRIES);
> + sxbl->is_customer_mode = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_IS_CUSTOMER_MODE);
> + sxbl->is_act_mode = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_IS_ACT_MODE);
> + sxbl->pmic_reset_reason = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_PMIC_RESET_REASON);
> +
> + for (index = 0; index < SURFACE_XBL_MAX_VERSION_LEN; index++)
"i" is a good succinct variable name for an index.
> + sxbl->touch_fw_version[index] = surface_xbl_readb(sxbl->regs,
> + SURFACE_XBL_TOUCH_FW_VERSION + index);
> +
> + sxbl->ocp_error_location = surface_xbl_readw(sxbl->regs,
> + SURFACE_XBL_OCP_ERROR_LOCATION);
> +
> + return 0;
> +}
> +
> +static int surface_xbl_remove(struct platform_device *pdev)
Your remove function is empty, simply omit it...
Regards,
Bjorn
> +{
> + return 0;
> +}
> +
> +static const struct of_device_id surface_xbl_of_match[] = {
> + {
> + .compatible = "microsoft,sm8150-surface-duo-xbl"
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
> +
> +static struct platform_driver surface_xbl_driver = {
> + .driver = {
> + .name = "surface-xbl",
> + .of_match_table = surface_xbl_of_match,
> + .dev_groups = xbl_sysfs_groups
> + },
> + .probe = surface_xbl_probe,
> + .remove = surface_xbl_remove
> +};
> +module_platform_driver(surface_xbl_driver);
> +
> +MODULE_AUTHOR("Jarrett Schultz <[email protected]>");
> +MODULE_DESCRIPTION("Surface Extensible Bootloader");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
On Mon 08 Nov 08:44 PST 2021, Jarrett Schultz wrote:
> Introduce the imem section in preparation for the surface xbl driver.
>
> Signed-off-by: Jarrett Schultz <[email protected]>
>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
>
> Changes in v2:
> - Created to properly reference the xbl section inside of imem
>
> ---
>
> arch/arm64/boot/dts/qcom/sm8150.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index ef0232c2cf45..1da327cd49ae 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -1176,6 +1176,14 @@ gpi_dma1: dma-controller@a00000 {
> status = "disabled";
> };
>
> + imem: imem@146bf000 {
> + compatible = "simple-mfd";
> + reg = <0x0 0x146bf000 0x0 0x1000>;
> + ranges = <0x0 0x0 0x146bf000 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + };
> +
> qupv3_id_1: geniqup@ac0000 {
> compatible = "qcom,geni-se-qup";
> reg = <0x0 0x00ac0000 0x0 0x6000>;
> --
> 2.25.1
>
On 11/8/21 17:44, Jarrett Schultz wrote:> From: Jarrett Schultz <[email protected]>
>
> Since the Surface XBL Driver does not depend on ACPI, the
> platform/surface directory as a whole no longer depends on ACPI. With
> respect to this, the ACPI dependency is moved into each config that
> depends on ACPI individually.
>
> Signed-off-by: Jarrett Schultz <[email protected]>
Some remarks inline:
> ---
>
> Changes in v2:
> - Created to propagate ACPI dependency
>
> ---
>
> drivers/platform/surface/Kconfig | 12 +++++++++++-
You also need to account for included Kconfigs, specifically:
drivers/platform/surface/aggregator/Kconfig.
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 3105f651614f..0d3970e1d144 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -5,7 +5,6 @@
>
> menuconfig SURFACE_PLATFORMS
> bool "Microsoft Surface Platform-Specific Device Drivers"
> - depends on ACPI
> default y
> help
> Say Y here to get to see options for platform-specific device drivers
> @@ -18,6 +17,7 @@ if SURFACE_PLATFORMS
>
> config SURFACE3_WMI
> tristate "Surface 3 WMI Driver"
> + depends on ACPI
This is redundant, you can drop that. ACPI_WMI already depends on ACPI.
> depends on ACPI_WMI
> depends on DMI
> depends on INPUT
> @@ -30,12 +30,14 @@ config SURFACE3_WMI
>
> config SURFACE_3_BUTTON
> tristate "Power/home/volume buttons driver for Microsoft Surface 3 tablet"
> + depends on ACPI
> depends on KEYBOARD_GPIO && I2C
> help
> This driver handles the power/home/volume buttons on the Microsoft Surface 3 tablet.
>
> config SURFACE_3_POWER_OPREGION
> tristate "Surface 3 battery platform operation region support"
> + depends on ACPI
> depends on I2C
> help
> This driver provides support for ACPI operation
> @@ -43,6 +45,7 @@ config SURFACE_3_POWER_OPREGION
>
> config SURFACE_ACPI_NOTIFY
> tristate "Surface ACPI Notify Driver"
> + depends on ACPI
As mentioned above, you're missing aggregator/Kconfig. All you need to
do is add "depends on ACPI" to SURFACE_AGGREGATOR in that file. Then you
can drop the "depends on ACPI" for anything that depends on that.
Same holds for the couple of options depending on SURFACE_AGGREGATOR
below.
> depends on SURFACE_AGGREGATOR
> help
> Surface ACPI Notify (SAN) driver for Microsoft Surface devices.
> @@ -62,6 +65,7 @@ config SURFACE_ACPI_NOTIFY
>
> config SURFACE_AGGREGATOR_CDEV
> tristate "Surface System Aggregator Module User-Space Interface"
> + depends on ACPI
> depends on SURFACE_AGGREGATOR
> help
> Provides a misc-device interface to the Surface System Aggregator
> @@ -79,6 +83,7 @@ config SURFACE_AGGREGATOR_CDEV
>
> config SURFACE_AGGREGATOR_REGISTRY
> tristate "Surface System Aggregator Module Device Registry"
> + depends on ACPI
> depends on SURFACE_AGGREGATOR
> depends on SURFACE_AGGREGATOR_BUS
> help
> @@ -106,6 +111,7 @@ config SURFACE_AGGREGATOR_REGISTRY
>
> config SURFACE_DTX
> tristate "Surface DTX (Detachment System) Driver"
> + depends on ACPI
> depends on SURFACE_AGGREGATOR
> depends on INPUT
> help
> @@ -126,6 +132,7 @@ config SURFACE_DTX
>
> config SURFACE_GPE
> tristate "Surface GPE/Lid Support Driver"
> + depends on ACPI
> depends on DMI
> help
> This driver marks the GPEs related to the ACPI lid device found on
> @@ -135,6 +142,7 @@ config SURFACE_GPE
>
> config SURFACE_HOTPLUG
> tristate "Surface Hot-Plug Driver"
> + depends on ACPI
> depends on GPIOLIB
> help
> Driver for out-of-band hot-plug event signaling on Microsoft Surface
> @@ -154,6 +162,7 @@ config SURFACE_HOTPLUG
>
> config SURFACE_PLATFORM_PROFILE
> tristate "Surface Platform Profile Driver"
> + depends on ACPI
> depends on SURFACE_AGGREGATOR_REGISTRY
> select ACPI_PLATFORM_PROFILE
> help
> @@ -176,6 +185,7 @@ config SURFACE_PLATFORM_PROFILE
>
> config SURFACE_PRO3_BUTTON
> tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
> + depends on ACPI
> depends on INPUT
> help
> This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
>
Hi Jarrett,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on platform-drivers-x86/for-next linus/master v5.15 next-20211111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jarrett-Schultz/platform-surface-Introduce-Surface-XBL-Driver/20211109-004605
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6cc91cd949ff1d32a3f6b323d055b1925627be02
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jarrett-Schultz/platform-surface-Introduce-Surface-XBL-Driver/20211109-004605
git checkout 6cc91cd949ff1d32a3f6b323d055b1925627be02
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/platform/surface/aggregator/core.c: In function 'ssam_serial_hub_probe':
>> drivers/platform/surface/aggregator/core.c:648:49: error: invalid use of undefined type 'struct acpi_device'
648 | astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
| ^~
>> drivers/platform/surface/aggregator/core.c:702:9: error: implicit declaration of function 'acpi_dev_clear_dependencies' [-Werror=implicit-function-declaration]
702 | acpi_dev_clear_dependencies(ssh);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/perf_event.h:25,
from include/linux/trace_events.h:10,
from include/trace/trace_events.h:21,
from include/trace/define_trace.h:102,
from drivers/platform/surface/aggregator/trace.h:632,
from drivers/platform/surface/aggregator/core.c:30:
At top level:
arch/arc/include/asm/perf_event.h:126:27: error: 'arc_pmu_cache_map' defined but not used [-Werror=unused-const-variable=]
126 | static const unsigned int arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
| ^~~~~~~~~~~~~~~~~
arch/arc/include/asm/perf_event.h:91:27: error: 'arc_pmu_ev_hw_map' defined but not used [-Werror=unused-const-variable=]
91 | static const char * const arc_pmu_ev_hw_map[] = {
| ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
drivers/platform/surface/aggregator/controller.c: In function 'ssam_dsm_get_functions':
>> drivers/platform/surface/aggregator/controller.c:1044:14: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Werror=implicit-function-declaration]
1044 | if (!acpi_has_method(handle, "_DSM"))
| ^~~~~~~~~~~~~~~
| acpi_has_watchdog
>> drivers/platform/surface/aggregator/controller.c:1047:15: error: implicit declaration of function 'acpi_evaluate_dsm_typed'; did you mean 'acpi_evaluate_dsm'? [-Werror=implicit-function-declaration]
1047 | obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
| ^~~~~~~~~~~~~~~~~~~~~~~
| acpi_evaluate_dsm
>> drivers/platform/surface/aggregator/controller.c:1047:13: error: assignment to 'union acpi_object *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
1047 | obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
| ^
drivers/platform/surface/aggregator/controller.c: In function 'ssam_dsm_load_u32':
drivers/platform/surface/aggregator/controller.c:1071:13: error: assignment to 'union acpi_object *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
1071 | obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
| ^
cc1: all warnings being treated as errors
vim +648 drivers/platform/surface/aggregator/core.c
c167b9c7e3d613 Maximilian Luz 2020-12-21 614
c167b9c7e3d613 Maximilian Luz 2020-12-21 615 static int ssam_serial_hub_probe(struct serdev_device *serdev)
c167b9c7e3d613 Maximilian Luz 2020-12-21 616 {
a9e10e58730432 Daniel Scally 2021-06-03 617 struct acpi_device *ssh = ACPI_COMPANION(&serdev->dev);
c167b9c7e3d613 Maximilian Luz 2020-12-21 618 struct ssam_controller *ctrl;
c167b9c7e3d613 Maximilian Luz 2020-12-21 619 acpi_status astatus;
c167b9c7e3d613 Maximilian Luz 2020-12-21 620 int status;
c167b9c7e3d613 Maximilian Luz 2020-12-21 621
c167b9c7e3d613 Maximilian Luz 2020-12-21 622 if (gpiod_count(&serdev->dev, NULL) < 0)
c167b9c7e3d613 Maximilian Luz 2020-12-21 623 return -ENODEV;
c167b9c7e3d613 Maximilian Luz 2020-12-21 624
c167b9c7e3d613 Maximilian Luz 2020-12-21 625 status = devm_acpi_dev_add_driver_gpios(&serdev->dev, ssam_acpi_gpios);
c167b9c7e3d613 Maximilian Luz 2020-12-21 626 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 627 return status;
c167b9c7e3d613 Maximilian Luz 2020-12-21 628
c167b9c7e3d613 Maximilian Luz 2020-12-21 629 /* Allocate controller. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 630 ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
c167b9c7e3d613 Maximilian Luz 2020-12-21 631 if (!ctrl)
c167b9c7e3d613 Maximilian Luz 2020-12-21 632 return -ENOMEM;
c167b9c7e3d613 Maximilian Luz 2020-12-21 633
c167b9c7e3d613 Maximilian Luz 2020-12-21 634 /* Initialize controller. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 635 status = ssam_controller_init(ctrl, serdev);
c167b9c7e3d613 Maximilian Luz 2020-12-21 636 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 637 goto err_ctrl_init;
c167b9c7e3d613 Maximilian Luz 2020-12-21 638
c167b9c7e3d613 Maximilian Luz 2020-12-21 639 ssam_controller_lock(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 640
c167b9c7e3d613 Maximilian Luz 2020-12-21 641 /* Set up serdev device. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 642 serdev_device_set_drvdata(serdev, ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 643 serdev_device_set_client_ops(serdev, &ssam_serdev_ops);
c167b9c7e3d613 Maximilian Luz 2020-12-21 644 status = serdev_device_open(serdev);
c167b9c7e3d613 Maximilian Luz 2020-12-21 645 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 646 goto err_devopen;
c167b9c7e3d613 Maximilian Luz 2020-12-21 647
a9e10e58730432 Daniel Scally 2021-06-03 @648 astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
c167b9c7e3d613 Maximilian Luz 2020-12-21 649 if (ACPI_FAILURE(astatus)) {
c167b9c7e3d613 Maximilian Luz 2020-12-21 650 status = -ENXIO;
c167b9c7e3d613 Maximilian Luz 2020-12-21 651 goto err_devinit;
c167b9c7e3d613 Maximilian Luz 2020-12-21 652 }
c167b9c7e3d613 Maximilian Luz 2020-12-21 653
c167b9c7e3d613 Maximilian Luz 2020-12-21 654 /* Start controller. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 655 status = ssam_controller_start(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 656 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 657 goto err_devinit;
c167b9c7e3d613 Maximilian Luz 2020-12-21 658
c167b9c7e3d613 Maximilian Luz 2020-12-21 659 ssam_controller_unlock(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 660
c167b9c7e3d613 Maximilian Luz 2020-12-21 661 /*
c167b9c7e3d613 Maximilian Luz 2020-12-21 662 * Initial SAM requests: Log version and notify default/init power
c167b9c7e3d613 Maximilian Luz 2020-12-21 663 * states.
c167b9c7e3d613 Maximilian Luz 2020-12-21 664 */
c167b9c7e3d613 Maximilian Luz 2020-12-21 665 status = ssam_log_firmware_version(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 666 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 667 goto err_initrq;
c167b9c7e3d613 Maximilian Luz 2020-12-21 668
c167b9c7e3d613 Maximilian Luz 2020-12-21 669 status = ssam_ctrl_notif_d0_entry(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 670 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 671 goto err_initrq;
c167b9c7e3d613 Maximilian Luz 2020-12-21 672
c167b9c7e3d613 Maximilian Luz 2020-12-21 673 status = ssam_ctrl_notif_display_on(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 674 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 675 goto err_initrq;
c167b9c7e3d613 Maximilian Luz 2020-12-21 676
c167b9c7e3d613 Maximilian Luz 2020-12-21 677 status = sysfs_create_group(&serdev->dev.kobj, &ssam_sam_group);
c167b9c7e3d613 Maximilian Luz 2020-12-21 678 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 679 goto err_initrq;
c167b9c7e3d613 Maximilian Luz 2020-12-21 680
c167b9c7e3d613 Maximilian Luz 2020-12-21 681 /* Set up IRQ. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 682 status = ssam_irq_setup(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 683 if (status)
c167b9c7e3d613 Maximilian Luz 2020-12-21 684 goto err_irq;
c167b9c7e3d613 Maximilian Luz 2020-12-21 685
c167b9c7e3d613 Maximilian Luz 2020-12-21 686 /* Finally, set main controller reference. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 687 status = ssam_try_set_controller(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 688 if (WARN_ON(status)) /* Currently, we're the only provider. */
c167b9c7e3d613 Maximilian Luz 2020-12-21 689 goto err_mainref;
c167b9c7e3d613 Maximilian Luz 2020-12-21 690
c167b9c7e3d613 Maximilian Luz 2020-12-21 691 /*
c167b9c7e3d613 Maximilian Luz 2020-12-21 692 * TODO: The EC can wake up the system via the associated GPIO interrupt
c167b9c7e3d613 Maximilian Luz 2020-12-21 693 * in multiple situations. One of which is the remaining battery
c167b9c7e3d613 Maximilian Luz 2020-12-21 694 * capacity falling below a certain threshold. Normally, we should
c167b9c7e3d613 Maximilian Luz 2020-12-21 695 * use the device_init_wakeup function, however, the EC also seems
c167b9c7e3d613 Maximilian Luz 2020-12-21 696 * to have other reasons for waking up the system and it seems
c167b9c7e3d613 Maximilian Luz 2020-12-21 697 * that Windows has additional checks whether the system should be
c167b9c7e3d613 Maximilian Luz 2020-12-21 698 * resumed. In short, this causes some spurious unwanted wake-ups.
c167b9c7e3d613 Maximilian Luz 2020-12-21 699 * For now let's thus default power/wakeup to false.
c167b9c7e3d613 Maximilian Luz 2020-12-21 700 */
c167b9c7e3d613 Maximilian Luz 2020-12-21 701 device_set_wakeup_capable(&serdev->dev, true);
a9e10e58730432 Daniel Scally 2021-06-03 @702 acpi_dev_clear_dependencies(ssh);
c167b9c7e3d613 Maximilian Luz 2020-12-21 703
c167b9c7e3d613 Maximilian Luz 2020-12-21 704 return 0;
c167b9c7e3d613 Maximilian Luz 2020-12-21 705
c167b9c7e3d613 Maximilian Luz 2020-12-21 706 err_mainref:
c167b9c7e3d613 Maximilian Luz 2020-12-21 707 ssam_irq_free(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 708 err_irq:
c167b9c7e3d613 Maximilian Luz 2020-12-21 709 sysfs_remove_group(&serdev->dev.kobj, &ssam_sam_group);
c167b9c7e3d613 Maximilian Luz 2020-12-21 710 err_initrq:
c167b9c7e3d613 Maximilian Luz 2020-12-21 711 ssam_controller_lock(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 712 ssam_controller_shutdown(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 713 err_devinit:
c167b9c7e3d613 Maximilian Luz 2020-12-21 714 serdev_device_close(serdev);
c167b9c7e3d613 Maximilian Luz 2020-12-21 715 err_devopen:
c167b9c7e3d613 Maximilian Luz 2020-12-21 716 ssam_controller_destroy(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 717 ssam_controller_unlock(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 718 err_ctrl_init:
c167b9c7e3d613 Maximilian Luz 2020-12-21 719 kfree(ctrl);
c167b9c7e3d613 Maximilian Luz 2020-12-21 720 return status;
c167b9c7e3d613 Maximilian Luz 2020-12-21 721 }
c167b9c7e3d613 Maximilian Luz 2020-12-21 722
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Nov 08, 2021 at 10:39:52AM -0800, Bjorn Andersson wrote:
> On Mon 08 Nov 08:44 PST 2021, Jarrett Schultz wrote:
>
> > Introduce support for the Extensible Boot Loader driver found on the
> > Surface Duo. Makes device information available to users via sysfs.
> >
> > Signed-off-by: Jarrett Schultz <[email protected]>
> >
> > ---
> >
> > Changes in v2:
> > - Added types.h inclusion and removed unused inclusions
> > - Minor updates to code and acronym style
> > - Remove __packed attribute on driver struct
> > - Use .dev_groups for sysfs
> > - Added more in-depth description of driver in Kconfig
> > - Changed target KernelVersion in sysfs documentation
> >
> > ---
> >
> > .../ABI/testing/sysfs-platform-surface-xbl | 78 +++++++
> > MAINTAINERS | 2 +
> > drivers/platform/surface/Kconfig | 12 +
> > drivers/platform/surface/Makefile | 1 +
> > drivers/platform/surface/surface-xbl.c | 215 ++++++++++++++++++
> > 5 files changed, 308 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
> > create mode 100644 drivers/platform/surface/surface-xbl.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-surface-xbl b/Documentation/ABI/testing/sysfs-platform-surface-xbl
> > new file mode 100644
> > index 000000000000..2ae047b884d3
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-surface-xbl
> > @@ -0,0 +1,78 @@
> > +What: /sys/devices/platform/146bfa94.xbl/battery_present
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns whether the battery is present. Valid
> > + values are:
> > + 0 - battery absent
> > + 1 - battery present
>
> Would this information not be available from some battery driver, under
> /sys/class/power_supply?
>
See below under touch_fw_version.
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/board_id
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns the board id.
>
> Is this a Microsoft-specific board id, or does it relate to the Qualcomm
> socinfo property with the same name?
>
Microsoft-specific board id. I will include this in the new description.
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/hw_init_retries
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns retries attempted to initialize the
> > + discrete hardware circuit.
>
> Which description hardware circuit?
>
This is a Microsoft specific value for the battery charging subsystem. I
will include this in the new description.
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/is_act_mode
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns whether ACT mode is enabled. Valid values
> > + are:
> > + 0 - ACT disabled
> > + 1 - ACT enabled
> > +
> > + ACT mode is used to run checks and put the device to shipmode
> > + at factory.
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/is_customer_mode
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns whether the device is in manufacturing
> > + mode. Valid values are:
> > + 0 - Not in manufacturing mode
> > + 1 - In manufacturing mode
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/ocp_error_location
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns 0 or which power rail has the OCP error.
>
> Sounds like the reason is singular, so why is this a bitmask?
>
There are multiple power rails so if there is an error, the value will
be the power rail which had the error.
> > + Valid values are:
> > + Bit(s) Meaning
> > + 15 More than one OCP error occurred
> > + 14-12 PMIC
> > + 11-7 SMPS
> > + 6-2 LDO
> > + 1-0 BOB
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/pmic_reset_reason
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns the reason for the reset. Valid values
> > + are:
>
> Is this different from the PMIC reset reason that we read from the PMIC?
> Could we make sure to expose this generically for all Qualcomm PMICs?
>
This is the same as the one read from PMIC. This information should be
exposed generically for all Qualcomm PMICs. Here is some information
from my colleague about the value:
- It is provided by QC (ResetRuntimeDxe driver in QComPkg)
- Protocol GUID: gEfiResetReasonProtocolGuid
- API: ReadPmicRegisterForResetReason
* Local Protocol: gQcomPmicPwrOnProtocolGuid
* Get Value: PmicPwrOnProtocol->GetSpareReg (PRIMARY_PMIC, EFI_PM_PON_SOFT_SPARE, &Value);
> > + 0 - no reason lol
> > + 9 - Battery driver triggered
> > +
> > +What: /sys/devices/platform/146bfa94.xbl/touch_fw_version
> > +Date: October 2021
> > +KernelVersion: 5.16
> > +Contact: [email protected]
> > +Description:
> > + Read only. It returns the version of the firmware.
>
> Why isn't this exposed by the touchscreen driver instead?
>
>
> Generally I wonder how you're consuming this information in userspace.
> Is it only for debugging purposes, i.e. would debugfs be a better place?
>
The information exposed through this driver is used in manufacturing
mode when producing the device.
Please let me know if you need additional information. All your other
other comments will be addressed in the next version, thank you for
your continued feedback.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8643546f8fab..d08b68d626f6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12428,7 +12428,9 @@ M: Jarrett Schultz <[email protected]>
> > L: [email protected]
> > L: [email protected]
> > S: Supported
> > +F: Documentation/ABI/testing/sysfs-platform-surface-xbl
> > F: Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> > +F: drivers/platform/surface/surface-xbl.c
> >
> > MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> > M: Maximilian Luz <[email protected]>
> > diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> > index 0d3970e1d144..3a1ced269d96 100644
> > --- a/drivers/platform/surface/Kconfig
> > +++ b/drivers/platform/surface/Kconfig
> > @@ -190,6 +190,18 @@ config SURFACE_PRO3_BUTTON
> > help
> > This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
> >
> > +config SURFACE_XBL
> > + tristate "Surface XBL Driver"
> > + depends on ARM64 || COMPILE_TEST
> > + depends on OF
> > + help
> > + If you say 'Y' to this option, support will be included for the
> > + Surface Extensible Boot Loader (XBL) Driver. This driver exposes
> > + information about the device through sysfs.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called surface-xbl.
> > +
> > source "drivers/platform/surface/aggregator/Kconfig"
> >
> > endif # SURFACE_PLATFORMS
> > diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
> > index 32889482de55..0946266a8a73 100644
> > --- a/drivers/platform/surface/Makefile
> > +++ b/drivers/platform/surface/Makefile
> > @@ -16,3 +16,4 @@ obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o
> > obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o
> > obj-$(CONFIG_SURFACE_PLATFORM_PROFILE) += surface_platform_profile.o
> > obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
> > +obj-$(CONFIG_SURFACE_XBL) += surface-xbl.o
>
> All other files in this directory are named with an underscore, would be
> nice to carry on with such convention.
>
> > diff --git a/drivers/platform/surface/surface-xbl.c b/drivers/platform/surface/surface-xbl.c
> > new file mode 100644
> > index 000000000000..9ecec4e55a4d
> > --- /dev/null
> > +++ b/drivers/platform/surface/surface-xbl.c
> > @@ -0,0 +1,215 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Surface eXtensible Boot Loader (XBL)
> > + *
> > + * Copyright (C) 2021 Microsoft Corporation
> > + * Author: Jarrett Schultz <[email protected]>
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +#define SURFACE_XBL_MAX_VERSION_LEN 16
> > +#define SURFACE_XBL_BOARD_ID 0
> > +#define SURFACE_XBL_BATTERY_PRESENT 1
> > +#define SURFACE_XBL_HW_INIT_RETRIES 2
> > +#define SURFACE_XBL_IS_CUSTOMER_MODE 3
> > +#define SURFACE_XBL_IS_ACT_MODE 4
> > +#define SURFACE_XBL_PMIC_RESET_REASON 5
> > +#define SURFACE_XBL_TOUCH_FW_VERSION 6
> > +#define SURFACE_XBL_OCP_ERROR_LOCATION \
> > + (SURFACE_XBL_TOUCH_FW_VERSION + \
> > + SURFACE_XBL_MAX_VERSION_LEN)
> > +
> > +struct surface_xbl {
> > + struct device *dev;
> > + void __iomem *regs;
> > +
> > + u8 board_id;
> > + u8 battery_present;
> > + u8 hw_init_retries;
> > + u8 is_customer_mode;
> > + u8 is_act_mode;
> > + u8 pmic_reset_reason;
> > + char touch_fw_version[SURFACE_XBL_MAX_VERSION_LEN];
> > + u16 ocp_error_location;
> > +};
> > +
> > +static ssize_t
> > +board_id_show(struct device *dev, struct device_attribute *attr, char *buf)
>
> I think it would be nice to avoid some duplication by putting all these
> integer ones in a single show(), see e.g. soc_info_show()
>
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->board_id);
> > +}
> > +static DEVICE_ATTR_RO(board_id);
> > +
> > +static ssize_t
> > +battery_present_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->battery_present);
> > +}
> > +static DEVICE_ATTR_RO(battery_present);
> > +
> > +static ssize_t
> > +hw_init_retries_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->hw_init_retries);
> > +}
> > +static DEVICE_ATTR_RO(hw_init_retries);
> > +
> > +static ssize_t
> > +is_customer_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->is_customer_mode);
> > +}
> > +static DEVICE_ATTR_RO(is_customer_mode);
> > +
> > +static ssize_t
> > +is_act_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->is_act_mode);
> > +}
> > +static DEVICE_ATTR_RO(is_act_mode);
> > +
> > +static ssize_t
> > +pmic_reset_reason_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->pmic_reset_reason);
> > +}
> > +static DEVICE_ATTR_RO(pmic_reset_reason);
> > +
> > +static ssize_t
> > +touch_fw_version_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "0x%s\n", sxbl->touch_fw_version);
> > +}
> > +static DEVICE_ATTR_RO(touch_fw_version);
> > +
> > +static ssize_t
> > +ocp_error_location_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct surface_xbl *sxbl = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%d\n", sxbl->ocp_error_location);
> > +}
> > +static DEVICE_ATTR_RO(ocp_error_location);
> > +
> > +static struct attribute *xbl_attrs[] = {
> > + &dev_attr_board_id.attr,
> > + &dev_attr_battery_present.attr,
> > + &dev_attr_hw_init_retries.attr,
> > + &dev_attr_is_customer_mode.attr,
> > + &dev_attr_is_act_mode.attr,
> > + &dev_attr_pmic_reset_reason.attr,
> > + &dev_attr_touch_fw_version.attr,
> > + &dev_attr_ocp_error_location.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group xbl_attr_group = {
> > + .attrs = xbl_attrs,
> > +};
> > +
> > +const struct attribute_group *xbl_sysfs_groups[] = {
> > + &xbl_attr_group,
> > + NULL
> > +};
> > +
> > +static u8 surface_xbl_readb(void __iomem *base, u32 offset)
> > +{
> > + return readb(base + offset);
>
> Instead of having these helpers I think you should just call readb(base
> + offset) directly below.
>
> The shorter function name (readb vs surface_xbl_readb) means that you
> don't even need to line wrap those lines.
>
> > +}
> > +
> > +static u16 surface_xbl_readw(void __iomem *base, u32 offset)
> > +{
> > + return readw(base + offset);
> > +}
> > +
> > +static int surface_xbl_probe(struct platform_device *pdev)
> > +{
> > + struct surface_xbl *sxbl;
> > + struct device *dev;
> > + void __iomem *regs;
> > + int index;
> > +
> > + dev = &pdev->dev;
> > + sxbl = devm_kzalloc(dev, sizeof(*sxbl), GFP_KERNEL);
>
> This is the only use of &pdev->dev, so put that here and drop "dev" from
> sxbl (and the stack).
>
> > + if (!sxbl)
> > + return -ENOMEM;
> > +
> > + sxbl->dev = dev;
> > +
> > + regs = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(regs))
> > + return PTR_ERR(regs);
> > +
> > + sxbl->regs = regs;
>
> Seems only reason you stash "regs" in sxbl is so that you can pass
> sxbl->regs in below function calls. I.e. it's a local variable and you
> can omit it from struct surface_xbl...
>
> > +
> > + platform_set_drvdata(pdev, sxbl);
> > +
> > + sxbl->board_id = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_BOARD_ID);
> > + sxbl->battery_present = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_BATTERY_PRESENT);
> > + sxbl->hw_init_retries = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_HW_INIT_RETRIES);
> > + sxbl->is_customer_mode = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_IS_CUSTOMER_MODE);
> > + sxbl->is_act_mode = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_IS_ACT_MODE);
> > + sxbl->pmic_reset_reason = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_PMIC_RESET_REASON);
> > +
> > + for (index = 0; index < SURFACE_XBL_MAX_VERSION_LEN; index++)
>
> "i" is a good succinct variable name for an index.
>
> > + sxbl->touch_fw_version[index] = surface_xbl_readb(sxbl->regs,
> > + SURFACE_XBL_TOUCH_FW_VERSION + index);
> > +
> > + sxbl->ocp_error_location = surface_xbl_readw(sxbl->regs,
> > + SURFACE_XBL_OCP_ERROR_LOCATION);
> > +
> > + return 0;
> > +}
> > +
> > +static int surface_xbl_remove(struct platform_device *pdev)
>
> Your remove function is empty, simply omit it...
>
> Regards,
> Bjorn
>
> > +{
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id surface_xbl_of_match[] = {
> > + {
> > + .compatible = "microsoft,sm8150-surface-duo-xbl"
> > + },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
> > +
> > +static struct platform_driver surface_xbl_driver = {
> > + .driver = {
> > + .name = "surface-xbl",
> > + .of_match_table = surface_xbl_of_match,
> > + .dev_groups = xbl_sysfs_groups
> > + },
> > + .probe = surface_xbl_probe,
> > + .remove = surface_xbl_remove
> > +};
> > +module_platform_driver(surface_xbl_driver);
> > +
> > +MODULE_AUTHOR("Jarrett Schultz <[email protected]>");
> > +MODULE_DESCRIPTION("Surface Extensible Bootloader");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >