[PATCH 0/3] platform: surface: Introduce Surface XBL Driver

[PATCH 0/3] platform: surface: Introduce Surface XBL Driver

[Jarrett Schultz]

Introduce the Surface Extensible Boot Loader driver for the Surface Duo.
Exposes information about the driver to user space via sysfs.

Jarrett Schultz (3):
  dt-bindings: platform: microsoft: Document surface xbl
  platform: surface: Add surface xbl
  arm64: dts: qcom: surface-duo: Add surface xbl

 .../ABI/testing/sysfs-platform-surface-xbl    |  78 ++++++
 .../platform/microsoft/surface-xbl.yaml       |  37 +++
 MAINTAINERS                                   |   9 +
 .../dts/qcom/sm8150-microsoft-surface-duo.dts |   6 +
 drivers/platform/surface/Kconfig              |  10 +
 drivers/platform/surface/Makefile             |   1 +
 drivers/platform/surface/surface-xbl.c        | 223 ++++++++++++++++++
 7 files changed, 364 insertions(+)
 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

[PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl

[Jarrett Schultz]

Introduce yaml for surface xbl driver.

Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
---
 .../platform/microsoft/surface-xbl.yaml       | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml

diff --git a/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
new file mode 100644
index 000000000000..3d2771322e72
--- /dev/null
+++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/platform/microsoft/surface-xbl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Surface Extensible Bootloader for Microsoft Surface Duo
+
+maintainers:
+  - Jarrett Schultz <jaschultzMS@gmail.com>
+
+description: |
+  Exposes device information to user space.
+
+allOf:
+  - $ref: /schemas/platform/microsoft/surface-xbl.c#
+
+properties:
+  compatible:
+    const: microsoft,sm8150-surface-duo-xbl
+
+  reg:
+    maxItems: 1
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+examples:
+  - |
+    xbl@146bfa94 {
+      compatible = "microsoft,sm8150-surface-duo-xbl";
+      reg = <0x00 0x146bfa94 0x00 0x100>;
+    };
-- 
2.25.1

[PATCH 2/3] platform: surface: Add surface xbl

[Jarrett Schultz]

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 <jaschultz@microsoft.com>
---
 .../ABI/testing/sysfs-platform-surface-xbl    |  78 ++++++
 MAINTAINERS                                   |   9 +
 drivers/platform/surface/Kconfig              |  10 +
 drivers/platform/surface/Makefile             |   1 +
 drivers/platform/surface/surface-xbl.c        | 223 ++++++++++++++++++
 5 files changed, 321 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..d3104dbbc6c1
--- /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.15
+Contact:	jaschultz@microsoft.com
+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.15
+Contact:	jaschultz@microsoft.com
+Description:
+		Read only. It returns the board id.
+
+What:		/sys/devices/platform/146bfa94.xbl/hw_init_retries
+Date:		October 2021
+KernelVersion:	5.15
+Contact:	jaschultz@microsoft.com
+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.15
+Contact:	jaschultz@microsoft.com
+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.15
+Contact:	jaschultz@microsoft.com
+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.15
+Contact:	jaschultz@microsoft.com
+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.15
+Contact:	jaschultz@microsoft.com
+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.15
+Contact:	jaschultz@microsoft.com
+Description:
+		Read only. It returns the version of the firmware.
diff --git a/MAINTAINERS b/MAINTAINERS
index eeb4c70b3d5b..d08b68d626f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12423,6 +12423,15 @@ F:	Documentation/driver-api/surface_aggregator/clients/dtx.rst
 F:	drivers/platform/surface/surface_dtx.c
 F:	include/uapi/linux/surface_aggregator/dtx.h
 
+MICROSOFT SURFACE DUO XBL DRIVER
+M:	Jarrett Schultz <jaschultz@microsoft.com>
+L:	linux-arm-msm@vger.kernel.org
+L:	platform-driver-x86@vger.kernel.org
+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 <luzmaximilian@gmail.com>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index 3105f651614f..ca0546397414 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -180,6 +180,16 @@ 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
+	help
+	  If you say 'Y' to this option, support will be included for the
+	  Surface XBL Driver.
+
+	  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..1ed5808301e9 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..910287f0c987
--- /dev/null
+++ b/drivers/platform/surface/surface-xbl.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * surface-xbl.c - Surface E(x)tensible (B)oot(l)oader
+ *
+ * Copyright (C) 2021 Microsoft Corporation
+ * Author: Jarrett Schultz <jaschultz@microsoft.com>
+ */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kstrtox.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.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;
+} __packed;
+
+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 *inputs_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 inputs_attr_group = {
+	.attrs = inputs_attrs,
+};
+
+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;
+	int					retval;
+
+	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);
+
+	retval = sysfs_create_group(&sxbl->dev->kobj, &inputs_attr_group);
+	if (retval < 0) {
+		dev_dbg(sxbl->dev,
+			"Can't register sysfs attr group: %d\n", retval);
+		return retval;
+	}
+
+	return 0;
+}
+
+static int surface_xbl_remove(struct platform_device *pdev)
+{
+	sysfs_remove_group(&pdev->dev.kobj, &inputs_attr_group);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id surface_xbl_of_match[] = {
+	{
+		.compatible = "microsoft,sm8150-surface-duo-xbl"
+	},
+	{  }, /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
+#endif
+
+static struct platform_driver surface_xbl_driver = {
+	.probe		= surface_xbl_probe,
+	.remove		= surface_xbl_remove,
+	.driver		= {
+		.name	= "surface-xbl",
+		.of_match_table = of_match_ptr(surface_xbl_of_match),
+	},
+};
+
+module_platform_driver(surface_xbl_driver);
+
+MODULE_AUTHOR("Jarrett Schultz <jaschultz@microsoft.com>");
+MODULE_DESCRIPTION("Surface Extensible Bootloader");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH 3/3] arm64: dts: qcom: surface-duo: Add surface xbl

[Jarrett Schultz]

Introduce device tree source for the surface xbl driver.

Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
---
 arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts | 6 ++++++
 1 file changed, 6 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..9fd9f733a791 100644
--- a/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
+++ b/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
@@ -57,6 +57,12 @@ vol_up {
 			linux,code = <KEY_VOLUMEUP>;
 		};
 	};
+
+	xbl@146bfa94 {
+		compatible = "microsoft,sm8150-surface-duo-xbl";
+		reg = <0x00 0x146bfa94 0x00 0x100>;
+		status = "okay";
+	};
 };
 
 &apps_rsc {
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl

[Rob Herring]

On Thu, Oct 28, 2021 at 4:18 PM Jarrett Schultz <jaschultzms@gmail.com> wrote:
>
> Introduce yaml for surface xbl driver.

What's surface? What's xbl? Bindings are for h/w devices, not drivers.

Please send DT patches to the DT list. IOW, run get_maintainers.pl.

>
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> ---
>  .../platform/microsoft/surface-xbl.yaml       | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
>
> diff --git a/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> new file mode 100644
> index 000000000000..3d2771322e72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license please. Run checkpatch.pl as that will tell you this.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/platform/microsoft/surface-xbl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Surface Extensible Bootloader for Microsoft Surface Duo
> +
> +maintainers:
> +  - Jarrett Schultz <jaschultzMS@gmail.com>
> +
> +description: |
> +  Exposes device information to user space.

What does that mean?

> +
> +allOf:
> +  - $ref: /schemas/platform/microsoft/surface-xbl.c#

You have a C file with json-schema?

> +
> +properties:
> +  compatible:
> +    const: microsoft,sm8150-surface-duo-xbl
> +
> +  reg:
> +    maxItems: 1
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +examples:
> +  - |
> +    xbl@146bfa94 {
> +      compatible = "microsoft,sm8150-surface-duo-xbl";
> +      reg = <0x00 0x146bfa94 0x00 0x100>;

That's an odd address. Is this part of some other block?

> +    };
> --
> 2.25.1
>

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Randy Dunlap]

On 10/28/21 2:17 PM, Jarrett Schultz wrote:
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 3105f651614f..ca0546397414 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -180,6 +180,16 @@ 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
> +	help
> +	  If you say 'Y' to this option, support will be included for the
> +	  Surface XBL Driver.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called surface-xbl.

Hi,

Can you tell the user/reader what XBL means, please?

Don't assume that they will look at some documentation.

thanks.
-- 
~Randy

Re: [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl

[Bjorn Andersson]

On Thu 28 Oct 14:17 PDT 2021, Jarrett Schultz wrote:

> Introduce yaml for surface xbl driver.
> 
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> ---
>  .../platform/microsoft/surface-xbl.yaml       | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> new file mode 100644
> index 000000000000..3d2771322e72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/platform/microsoft/surface-xbl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Surface Extensible Bootloader for Microsoft Surface Duo
> +
> +maintainers:
> +  - Jarrett Schultz <jaschultzMS@gmail.com>
> +
> +description: |
> +  Exposes device information to user space.
> +
> +allOf:
> +  - $ref: /schemas/platform/microsoft/surface-xbl.c#
> +
> +properties:
> +  compatible:
> +    const: microsoft,sm8150-surface-duo-xbl
> +
> +  reg:
> +    maxItems: 1
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +examples:
> +  - |
> +    xbl@146bfa94 {
> +      compatible = "microsoft,sm8150-surface-duo-xbl";
> +      reg = <0x00 0x146bfa94 0x00 0x100>;

I believe this is in the middle of IMEM. If so it would be better to
describe the entire IMEM section in one block and then list this as one
of the pieces within.

Please see e.g. imem@146aa000 in sc7180.dtsi

Thanks,
Bjorn

> +    };
> -- 
> 2.25.1
> 

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Maximilian Luz]

On 10/28/21 23:17, 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 <jaschultz@microsoft.com>

In addition to the comments of Andy and Randy:

  - Your S-o-b address doesn't match the address you've sent this from.

  - Are you aware that the platform/surface directory as a whole depends
    on CONFIG_ACPI? As I understand it this patch is intended for an ARM
    device with DT/OF, so you might want to add a patch that moves the
    CONFIG_ACPI dependency to the individual drivers. A dependency of
    CONFIG_ACPI for the XBL driver seems rather unnecessary.

> ---
>   .../ABI/testing/sysfs-platform-surface-xbl    |  78 ++++++
>   MAINTAINERS                                   |   9 +
>   drivers/platform/surface/Kconfig              |  10 +
>   drivers/platform/surface/Makefile             |   1 +
>   drivers/platform/surface/surface-xbl.c        | 223 ++++++++++++++++++
>   5 files changed, 321 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..d3104dbbc6c1
> --- /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.15
> +Contact:	jaschultz@microsoft.com
> +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.15
> +Contact:	jaschultz@microsoft.com
> +Description:
> +		Read only. It returns the board id.
> +
> +What:		/sys/devices/platform/146bfa94.xbl/hw_init_retries
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@microsoft.com
> +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.15
> +Contact:	jaschultz@microsoft.com
> +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.15
> +Contact:	jaschultz@microsoft.com
> +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.15
> +Contact:	jaschultz@microsoft.com
> +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.15
> +Contact:	jaschultz@microsoft.com
> +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.15
> +Contact:	jaschultz@microsoft.com
> +Description:
> +		Read only. It returns the version of the firmware.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eeb4c70b3d5b..d08b68d626f6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12423,6 +12423,15 @@ F:	Documentation/driver-api/surface_aggregator/clients/dtx.rst
>   F:	drivers/platform/surface/surface_dtx.c
>   F:	include/uapi/linux/surface_aggregator/dtx.h
>   
> +MICROSOFT SURFACE DUO XBL DRIVER
> +M:	Jarrett Schultz <jaschultz@microsoft.com>
> +L:	linux-arm-msm@vger.kernel.org
> +L:	platform-driver-x86@vger.kernel.org
> +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 <luzmaximilian@gmail.com>
>   L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 3105f651614f..ca0546397414 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -180,6 +180,16 @@ 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
> +	help
> +	  If you say 'Y' to this option, support will be included for the
> +	  Surface XBL Driver.
> +
> +	  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..1ed5808301e9 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..910287f0c987
> --- /dev/null
> +++ b/drivers/platform/surface/surface-xbl.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * surface-xbl.c - Surface E(x)tensible (B)oot(l)oader
> + *
> + * Copyright (C) 2021 Microsoft Corporation
> + * Author: Jarrett Schultz <jaschultz@microsoft.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/kstrtox.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.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;
> +} __packed;
> +
> +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 *inputs_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 inputs_attr_group = {
> +	.attrs = inputs_attrs,
> +};
> +
> +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;
> +	int					retval;
> +
> +	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);
> +
> +	retval = sysfs_create_group(&sxbl->dev->kobj, &inputs_attr_group);
> +	if (retval < 0) {
> +		dev_dbg(sxbl->dev,
> +			"Can't register sysfs attr group: %d\n", retval);
> +		return retval;
> +	}
> +
> +	return 0;
> +}
> +
> +static int surface_xbl_remove(struct platform_device *pdev)
> +{
> +	sysfs_remove_group(&pdev->dev.kobj, &inputs_attr_group);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id surface_xbl_of_match[] = {
> +	{
> +		.compatible = "microsoft,sm8150-surface-duo-xbl"
> +	},
> +	{  }, /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
> +#endif
> +
> +static struct platform_driver surface_xbl_driver = {
> +	.probe		= surface_xbl_probe,
> +	.remove		= surface_xbl_remove,
> +	.driver		= {
> +		.name	= "surface-xbl",
> +		.of_match_table = of_match_ptr(surface_xbl_of_match),
> +	},
> +};
> +
> +module_platform_driver(surface_xbl_driver);
> +
> +MODULE_AUTHOR("Jarrett Schultz <jaschultz@microsoft.com>");
> +MODULE_DESCRIPTION("Surface Extensible Bootloader");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Felipe Balbi]

Hi,

Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>  diff --git a/drivers/platform/surface/surface-xbl.c b/drivers/platform/surface/surface-xbl.c
>  new file mode 100644
>  index 000000000000..910287f0c987
>  --- /dev/null
>  +++ b/drivers/platform/surface/surface-xbl.c
>  @@ -0,0 +1,223 @@
>  +// SPDX-License-Identifier: GPL-2.0-only
>  +/*
>  + * surface-xbl.c - Surface E(x)tensible (B)oot(l)oader
>  + *
>
> First of all, do not include filename in the file.
>
> Capital L will be better to read and understand the
> abbreviation. Actually usually we do something like this:
>
> Extensible Boot Loader (EBL)

nah, this is silly Andy. It's just capitalized as eXtensible Boot
Loader, very much akin to eXtensible Host Controller Interface.

>  +static const struct attribute_group inputs_attr_group = {
>  +       .attrs = inputs_attrs,
>  +};
>  +
>  +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);
>  +}
>
> Either use corresponding io accessors in-line, or make first parameter
> to be sirface_xbl pointer. Otherwise these helpers useless.

I agree with passing surface_xbl point as first parameter, but calling
the accessors pointless is a bit much. At a minimum, they make it easier
to ftrace the entire driver by simply ftracing surface_xbl_*

-- 
balbi

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Andy Shevchenko]

On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@kernel.org> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:

...

> > Capital L will be better to read and understand the
> > abbreviation. Actually usually we do something like this:
> >
> > Extensible Boot Loader (EBL)
>
> nah, this is silly Andy. It's just capitalized as eXtensible Boot
> Loader, very much akin to eXtensible Host Controller Interface.

My point here is to have a full name followed by the abbreviation. and
n(O)t in (F)ancy st(Y)le.

...

> >  +static const struct attribute_group inputs_attr_group = {
> >  +       .attrs = inputs_attrs,
> >  +};
> >  +
> >  +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);
> >  +}
> >
> > Either use corresponding io accessors in-line, or make first parameter
> > to be sirface_xbl pointer. Otherwise these helpers useless.
>
> I agree with passing surface_xbl point as first parameter, but calling
> the accessors pointless is a bit much. At a minimum, they make it easier
> to ftrace the entire driver by simply ftracing surface_xbl_*

My point is that the above seems half-baked. It's pointless to have a
func(a,b) { return readl(a + b); }. It doesn't add value.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Felipe Balbi]

Hi,

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@kernel.org> wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
> ...
>
>> > Capital L will be better to read and understand the
>> > abbreviation. Actually usually we do something like this:
>> >
>> > Extensible Boot Loader (EBL)
>>
>> nah, this is silly Andy. It's just capitalized as eXtensible Boot
>> Loader, very much akin to eXtensible Host Controller Interface.
>
> My point here is to have a full name followed by the abbreviation. and
> n(O)t in (F)ancy st(Y)le.

too bad my patch removing acronyms from the kernel got rejects :-p

Seriously, this is pretty pointless. You're vouching for something that
will just cause confusion. Every piece of internal documentation refers
to xbl and you want this to be renamed to ebl because it looks nicer for
you. Thanks, but no thanks.

>> >  +static const struct attribute_group inputs_attr_group = {
>> >  +       .attrs = inputs_attrs,
>> >  +};
>> >  +
>> >  +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);
>> >  +}
>> >
>> > Either use corresponding io accessors in-line, or make first parameter
>> > to be sirface_xbl pointer. Otherwise these helpers useless.
>>
>> I agree with passing surface_xbl point as first parameter, but calling
>> the accessors pointless is a bit much. At a minimum, they make it easier
>> to ftrace the entire driver by simply ftracing surface_xbl_*
>
> My point is that the above seems half-baked. It's pointless to have a
> func(a,b) { return readl(a + b); }. It doesn't add value.

sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever
the filename was) it reason enough IMHO. Not to mention that these
little accessors will likely be optimized by the compiler.

-- 
balbi

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Andy Shevchenko]

On Fri, Oct 29, 2021 at 3:34 PM Felipe Balbi <balbi@kernel.org> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@kernel.org> wrote:
> >> Andy Shevchenko <andy.shevchenko@gmail.com> writes:

...

> >> > Capital L will be better to read and understand the
> >> > abbreviation. Actually usually we do something like this:
> >> >
> >> > Extensible Boot Loader (EBL)
> >>
> >> nah, this is silly Andy. It's just capitalized as eXtensible Boot
> >> Loader, very much akin to eXtensible Host Controller Interface.
> >
> > My point here is to have a full name followed by the abbreviation. and
> > n(O)t in (F)ancy st(Y)le.
>
> too bad my patch removing acronyms from the kernel got rejects :-p
>
> Seriously, this is pretty pointless. You're vouching for something that
> will just cause confusion. Every piece of internal documentation refers
> to xbl and you want this to be renamed to ebl because it looks nicer for
> you. Thanks, but no thanks.

Maybe I was too unclear. I'm not pushing for EBL, I'm pushing for the form os

"Foo bAr BullSh*t (FABS)" vs. "(F)oo b(a)r (B)ull(s)h*t".

If you have x there to be capitalized, do it like "eXtensible Boot
Loader (XBL)". Is it too hard?

...

> >> >  +static const struct attribute_group inputs_attr_group = {
> >> >  +       .attrs = inputs_attrs,
> >> >  +};
> >> >  +
> >> >  +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);
> >> >  +}
> >> >
> >> > Either use corresponding io accessors in-line, or make first parameter
> >> > to be sirface_xbl pointer. Otherwise these helpers useless.
> >>
> >> I agree with passing surface_xbl point as first parameter, but calling
> >> the accessors pointless is a bit much. At a minimum, they make it easier
> >> to ftrace the entire driver by simply ftracing surface_xbl_*
> >
> > My point is that the above seems half-baked. It's pointless to have a
> > func(a,b) { return readl(a + b); }. It doesn't add value.
>
> sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever
> the filename was) it reason enough IMHO. Not to mention that these
> little accessors will likely be optimized by the compiler.

readl() will appear in the traces, no? But yeah I also was thinking
about the weakness in your argument that the compiler can silently
inline them anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Felipe Balbi]

Hi,

Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> >> > Capital L will be better to read and understand the
>> >> > abbreviation. Actually usually we do something like this:
>> >> >
>> >> > Extensible Boot Loader (EBL)
>> >>
>> >> nah, this is silly Andy. It's just capitalized as eXtensible Boot
>> >> Loader, very much akin to eXtensible Host Controller Interface.
>> >
>> > My point here is to have a full name followed by the abbreviation. and
>> > n(O)t in (F)ancy st(Y)le.
>>
>> too bad my patch removing acronyms from the kernel got rejects :-p
>>
>> Seriously, this is pretty pointless. You're vouching for something that
>> will just cause confusion. Every piece of internal documentation refers
>> to xbl and you want this to be renamed to ebl because it looks nicer for
>> you. Thanks, but no thanks.
>
> Maybe I was too unclear. I'm not pushing for EBL, I'm pushing for the form os
>
> "Foo bAr BullSh*t (FABS)" vs. "(F)oo b(a)r (B)ull(s)h*t".
>
> If you have x there to be capitalized, do it like "eXtensible Boot
> Loader (XBL)". Is it too hard?

Take a breather Andy, you need it. Winter sure is coming

>> >> >  +static const struct attribute_group inputs_attr_group = {
>> >> >  +       .attrs = inputs_attrs,
>> >> >  +};
>> >> >  +
>> >> >  +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);
>> >> >  +}
>> >> >
>> >> > Either use corresponding io accessors in-line, or make first parameter
>> >> > to be sirface_xbl pointer. Otherwise these helpers useless.
>> >>
>> >> I agree with passing surface_xbl point as first parameter, but calling
>> >> the accessors pointless is a bit much. At a minimum, they make it easier
>> >> to ftrace the entire driver by simply ftracing surface_xbl_*
>> >
>> > My point is that the above seems half-baked. It's pointless to have a
>> > func(a,b) { return readl(a + b); }. It doesn't add value.
>>
>> sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever
>> the filename was) it reason enough IMHO. Not to mention that these
>> little accessors will likely be optimized by the compiler.
>
> readl() will appear in the traces, no? But yeah I also was thinking
> about the weakness in your argument that the compiler can silently
> inline them anyway.

In non-debug builds, when tracers are enabled a thunk will be added for
runtime patching ;-) (IIRC)

-- 
balbi

Re: [PATCH 2/3] platform: surface: Add surface xbl

[Andy Shevchenko]

On Fri, Oct 29, 2021 at 4:14 PM Felipe Balbi <balbi@kernel.org> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> Take a breather Andy, you need it. Winter sure is coming

Indeed.

-- 
With Best Regards,
Andy Shevchenko