[PATCH v2 0/2] Add driver for SIE Cronos control CPLD

[PATCH v2 0/2] Add driver for SIE Cronos control CPLD

[Shawn Anastasio]

Hello all,

This series adds a driver for the multi-function CPLD found on the Sony
Interactive Entertainment Cronos x86 server platform. It provides a
watchdog timer and an LED controller, both of which will depend on the
MFD parent driver implemented in this series. Device tree bindings are
also included.

Thanks,

Changes in v2:
  - Change SIE to Sony (SIE's parent company) to be more consistent
  with how other subsidiaries are treated in the kernel.
  - Drop SIE prefix addition patch
  - Address review comments to dt bindings
  - Add new properties to dt bindings
  - Fix driver build failure detected by kernel test robot

Shawn Anastasio (1):
  dt-bindings: mfd: Add sony,cronos-cpld

Timothy Pearson (1):
  mfd: sie-cronos-cpld: Add driver for Sony Cronos CPLD

 .../bindings/mfd/sony,cronos-cpld.yaml        |  92 +++
 MAINTAINERS                                   |   7 +
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/sony-cronos-cpld.c                | 591 ++++++++++++++++++
 include/linux/mfd/sony/cronos/core.h          |  17 +
 include/linux/mfd/sony/cronos/registers.h     |  59 ++
 7 files changed, 778 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
 create mode 100644 drivers/mfd/sony-cronos-cpld.c
 create mode 100644 include/linux/mfd/sony/cronos/core.h
 create mode 100644 include/linux/mfd/sony/cronos/registers.h

--
2.30.2

[PATCH v2 1/2] dt-bindings: mfd: Add sony,cronos-cpld

[Shawn Anastasio]

The Sony Cronos Platform Controller CPLD is a multi-purpose platform
controller that provides both a watchdog timer and an LED controller for
the Sony Interactive Entertainment Cronos x86 server platform. As both
functions are provided by the same CPLD, a multi-function device is
exposed as the parent of both functions.

Add a DT binding for this device.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in v2:
  - Change SIE to Sony to use the already-established prefix.
  - Clarify that Cronos is an x86 server platform in description
  - Drop #address-cells/#size-cells
  - Add missing additionalProperties to leds/watchdog objects
  - Add sony,led-mask property to leds object
  - Add sony,default-timeout property to watchdog object
  - Update example

 .../bindings/mfd/sony,cronos-cpld.yaml        | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml

diff --git a/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
new file mode 100644
index 000000000000..df2c2e83ccb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Raptor Engineering, LLC
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/sony,cronos-cpld.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony Cronos Platform Controller CPLD multi-function device
+
+maintainers:
+  - Timothy Pearson <tpearson@raptorengineering.com>
+
+description: |
+  The Sony Cronos Platform Controller CPLD is a multi-purpose platform
+  controller that provides both a watchdog timer and an LED controller for the
+  Sony Interactive Entertainment Cronos x86 server platform. As both functions
+  are provided by the same CPLD, a multi-function device is exposed as the
+  parent of both functions.
+
+properties:
+  compatible:
+    const: sony,cronos-cpld
+
+  reg:
+    maxItems: 1
+
+  leds:
+    type: object
+    description: Cronos Platform Status LEDs
+
+    properties:
+      compatible:
+        const: sony,cronos-leds
+
+      sony,led-mask:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        minimum: 0x0
+        maximum: 0x7fff
+        description: |
+          A bitmask that specifies which LEDs are present and can be controlled
+          by the Cronos CPLD. Bits 0-5 correspond to platform Status LEDs, bits
+          6-10 correspond to Link LEDs, and bits 11-14 correspond to the Power
+          State LEDs. All other bits are unused. The default value is 0x7fff
+          (all possible LEDs enabled).
+
+    additionalProperties: false
+
+  watchdog:
+    type: object
+    description: Cronos Platform Watchdog Timer
+
+    properties:
+      compatible:
+        const: sony,cronos-watchdog
+
+      sony,default-timeout:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          The default timeout with which the watchdog timer is initialized, in
+          seconds. Supported values are: 10, 20, 30, 40, 50, 60, 70, 80. All
+          other values will be rounded down to the nearest supported value.  The
+          default value is 80.
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      cpld@3f {
+        compatible = "sony,cronos-cpld";
+        reg = <0x3f>;
+
+        watchdog {
+          compatible = "sony,cronos-watchdog";
+          sony,default-timeout = <20>;
+        };
+
+        leds {
+          compatible = "sony,cronos-leds";
+          sony,led-mask = <0x7fff>;
+        };
+      };
+    };
--
2.30.2

[PATCH v2 2/2] mfd: sie-cronos-cpld: Add driver for Sony Cronos CPLD

[Shawn Anastasio]

From: Timothy Pearson <tpearson@raptorengineering.com>

The Sony Cronos Platform Controller CPLD is a multi-purpose platform
controller that provides both a watchdog timer and an LED controller for
the Sony Interactive Entertainment Cronos x86 server platform. As both
functions are provided by the same CPLD, a multi-function device is
exposed as the parent of both functions.

Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
Changes in v2:
  - Change SIE to Sony (SIE's parent company) to be more consistent
  with how other subsidiaries are treated in the kernel
  - Fix build issue under !CONFIG_OF discovered by kernel test robot
  by guarding definition of `cronos_cpld_dt_ids` as is done in other
  drivers.

 MAINTAINERS                               |   7 +
 drivers/mfd/Kconfig                       |  11 +
 drivers/mfd/Makefile                      |   1 +
 drivers/mfd/sony-cronos-cpld.c            | 591 ++++++++++++++++++++++
 include/linux/mfd/sony/cronos/core.h      |  17 +
 include/linux/mfd/sony/cronos/registers.h |  59 +++
 6 files changed, 686 insertions(+)
 create mode 100644 drivers/mfd/sony-cronos-cpld.c
 create mode 100644 include/linux/mfd/sony/cronos/core.h
 create mode 100644 include/linux/mfd/sony/cronos/registers.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6c4cce45a09d..623681826820 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19932,6 +19932,13 @@ S:	Maintained
 F:	drivers/ssb/
 F:	include/linux/ssb/

+SONY CRONOS CPLD DRIVER
+M:	Georgy Yakovlev <Georgy.Yakovlev@sony.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
+F:	drivers/mfd/sony-cronos-cpld.c
+F:	include/linux/mfd/sony/cronos/
+
 SONY IMX208 SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 90ce58fd629e..27f28fbbc7cc 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2217,6 +2217,17 @@ config MFD_QCOM_PM8008
 	  under it in the device tree. Additional drivers must be enabled in
 	  order to use the functionality of the device.

+config MFD_SONY_CRONOS_CPLD
+	tristate "Sony Cronos CPLD Support"
+	select MFD_CORE
+	select REGMAP_I2C
+	depends on I2C
+	help
+      Support for the Sony Cronos system control CPLDs. Additional drivers must
+      be enabled in order to use the functionality of the device, including LED
+      control and the system watchdog. The controller itself is a custom design
+      tailored to the specific needs of the Sony Cronos hardware platform.
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..be9974f0fe9c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -284,3 +284,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
 rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
 obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
 obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
+obj-$(CONFIG_MFD_SONY_CRONOS_CPLD)	+= sony-cronos-cpld.o
diff --git a/drivers/mfd/sony-cronos-cpld.c b/drivers/mfd/sony-cronos-cpld.c
new file mode 100644
index 000000000000..569793cd9697
--- /dev/null
+++ b/drivers/mfd/sony-cronos-cpld.c
@@ -0,0 +1,591 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * I2C device driver for Sony Cronos CPLDs
+ * Copyright (C) 2015-2017  Dialog Semiconductor
+ * Copyright (C) 2022  Raptor Engineering, LLC
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/mfd/core.h>
+#include <linux/i2c.h>
+#include <linux/mfd/sony/cronos/core.h>
+#include <linux/mfd/sony/cronos/registers.h>
+
+static struct resource cronos_wdt_resources[] = {
+};
+
+static struct resource cronos_led_resources[] = {
+};
+
+static const struct mfd_cell cronos_cpld_devs[] = {
+	{
+		.name          = "cronos-watchdog",
+		.num_resources = ARRAY_SIZE(cronos_wdt_resources),
+		.resources     = cronos_wdt_resources,
+		.of_compatible = "sony,cronos-watchdog",
+	},
+	{
+		.name          = "cronos-leds",
+		.id            = 1,
+		.num_resources = ARRAY_SIZE(cronos_led_resources),
+		.resources     = cronos_led_resources,
+		.of_compatible = "sony,cronos-leds",
+	},
+};
+
+static ssize_t payload_power_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int payloadpower_val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_PAYLOAD_POWER_CTL_REG, &payloadpower_val);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", payloadpower_val);
+}
+
+static ssize_t payload_power_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t len)
+{
+	u8 val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	ret = regmap_write(chip->regmap, CRONOS_CPLD_PAYLOAD_POWER_CTL_REG, val);
+	if (ret) {
+		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
+			val, CRONOS_CPLD_PAYLOAD_POWER_CTL_REG);
+		return ret;
+	}
+	return len;
+}
+
+
+static ssize_t bmc_flash_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int bmcflash_val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG, &bmcflash_val);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", bmcflash_val);
+}
+
+static ssize_t bmc_flash_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t len)
+{
+	u8 val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	ret = regmap_write(chip->regmap, CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG, val);
+	if (ret) {
+		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
+			val, CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG);
+		return ret;
+	}
+	return len;
+}
+
+
+static ssize_t switch_reset_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int switchreset_val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_SWITCH_RESET_CMD_REG, &switchreset_val);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", switchreset_val);
+}
+
+static ssize_t switch_reset_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t len)
+{
+	unsigned int switchreset_val = 0;
+	u8 val = -EINVAL;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	if (val != 1)
+		return -EINVAL;
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_SWITCH_RESET_CMD_REG, &switchreset_val);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(chip->regmap, CRONOS_CPLD_SWITCH_RESET_CMD_REG, switchreset_val);
+	if (ret) {
+		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
+				switchreset_val, CRONOS_CPLD_SWITCH_RESET_CMD_REG);
+		return ret;
+	}
+	return len;
+}
+
+
+static ssize_t switch_flash_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int switchflash_val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG, &switchflash_val);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", switchflash_val);
+}
+
+static ssize_t switch_flash_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t len)
+{
+	u8 val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	ret = regmap_write(chip->regmap, CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG, val);
+	if (ret) {
+		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
+			val, CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG);
+		return ret;
+	}
+	return len;
+}
+
+
+static ssize_t uart_mux_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int uartmux_val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_UART_MUX_REG, &uartmux_val);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", uartmux_val);
+}
+
+static ssize_t uart_mux_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t len)
+{
+	u8 val = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	ret = regmap_write(chip->regmap, CRONOS_CPLD_UART_MUX_REG, val);
+	if (ret) {
+		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
+			val, CRONOS_CPLD_UART_MUX_REG);
+		return ret;
+	}
+	return len;
+}
+
+
+static ssize_t led_get_brightness(struct sony_cronos_cpld *chip, unsigned int reg, char *buf)
+{
+	unsigned int brightness_val;
+	int ret = -EIO;
+
+	ret = regmap_read(chip->regmap, reg, &brightness_val);
+	if (ret != 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", brightness_val);
+}
+
+static ssize_t led_set_brightness(struct sony_cronos_cpld *chip, unsigned int reg, const char *buf,
+	size_t len)
+{
+	u8 val = 0;
+	int ret = -EIO;
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	ret = regmap_update_bits(chip->regmap, reg, CRONOS_CPLD_LEDS_BRIGHTNESS_SET_MASK, val);
+	if (ret) {
+		dev_err(chip->dev, "Failed to write value 0x%02x to address 0x%02x", val, reg);
+		return ret;
+	}
+	return len;
+}
+
+static ssize_t brightness_red_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	return led_get_brightness(chip, CRONOS_CPLD_BRIGHTNESS_RED_REG, buf);
+}
+
+static ssize_t brightness_red_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t len)
+{
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	return led_set_brightness(chip, CRONOS_CPLD_BRIGHTNESS_RED_REG, buf, len);
+}
+
+static ssize_t brightness_green_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	return led_get_brightness(chip, CRONOS_CPLD_BRIGHTNESS_GREEN_REG, buf);
+}
+
+static ssize_t brightness_green_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t len)
+{
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	return led_set_brightness(chip, CRONOS_CPLD_BRIGHTNESS_GREEN_REG, buf, len);
+}
+
+static ssize_t brightness_blue_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	return led_get_brightness(chip, CRONOS_CPLD_BRIGHTNESS_BLUE_REG, buf);
+}
+
+static ssize_t brightness_blue_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t len)
+{
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	return led_set_brightness(chip, CRONOS_CPLD_BRIGHTNESS_BLUE_REG, buf, len);
+}
+
+
+static ssize_t revision_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	u16 revision = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_bulk_read(chip->regmap, CRONOS_CPLD_REVISION_LOW_REG, &revision, 2);
+	if (ret)
+		return -EIO;
+
+	return snprintf(buf, PAGE_SIZE, "0x%04x\n", revision);
+}
+
+static ssize_t device_id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	u16 device_id = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_bulk_read(chip->regmap, CRONOS_CPLD_DEVICE_ID_LOW_REG, &device_id, 2);
+	if (ret)
+		return -EIO;
+
+	return snprintf(buf, PAGE_SIZE, "0x%04x\n", device_id);
+}
+
+static ssize_t bmc_mac_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	u8 bmc_mac[6];
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_bulk_read(chip->regmap, CRONOS_CPLD_BMC_MAC_LOW_REG, bmc_mac, 6);
+	if (ret)
+		return -EIO;
+
+	return snprintf(buf, PAGE_SIZE, "%pM\n", bmc_mac);
+}
+
+static ssize_t status_2_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int last_boot = 0;
+	int ret = -EIO;
+	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_STATUS_2_REG, &last_boot);
+	if (ret < 0)
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "0x%02x\n", last_boot);
+}
+
+
+static DEVICE_ATTR_RO(revision);
+static DEVICE_ATTR_RO(device_id);
+static DEVICE_ATTR_RO(bmc_mac);
+static DEVICE_ATTR_RO(status_2);
+
+static DEVICE_ATTR_RW(uart_mux);
+static DEVICE_ATTR_RW(switch_flash);
+static DEVICE_ATTR_RW(switch_reset);
+static DEVICE_ATTR_RW(bmc_flash);
+static DEVICE_ATTR_RW(payload_power);
+
+static DEVICE_ATTR_RW(brightness_red);
+static DEVICE_ATTR_RW(brightness_green);
+static DEVICE_ATTR_RW(brightness_blue);
+static struct attribute *cronos_cpld_sysfs_entries[] = {
+	&dev_attr_revision.attr,
+	&dev_attr_device_id.attr,
+	&dev_attr_bmc_mac.attr,
+	&dev_attr_status_2.attr,
+	&dev_attr_uart_mux.attr,
+	&dev_attr_switch_flash.attr,
+	&dev_attr_switch_reset.attr,
+	&dev_attr_bmc_flash.attr,
+	&dev_attr_payload_power.attr,
+	&dev_attr_brightness_red.attr,
+	&dev_attr_brightness_green.attr,
+	&dev_attr_brightness_blue.attr,
+	NULL,
+};
+
+static const struct attribute_group cronos_cpld_attr_group = {
+	.attrs	= cronos_cpld_sysfs_entries,
+};
+
+static int sony_cronos_get_device_type(struct sony_cronos_cpld *chip)
+{
+	int device_id;
+	int byte;
+	int ret;
+
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_DEVICE_ID_HIGH_REG, &byte);
+	if (ret < 0) {
+		dev_err(chip->dev, "Cannot read chip ID.\n");
+		return -EIO;
+	}
+	device_id = byte << 8;
+	ret = regmap_read(chip->regmap, CRONOS_CPLD_DEVICE_ID_LOW_REG, &byte);
+	if (ret < 0) {
+		dev_err(chip->dev, "Cannot read chip ID.\n");
+		return -EIO;
+	}
+	device_id |= byte;
+	if (device_id != CRONOS_CPLD_DEVICE_ID) {
+		dev_err(chip->dev, "Invalid device ID: 0x%04x\n", device_id);
+		return -ENODEV;
+	}
+
+	dev_info(chip->dev,
+		 "Device detected (device-ID: 0x%04X)\n",
+		 device_id);
+
+	return ret;
+}
+
+static bool cronos_cpld_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case CRONOS_CPLD_BRIGHTNESS_RED_REG:
+	case CRONOS_CPLD_BRIGHTNESS_GREEN_REG:
+	case CRONOS_CPLD_BRIGHTNESS_BLUE_REG:
+	case CRONOS_LEDS_SMC_STATUS_REG:
+	case CRONOS_LEDS_SWITCH_STATUS_REG:
+	case CRONOS_LEDS_CCM1_STATUS_REG:
+	case CRONOS_LEDS_CCM2_STATUS_REG:
+	case CRONOS_LEDS_CCM3_STATUS_REG:
+	case CRONOS_LEDS_CCM4_STATUS_REG:
+	case CRONOS_LEDS_CCM_POWER_REG:
+
+	case CRONOS_WDT_CTL_REG:
+	case CRONOS_WDT_CLR_REG:
+
+	case CRONOS_CPLD_UART_MUX_REG:
+	case CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG:
+	case CRONOS_CPLD_SWITCH_RESET_CMD_REG:
+	case CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG:
+	case CRONOS_CPLD_PAYLOAD_POWER_CTL_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool cronos_cpld_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case CRONOS_CPLD_REVISION_HIGH_REG:
+	case CRONOS_CPLD_REVISION_LOW_REG:
+	case CRONOS_CPLD_DEVICE_ID_HIGH_REG:
+	case CRONOS_CPLD_DEVICE_ID_LOW_REG:
+
+	case CRONOS_CPLD_BRIGHTNESS_RED_REG:
+	case CRONOS_CPLD_BRIGHTNESS_GREEN_REG:
+	case CRONOS_CPLD_BRIGHTNESS_BLUE_REG:
+	case CRONOS_LEDS_SMC_STATUS_REG:
+	case CRONOS_LEDS_SWITCH_STATUS_REG:
+	case CRONOS_LEDS_CCM1_STATUS_REG:
+	case CRONOS_LEDS_CCM2_STATUS_REG:
+	case CRONOS_LEDS_CCM3_STATUS_REG:
+	case CRONOS_LEDS_CCM4_STATUS_REG:
+	case CRONOS_LEDS_CCM_POWER_REG:
+
+	case CRONOS_WDT_CTL_REG:
+	case CRONOS_WDT_CLR_REG:
+
+	case CRONOS_CPLD_STATUS_2_REG:
+	case CRONOS_CPLD_UART_MUX_REG:
+	case CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG:
+	case CRONOS_CPLD_SWITCH_RESET_CMD_REG:
+	case CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG:
+	case CRONOS_CPLD_PAYLOAD_POWER_CTL_REG:
+
+	case CRONOS_CPLD_BMC_MAC_LOW_REG ... CRONOS_CPLD_BMC_MAC_HIGH_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool cronos_cpld_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case CRONOS_CPLD_REVISION_HIGH_REG:
+	case CRONOS_CPLD_REVISION_LOW_REG:
+
+	case CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG:
+	case CRONOS_CPLD_SWITCH_RESET_CMD_REG:
+	case CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG:
+	case CRONOS_CPLD_PAYLOAD_POWER_CTL_REG:
+
+	case CRONOS_WDT_CTL_REG:
+	case CRONOS_WDT_CLR_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static struct regmap_config cronos_cpld_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = CRONOS_CPLD_REVISION_HIGH_REG,
+	.writeable_reg = cronos_cpld_is_writeable_reg,
+	.readable_reg = cronos_cpld_is_readable_reg,
+	.volatile_reg = cronos_cpld_is_volatile_reg,
+	.use_single_read = true,
+	.use_single_write = true,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id cronos_cpld_dt_ids[] = {
+	{ .compatible = "sony,cronos-cpld", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cronos_cpld_dt_ids);
+#endif
+
+static int sony_cronos_i2c_probe(struct i2c_client *i2c)
+{
+	struct sony_cronos_cpld *chip;
+	const struct of_device_id *match;
+	const struct mfd_cell *cell;
+	const struct regmap_config *config;
+	int cell_num;
+	int ret;
+
+	chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	if (i2c->dev.of_node) {
+		match = of_match_node(cronos_cpld_dt_ids, i2c->dev.of_node);
+		if (!match)
+			return -EINVAL;
+	}
+
+	i2c_set_clientdata(i2c, chip);
+	chip->dev = &i2c->dev;
+
+	cell = cronos_cpld_devs;
+	cell_num = ARRAY_SIZE(cronos_cpld_devs);
+	config = &cronos_cpld_regmap_config;
+
+	chip->regmap = devm_regmap_init_i2c(i2c, config);
+	if (IS_ERR(chip->regmap)) {
+		ret = PTR_ERR(chip->regmap);
+		dev_err(chip->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = sony_cronos_get_device_type(chip);
+	if (ret)
+		return ret;
+
+	ret = mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE, cell,
+			      cell_num, NULL, 0, NULL);
+	if (ret) {
+		dev_err(chip->dev, "Cannot register child devices\n");
+		return ret;
+	}
+
+	/* Add sysfs */
+	ret = sysfs_create_group(&chip->dev->kobj, &cronos_cpld_attr_group);
+	if (ret)
+		dev_err(chip->dev, "Failed to create sysfs entries\n");
+
+	return ret;
+}
+
+static void sony_cronos_i2c_remove(struct i2c_client *i2c)
+{
+	struct sony_cronos_cpld *chip = i2c_get_clientdata(i2c);
+
+	sysfs_remove_group(&chip->dev->kobj, &cronos_cpld_attr_group);
+	mfd_remove_devices(chip->dev);
+}
+
+static struct i2c_driver sony_cronos_i2c_driver = {
+	.driver = {
+		.name = "sony-cronos",
+		.of_match_table = of_match_ptr(cronos_cpld_dt_ids),
+	},
+	.probe    = sony_cronos_i2c_probe,
+	.remove   = sony_cronos_i2c_remove,
+};
+
+module_i2c_driver(sony_cronos_i2c_driver);
+
+MODULE_DESCRIPTION("Core device driver for sony Cronos CPLDs");
+MODULE_AUTHOR("Raptor Engineering, LLC <support@raptorengineering.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/sony/cronos/core.h b/include/linux/mfd/sony/cronos/core.h
new file mode 100644
index 000000000000..6f80b90af5ca
--- /dev/null
+++ b/include/linux/mfd/sony/cronos/core.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2015-2017  Dialog Semiconductor
+ * Copyright (C) 2022  Raptor Engineering, LLC
+ */
+
+#ifndef __MFD_SONY_CRONOS_CORE_H__
+#define __MFD_SONY_CRONOS_CORE_H__
+
+#include <linux/mfd/sony/cronos/registers.h>
+
+struct sony_cronos_cpld {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+#endif /* __MFD_SONY_CRONOS_H__ */
diff --git a/include/linux/mfd/sony/cronos/registers.h b/include/linux/mfd/sony/cronos/registers.h
new file mode 100644
index 000000000000..2bcc3cf17fe5
--- /dev/null
+++ b/include/linux/mfd/sony/cronos/registers.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2015-2017  Dialog Semiconductor
+ * Copyright (C) 2022  Raptor Engineering, LLC
+ */
+
+#ifndef __SONY_CRONOS_H__
+#define __SONY_CRONOS_H__
+
+#define CRONOS_CPLD_DEVICE_ID		0x0134
+
+/*
+ * Registers and control masks / values
+ */
+
+#define CRONOS_CPLD_REVISION_HIGH_REG			0x73
+#define CRONOS_CPLD_REVISION_LOW_REG			0x72
+#define CRONOS_CPLD_DEVICE_ID_HIGH_REG			0x71
+#define CRONOS_CPLD_DEVICE_ID_LOW_REG			0x70
+
+#define CRONOS_CPLD_BRIGHTNESS_RED_REG			0x17
+#define CRONOS_CPLD_BRIGHTNESS_GREEN_REG		0x18
+#define CRONOS_CPLD_BRIGHTNESS_BLUE_REG			0x19
+
+#define CRONOS_CPLD_LEDS_BRIGHTNESS_SET_MASK		0x7F
+#define CRONOS_LEDS_MAX_BRIGHTNESS			0x7F
+
+#define CRONOS_LEDS_SMC_STATUS_REG			0x10
+#define CRONOS_LEDS_SWITCH_STATUS_REG			0x11
+
+#define CRONOS_LEDS_CCM1_STATUS_REG			0x15
+#define CRONOS_LEDS_CCM2_STATUS_REG			0x13
+#define CRONOS_LEDS_CCM3_STATUS_REG			0x12
+#define CRONOS_LEDS_CCM4_STATUS_REG			0x14
+
+#define CRONOS_LEDS_CCM_POWER_REG			0x16
+
+#define CRONOS_CPLD_UART_MUX_REG			0x0e
+#define CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG	0x00
+#define CRONOS_CPLD_SWITCH_RESET_CMD_REG		0x01
+#define CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG		0x02
+#define CRONOS_CPLD_PAYLOAD_POWER_CTL_REG		0x0a
+#define CRONOS_CPLD_BMC_MAC_LOW_REG			0x30
+#define CRONOS_CPLD_BMC_MAC_HIGH_REG			0x35
+
+#define CRONOS_WDT_CLR_REG		0x03
+#define CRONOS_WDT_CTL_REG		0x0c
+
+#define CRONOS_CPLD_STATUS_2_REG	0x05
+
+#define CRONOS_WDT_CLR_VAL		0xc3
+#define CRONOS_WDT_ENABLE_MASK		0x80
+#define CRONOS_WDT_ENABLE_VAL		0x80
+#define CRONOS_WDT_DISABLE_VAL		0x00
+#define CRONOS_WDT_TIMEOUT_MASK		0x07
+#define CRONOS_WDT_CTL_RESET_VAL	0x00
+
+
+#endif /* __SONY_CRONOS_H__ */
--
2.30.2

Re: [PATCH v2 1/2] dt-bindings: mfd: Add sony,cronos-cpld

[Krzysztof Kozlowski]

On 28/11/2023 22:00, Shawn Anastasio wrote:
> The Sony Cronos Platform Controller CPLD is a multi-purpose platform
> controller that provides both a watchdog timer and an LED controller for
> the Sony Interactive Entertainment Cronos x86 server platform. As both
> functions are provided by the same CPLD, a multi-function device is
> exposed as the parent of both functions.
> 
> Add a DT binding for this device.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> ---
> Changes in v2:
>   - Change SIE to Sony to use the already-established prefix.
>   - Clarify that Cronos is an x86 server platform in description
>   - Drop #address-cells/#size-cells
>   - Add missing additionalProperties to leds/watchdog objects
>   - Add sony,led-mask property to leds object
>   - Add sony,default-timeout property to watchdog object
>   - Update example
> 
>  .../bindings/mfd/sony,cronos-cpld.yaml        | 92 +++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
> new file mode 100644
> index 000000000000..df2c2e83ccb4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Raptor Engineering, LLC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/sony,cronos-cpld.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony Cronos Platform Controller CPLD multi-function device
> +
> +maintainers:
> +  - Timothy Pearson <tpearson@raptorengineering.com>
> +
> +description: |
> +  The Sony Cronos Platform Controller CPLD is a multi-purpose platform
> +  controller that provides both a watchdog timer and an LED controller for the
> +  Sony Interactive Entertainment Cronos x86 server platform. As both functions
> +  are provided by the same CPLD, a multi-function device is exposed as the
> +  parent of both functions.
> +
> +properties:
> +  compatible:
> +    const: sony,cronos-cpld
> +
> +  reg:
> +    maxItems: 1
> +
> +  leds:
> +    type: object
> +    description: Cronos Platform Status LEDs

Missing ref to LEDs common bindings.

> +
> +    properties:
> +      compatible:
> +        const: sony,cronos-leds
> +
> +      sony,led-mask:
> +        $ref: /schemas/types.yaml#/definitions/uint32

Why aren't you using LEDs bindings? A node for one property is otherwise
quite useless. I already commented on this last time.

> +        minimum: 0x0
> +        maximum: 0x7fff
> +        description: |
> +          A bitmask that specifies which LEDs are present and can be controlled
> +          by the Cronos CPLD. Bits 0-5 correspond to platform Status LEDs, bits
> +          6-10 correspond to Link LEDs, and bits 11-14 correspond to the Power
> +          State LEDs. All other bits are unused. The default value is 0x7fff
> +          (all possible LEDs enabled).
> +
> +    additionalProperties: false
> +
> +  watchdog:
> +    type: object
> +    description: Cronos Platform Watchdog Timer


> +
> +    properties:
> +      compatible:
> +        const: sony,cronos-watchdog
> +
> +      sony,default-timeout:

No, you must use existing bindings. Missing ref to watchdog and drop all
duplicated properties like this one.

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: |
> +          The default timeout with which the watchdog timer is initialized, in
> +          seconds. Supported values are: 10, 20, 30, 40, 50, 60, 70, 80. All
> +          other values will be rounded down to the nearest supported value.  The
> +          default value is 80.
> +



Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: mfd: Add sony,cronos-cpld

[Shawn Anastasio]

On 11/29/23 3:23 AM, Krzysztof Kozlowski wrote:
> On 28/11/2023 22:00, Shawn Anastasio wrote:
>> The Sony Cronos Platform Controller CPLD is a multi-purpose platform
>> controller that provides both a watchdog timer and an LED controller for
>> the Sony Interactive Entertainment Cronos x86 server platform. As both
>> functions are provided by the same CPLD, a multi-function device is
>> exposed as the parent of both functions.
>>
>> Add a DT binding for this device.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> ---
>> Changes in v2:
>>   - Change SIE to Sony to use the already-established prefix.
>>   - Clarify that Cronos is an x86 server platform in description
>>   - Drop #address-cells/#size-cells
>>   - Add missing additionalProperties to leds/watchdog objects
>>   - Add sony,led-mask property to leds object
>>   - Add sony,default-timeout property to watchdog object
>>   - Update example
>>
>>  .../bindings/mfd/sony,cronos-cpld.yaml        | 92 +++++++++++++++++++
>>  1 file changed, 92 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
>> new file mode 100644
>> index 000000000000..df2c2e83ccb4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
>> @@ -0,0 +1,92 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2023 Raptor Engineering, LLC
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/sony,cronos-cpld.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sony Cronos Platform Controller CPLD multi-function device
>> +
>> +maintainers:
>> +  - Timothy Pearson <tpearson@raptorengineering.com>
>> +
>> +description: |
>> +  The Sony Cronos Platform Controller CPLD is a multi-purpose platform
>> +  controller that provides both a watchdog timer and an LED controller for the
>> +  Sony Interactive Entertainment Cronos x86 server platform. As both functions
>> +  are provided by the same CPLD, a multi-function device is exposed as the
>> +  parent of both functions.
>> +
>> +properties:
>> +  compatible:
>> +    const: sony,cronos-cpld
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  leds:
>> +    type: object
>> +    description: Cronos Platform Status LEDs
> 
> Missing ref to LEDs common bindings.
>

Will fix.

>> +
>> +    properties:
>> +      compatible:
>> +        const: sony,cronos-leds
>> +
>> +      sony,led-mask:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> Why aren't you using LEDs bindings? A node for one property is otherwise
> quite useless. I already commented on this last time.
>

Our driver as-is doesn't support any of the properties in the LEDs
common bindings, but it doesn't seem like there's anything that would
preclude support in hardware, so this can be fixed.

Will use the LED bindings in v3.

>> +        minimum: 0x0
>> +        maximum: 0x7fff
>> +        description: |
>> +          A bitmask that specifies which LEDs are present and can be controlled
>> +          by the Cronos CPLD. Bits 0-5 correspond to platform Status LEDs, bits
>> +          6-10 correspond to Link LEDs, and bits 11-14 correspond to the Power
>> +          State LEDs. All other bits are unused. The default value is 0x7fff
>> +          (all possible LEDs enabled).
>> +
>> +    additionalProperties: false
>> +
>> +  watchdog:
>> +    type: object
>> +    description: Cronos Platform Watchdog Timer
> 
> 
>> +
>> +    properties:
>> +      compatible:
>> +        const: sony,cronos-watchdog
>> +
>> +      sony,default-timeout:
> 
> No, you must use existing bindings. Missing ref to watchdog and drop all
> duplicated properties like this one.
>

In this case the existing watchdog binding allows for arbitrary timeout
values to be set, but the hardware only tolerates one of a few fixed
values, enumerated below, which is why I felt it was appropriate to use
a vendor-specific binding that documents the supported values.

Would you still prefer we ref to watchdog and just handle unsupported
values in the driver by e.g. rounding or rejecting unsupported values?

>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: |
>> +          The default timeout with which the watchdog timer is initialized, in
>> +          seconds. Supported values are: 10, 20, 30, 40, 50, 60, 70, 80. All
>> +          other values will be rounded down to the nearest supported value.  The
>> +          default value is 80.
>> +
> 
> 
> 
> Best regards,
> Krzysztof
> 

Thanks,
Shawn

Re: [PATCH v2 1/2] dt-bindings: mfd: Add sony,cronos-cpld

[Krzysztof Kozlowski]

On 01/12/2023 00:03, Shawn Anastasio wrote:
>>> +properties:
>>> +  compatible:
>>> +    const: sony,cronos-cpld
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  leds:
>>> +    type: object
>>> +    description: Cronos Platform Status LEDs
>>
>> Missing ref to LEDs common bindings.
>>
> 
> Will fix.
> 
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: sony,cronos-leds
>>> +
>>> +      sony,led-mask:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Why aren't you using LEDs bindings? A node for one property is otherwise
>> quite useless. I already commented on this last time.
>>
> 
> Our driver as-is doesn't support any of the properties in the LEDs
> common bindings, but it doesn't seem like there's anything that would
> preclude support in hardware, so this can be fixed.

Driver does not matter. We talk here about bindings, so about hardware,
not driver.

You must describe here hardware fully, not the driver.

> 
> Will use the LED bindings in v3.
> 
>>> +        minimum: 0x0
>>> +        maximum: 0x7fff
>>> +        description: |
>>> +          A bitmask that specifies which LEDs are present and can be controlled
>>> +          by the Cronos CPLD. Bits 0-5 correspond to platform Status LEDs, bits
>>> +          6-10 correspond to Link LEDs, and bits 11-14 correspond to the Power
>>> +          State LEDs. All other bits are unused. The default value is 0x7fff
>>> +          (all possible LEDs enabled).
>>> +
>>> +    additionalProperties: false
>>> +
>>> +  watchdog:
>>> +    type: object
>>> +    description: Cronos Platform Watchdog Timer
>>
>>
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: sony,cronos-watchdog
>>> +
>>> +      sony,default-timeout:
>>
>> No, you must use existing bindings. Missing ref to watchdog and drop all
>> duplicated properties like this one.
>>
> 
> In this case the existing watchdog binding allows for arbitrary timeout
> values to be set, but the hardware only tolerates one of a few fixed
> values, enumerated below, which is why I felt it was appropriate to use
> a vendor-specific binding that documents the supported values.

You can narrow the accepted values.

> 
> Would you still prefer we ref to watchdog and just handle unsupported
> values in the driver by e.g. rounding or rejecting unsupported values?

It's not preference, it's a must.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] mfd: sie-cronos-cpld: Add driver for Sony Cronos CPLD

[Lee Jones]

On Tue, 28 Nov 2023, Shawn Anastasio wrote:

> From: Timothy Pearson <tpearson@raptorengineering.com>
> 
> The Sony Cronos Platform Controller CPLD is a multi-purpose platform
> controller that provides both a watchdog timer and an LED controller for
> the Sony Interactive Entertainment Cronos x86 server platform. As both
> functions are provided by the same CPLD, a multi-function device is
> exposed as the parent of both functions.
> 
> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> ---
> Changes in v2:
>   - Change SIE to Sony (SIE's parent company) to be more consistent
>   with how other subsidiaries are treated in the kernel
>   - Fix build issue under !CONFIG_OF discovered by kernel test robot

Does this driver work without Device Tree?

Why can't you just drop the of_match_ptr()?

>   by guarding definition of `cronos_cpld_dt_ids` as is done in other
>   drivers.
> 
>  MAINTAINERS                               |   7 +
>  drivers/mfd/Kconfig                       |  11 +
>  drivers/mfd/Makefile                      |   1 +
>  drivers/mfd/sony-cronos-cpld.c            | 591 ++++++++++++++++++++++
>  include/linux/mfd/sony/cronos/core.h      |  17 +
>  include/linux/mfd/sony/cronos/registers.h |  59 +++
>  6 files changed, 686 insertions(+)
>  create mode 100644 drivers/mfd/sony-cronos-cpld.c
>  create mode 100644 include/linux/mfd/sony/cronos/core.h
>  create mode 100644 include/linux/mfd/sony/cronos/registers.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6c4cce45a09d..623681826820 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19932,6 +19932,13 @@ S:	Maintained
>  F:	drivers/ssb/
>  F:	include/linux/ssb/
> 
> +SONY CRONOS CPLD DRIVER
> +M:	Georgy Yakovlev <Georgy.Yakovlev@sony.com>

Are they aware of this?

They do not appear to be in the submission path.

> +S:	Maintained

This should probably be Supported.

> +F:	Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
> +F:	drivers/mfd/sony-cronos-cpld.c
> +F:	include/linux/mfd/sony/cronos/
> +
>  SONY IMX208 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 90ce58fd629e..27f28fbbc7cc 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2217,6 +2217,17 @@ config MFD_QCOM_PM8008
>  	  under it in the device tree. Additional drivers must be enabled in
>  	  order to use the functionality of the device.
> 
> +config MFD_SONY_CRONOS_CPLD
> +	tristate "Sony Cronos CPLD Support"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C
> +	help
> +      Support for the Sony Cronos system control CPLDs. Additional drivers must
> +      be enabled in order to use the functionality of the device, including LED
> +      control and the system watchdog. The controller itself is a custom design
> +      tailored to the specific needs of the Sony Cronos hardware platform.
> +
>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
> 
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..be9974f0fe9c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -284,3 +284,4 @@ rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
>  rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
>  obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
>  obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
> +obj-$(CONFIG_MFD_SONY_CRONOS_CPLD)	+= sony-cronos-cpld.o
> diff --git a/drivers/mfd/sony-cronos-cpld.c b/drivers/mfd/sony-cronos-cpld.c
> new file mode 100644
> index 000000000000..569793cd9697
> --- /dev/null
> +++ b/drivers/mfd/sony-cronos-cpld.c
> @@ -0,0 +1,591 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * I2C device driver for Sony Cronos CPLDs

This is not an I2C device driver.

> + * Copyright (C) 2015-2017  Dialog Semiconductor
> + * Copyright (C) 2022  Raptor Engineering, LLC

No changes since 2022?

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/core.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/sony/cronos/core.h>
> +#include <linux/mfd/sony/cronos/registers.h>

Alphabetical.

> +static struct resource cronos_wdt_resources[] = {
> +};
> +
> +static struct resource cronos_led_resources[] = {
> +};

What is the point of these?

> +static const struct mfd_cell cronos_cpld_devs[] = {
> +	{
> +		.name          = "cronos-watchdog",
> +		.num_resources = ARRAY_SIZE(cronos_wdt_resources),
> +		.resources     = cronos_wdt_resources,
> +		.of_compatible = "sony,cronos-watchdog",
> +	},
> +	{
> +		.name          = "cronos-leds",
> +		.id            = 1,

Why are you manually numbering them?

> +		.num_resources = ARRAY_SIZE(cronos_led_resources),
> +		.resources     = cronos_led_resources,
> +		.of_compatible = "sony,cronos-leds",
> +	},
> +};
> +
> +static ssize_t payload_power_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int payloadpower_val = 0;

Ironically, the "_val" provides no value here.

> +	int ret = -EIO;

Why is this being pre-initialised?  Same throughout.

> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);

Move this line to the top - same throughout.

> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_PAYLOAD_POWER_CTL_REG, &payloadpower_val);
> +	if (ret < 0)

if (ret) is fine.  For all cases below too.

> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", payloadpower_val);

sysfs_emit().  For all cases below too.

> +}
> +
> +static ssize_t payload_power_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t len)
> +{
> +	u8 val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	if (kstrtou8(buf, 0, &val))
> +		return -EINVAL;
> +
> +	ret = regmap_write(chip->regmap, CRONOS_CPLD_PAYLOAD_POWER_CTL_REG, val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
> +			val, CRONOS_CPLD_PAYLOAD_POWER_CTL_REG);

Why dump an error messages here and not in other places?

I suggest they are dropped.

> +		return ret;
> +	}
> +	return len;
> +}
> +
> +
> +static ssize_t bmc_flash_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int bmcflash_val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG, &bmcflash_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", bmcflash_val);
> +}
> +
> +static ssize_t bmc_flash_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t len)
> +{
> +	u8 val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	if (kstrtou8(buf, 0, &val))
> +		return -EINVAL;
> +
> +	ret = regmap_write(chip->regmap, CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG, val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
> +			val, CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG);
> +		return ret;
> +	}
> +	return len;
> +}
> +
> +
> +static ssize_t switch_reset_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int switchreset_val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_SWITCH_RESET_CMD_REG, &switchreset_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", switchreset_val);
> +}
> +
> +static ssize_t switch_reset_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	unsigned int switchreset_val = 0;
> +	u8 val = -EINVAL;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	if (kstrtou8(buf, 0, &val))
> +		return -EINVAL;
> +
> +	if (val != 1)
> +		return -EINVAL;
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_SWITCH_RESET_CMD_REG, &switchreset_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(chip->regmap, CRONOS_CPLD_SWITCH_RESET_CMD_REG, switchreset_val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
> +				switchreset_val, CRONOS_CPLD_SWITCH_RESET_CMD_REG);
> +		return ret;
> +	}
> +	return len;
> +}
> +
> +
> +static ssize_t switch_flash_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int switchflash_val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG, &switchflash_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", switchflash_val);
> +}
> +
> +static ssize_t switch_flash_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	u8 val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	if (kstrtou8(buf, 0, &val))
> +		return -EINVAL;
> +
> +	ret = regmap_write(chip->regmap, CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG, val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
> +			val, CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG);
> +		return ret;
> +	}
> +	return len;
> +}
> +
> +
> +static ssize_t uart_mux_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int uartmux_val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_UART_MUX_REG, &uartmux_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", uartmux_val);
> +}
> +
> +static ssize_t uart_mux_store(struct device *dev,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t len)
> +{
> +	u8 val = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	if (kstrtou8(buf, 0, &val))
> +		return -EINVAL;
> +
> +	ret = regmap_write(chip->regmap, CRONOS_CPLD_UART_MUX_REG, val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write value 0x%02x to address 0x%02x",
> +			val, CRONOS_CPLD_UART_MUX_REG);
> +		return ret;
> +	}
> +	return len;
> +}
> +
> +
> +static ssize_t led_get_brightness(struct sony_cronos_cpld *chip, unsigned int reg, char *buf)
> +{
> +	unsigned int brightness_val;
> +	int ret = -EIO;
> +
> +	ret = regmap_read(chip->regmap, reg, &brightness_val);
> +	if (ret != 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", brightness_val);
> +}
> +
> +static ssize_t led_set_brightness(struct sony_cronos_cpld *chip, unsigned int reg, const char *buf,
> +	size_t len)
> +{
> +	u8 val = 0;
> +	int ret = -EIO;
> +
> +	if (kstrtou8(buf, 0, &val))
> +		return -EINVAL;
> +
> +	ret = regmap_update_bits(chip->regmap, reg, CRONOS_CPLD_LEDS_BRIGHTNESS_SET_MASK, val);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to write value 0x%02x to address 0x%02x", val, reg);
> +		return ret;
> +	}
> +	return len;
> +}
> +
> +static ssize_t brightness_red_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	return led_get_brightness(chip, CRONOS_CPLD_BRIGHTNESS_RED_REG, buf);
> +}
> +
> +static ssize_t brightness_red_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t len)
> +{
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	return led_set_brightness(chip, CRONOS_CPLD_BRIGHTNESS_RED_REG, buf, len);
> +}
> +
> +static ssize_t brightness_green_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	return led_get_brightness(chip, CRONOS_CPLD_BRIGHTNESS_GREEN_REG, buf);
> +}
> +
> +static ssize_t brightness_green_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t len)
> +{
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	return led_set_brightness(chip, CRONOS_CPLD_BRIGHTNESS_GREEN_REG, buf, len);
> +}
> +
> +static ssize_t brightness_blue_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	return led_get_brightness(chip, CRONOS_CPLD_BRIGHTNESS_BLUE_REG, buf);
> +}
> +
> +static ssize_t brightness_blue_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t len)
> +{
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	return led_set_brightness(chip, CRONOS_CPLD_BRIGHTNESS_BLUE_REG, buf, len);
> +}

All of the LED related code should be in the LED driver.

> +
> +

No double line spacings.

> +static ssize_t revision_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	u16 revision = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_bulk_read(chip->regmap, CRONOS_CPLD_REVISION_LOW_REG, &revision, 2);
> +	if (ret)
> +		return -EIO;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%04x\n", revision);
> +}
> +
> +static ssize_t device_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	u16 device_id = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_bulk_read(chip->regmap, CRONOS_CPLD_DEVICE_ID_LOW_REG, &device_id, 2);
> +	if (ret)
> +		return -EIO;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%04x\n", device_id);
> +}
> +
> +static ssize_t bmc_mac_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	u8 bmc_mac[6];
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_bulk_read(chip->regmap, CRONOS_CPLD_BMC_MAC_LOW_REG, bmc_mac, 6);
> +	if (ret)
> +		return -EIO;
> +
> +	return snprintf(buf, PAGE_SIZE, "%pM\n", bmc_mac);
> +}
> +
> +static ssize_t status_2_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int last_boot = 0;
> +	int ret = -EIO;
> +	struct sony_cronos_cpld *chip = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_STATUS_2_REG, &last_boot);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%02x\n", last_boot);
> +}
> +
> +
> +static DEVICE_ATTR_RO(revision);
> +static DEVICE_ATTR_RO(device_id);
> +static DEVICE_ATTR_RO(bmc_mac);
> +static DEVICE_ATTR_RO(status_2);
> +
> +static DEVICE_ATTR_RW(uart_mux);
> +static DEVICE_ATTR_RW(switch_flash);
> +static DEVICE_ATTR_RW(switch_reset);
> +static DEVICE_ATTR_RW(bmc_flash);
> +static DEVICE_ATTR_RW(payload_power);
> +
> +static DEVICE_ATTR_RW(brightness_red);
> +static DEVICE_ATTR_RW(brightness_green);
> +static DEVICE_ATTR_RW(brightness_blue);

'\n'

> +static struct attribute *cronos_cpld_sysfs_entries[] = {
> +	&dev_attr_revision.attr,
> +	&dev_attr_device_id.attr,
> +	&dev_attr_bmc_mac.attr,
> +	&dev_attr_status_2.attr,
> +	&dev_attr_uart_mux.attr,
> +	&dev_attr_switch_flash.attr,
> +	&dev_attr_switch_reset.attr,
> +	&dev_attr_bmc_flash.attr,
> +	&dev_attr_payload_power.attr,
> +	&dev_attr_brightness_red.attr,
> +	&dev_attr_brightness_green.attr,
> +	&dev_attr_brightness_blue.attr,
> +	NULL,
> +};

These all need to be documented or dropped.

Please read:

  Documentation/filesystems/sysfs.rst

And do:

  `find Documentation -name *sysfs*`

> +static const struct attribute_group cronos_cpld_attr_group = {
> +	.attrs	= cronos_cpld_sysfs_entries,
> +};
> +
> +static int sony_cronos_get_device_type(struct sony_cronos_cpld *chip)
> +{
> +	int device_id;
> +	int byte;

Introduce 2 variables and do all of the bit manipulation at the bottom.

> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_DEVICE_ID_HIGH_REG, &byte);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Cannot read chip ID.\n");
> +		return -EIO;
> +	}

Spread these out with '\n's.

> +	device_id = byte << 8;
> +	ret = regmap_read(chip->regmap, CRONOS_CPLD_DEVICE_ID_LOW_REG, &byte);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Cannot read chip ID.\n");
> +		return -EIO;
> +	}
> +	device_id |= byte;
> +	if (device_id != CRONOS_CPLD_DEVICE_ID) {
> +		dev_err(chip->dev, "Invalid device ID: 0x%04x\n", device_id);

"Unsupported"

> +		return -ENODEV;
> +	}
> +
> +	dev_info(chip->dev,
> +		 "Device detected (device-ID: 0x%04X)\n",
> +		 device_id);

Why line break here?  The lines above are way longer.

> +	return ret;
> +}
> +
> +static bool cronos_cpld_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case CRONOS_CPLD_BRIGHTNESS_RED_REG:
> +	case CRONOS_CPLD_BRIGHTNESS_GREEN_REG:
> +	case CRONOS_CPLD_BRIGHTNESS_BLUE_REG:
> +	case CRONOS_LEDS_SMC_STATUS_REG:
> +	case CRONOS_LEDS_SWITCH_STATUS_REG:
> +	case CRONOS_LEDS_CCM1_STATUS_REG:
> +	case CRONOS_LEDS_CCM2_STATUS_REG:
> +	case CRONOS_LEDS_CCM3_STATUS_REG:
> +	case CRONOS_LEDS_CCM4_STATUS_REG:
> +	case CRONOS_LEDS_CCM_POWER_REG:
> +
> +	case CRONOS_WDT_CTL_REG:
> +	case CRONOS_WDT_CLR_REG:
> +
> +	case CRONOS_CPLD_UART_MUX_REG:
> +	case CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG:
> +	case CRONOS_CPLD_SWITCH_RESET_CMD_REG:
> +	case CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG:
> +	case CRONOS_CPLD_PAYLOAD_POWER_CTL_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool cronos_cpld_is_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case CRONOS_CPLD_REVISION_HIGH_REG:
> +	case CRONOS_CPLD_REVISION_LOW_REG:
> +	case CRONOS_CPLD_DEVICE_ID_HIGH_REG:
> +	case CRONOS_CPLD_DEVICE_ID_LOW_REG:
> +
> +	case CRONOS_CPLD_BRIGHTNESS_RED_REG:
> +	case CRONOS_CPLD_BRIGHTNESS_GREEN_REG:
> +	case CRONOS_CPLD_BRIGHTNESS_BLUE_REG:
> +	case CRONOS_LEDS_SMC_STATUS_REG:
> +	case CRONOS_LEDS_SWITCH_STATUS_REG:
> +	case CRONOS_LEDS_CCM1_STATUS_REG:
> +	case CRONOS_LEDS_CCM2_STATUS_REG:
> +	case CRONOS_LEDS_CCM3_STATUS_REG:
> +	case CRONOS_LEDS_CCM4_STATUS_REG:
> +	case CRONOS_LEDS_CCM_POWER_REG:
> +
> +	case CRONOS_WDT_CTL_REG:
> +	case CRONOS_WDT_CLR_REG:
> +
> +	case CRONOS_CPLD_STATUS_2_REG:
> +	case CRONOS_CPLD_UART_MUX_REG:
> +	case CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG:
> +	case CRONOS_CPLD_SWITCH_RESET_CMD_REG:
> +	case CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG:
> +	case CRONOS_CPLD_PAYLOAD_POWER_CTL_REG:
> +
> +	case CRONOS_CPLD_BMC_MAC_LOW_REG ... CRONOS_CPLD_BMC_MAC_HIGH_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool cronos_cpld_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case CRONOS_CPLD_REVISION_HIGH_REG:
> +	case CRONOS_CPLD_REVISION_LOW_REG:
> +
> +	case CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG:
> +	case CRONOS_CPLD_SWITCH_RESET_CMD_REG:
> +	case CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG:
> +	case CRONOS_CPLD_PAYLOAD_POWER_CTL_REG:
> +
> +	case CRONOS_WDT_CTL_REG:
> +	case CRONOS_WDT_CLR_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static struct regmap_config cronos_cpld_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = CRONOS_CPLD_REVISION_HIGH_REG,
> +	.writeable_reg = cronos_cpld_is_writeable_reg,
> +	.readable_reg = cronos_cpld_is_readable_reg,
> +	.volatile_reg = cronos_cpld_is_volatile_reg,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +	.cache_type = REGCACHE_RBTREE,

This is being phased out for the Maple Tree.

> +};
> +
> +#ifdef CONFIG_OF

These are ugly and shouldn't be required.

> +static const struct of_device_id cronos_cpld_dt_ids[] = {
> +	{ .compatible = "sony,cronos-cpld", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cronos_cpld_dt_ids);
> +#endif
> +
> +static int sony_cronos_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct sony_cronos_cpld *chip;

Chip is wrong here.  This should be ddata.

> +	const struct of_device_id *match;
> +	const struct mfd_cell *cell;

Why do you need this?

> +	const struct regmap_config *config;
> +	int cell_num;
> +	int ret;
> +
> +	chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	if (i2c->dev.of_node) {
> +		match = of_match_node(cronos_cpld_dt_ids, i2c->dev.of_node);

Why are you doing this?

> +		if (!match)
> +			return -EINVAL;
> +	}
> +
> +	i2c_set_clientdata(i2c, chip);

Do this *after* the struct has been populated.

> +	chip->dev = &i2c->dev;
> +
> +	cell = cronos_cpld_devs;
> +	cell_num = ARRAY_SIZE(cronos_cpld_devs);
> +	config = &cronos_cpld_regmap_config;

This is bananas.  Why the intermediary variables?

> +	chip->regmap = devm_regmap_init_i2c(i2c, config);
> +	if (IS_ERR(chip->regmap)) {
> +		ret = PTR_ERR(chip->regmap);
> +		dev_err(chip->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;

dev_err_probe()

> +	}
> +
> +	ret = sony_cronos_get_device_type(chip);
> +	if (ret)
> +		return ret;
> +
> +	ret = mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE, cell,

Why not AUTO?

> +			      cell_num, NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(chip->dev, "Cannot register child devices\n");
> +		return ret;
> +	}
> +
> +	/* Add sysfs */

This doesn't add anything.

> +	ret = sysfs_create_group(&chip->dev->kobj, &cronos_cpld_attr_group);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to create sysfs entries\n");
> +
> +	return ret;
> +}
> +
> +static void sony_cronos_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct sony_cronos_cpld *chip = i2c_get_clientdata(i2c);
> +
> +	sysfs_remove_group(&chip->dev->kobj, &cronos_cpld_attr_group);
> +	mfd_remove_devices(chip->dev);

Use devm_* instead.

> +}
> +
> +static struct i2c_driver sony_cronos_i2c_driver = {
> +	.driver = {
> +		.name = "sony-cronos",

It's 'cpld' everywhere else.  Why the change of heart?

> +		.of_match_table = of_match_ptr(cronos_cpld_dt_ids),
> +	},
> +	.probe    = sony_cronos_i2c_probe,
> +	.remove   = sony_cronos_i2c_remove,
> +};
> +

Remove this line.

> +module_i2c_driver(sony_cronos_i2c_driver);
> +
> +MODULE_DESCRIPTION("Core device driver for sony Cronos CPLDs");

First mention of Core too.

"Sony"

> +MODULE_AUTHOR("Raptor Engineering, LLC <support@raptorengineering.com>");

Your 'support' department did not author this driver.

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/sony/cronos/core.h b/include/linux/mfd/sony/cronos/core.h
> new file mode 100644
> index 000000000000..6f80b90af5ca
> --- /dev/null
> +++ b/include/linux/mfd/sony/cronos/core.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2015-2017  Dialog Semiconductor
> + * Copyright (C) 2022  Raptor Engineering, LLC
> + */
> +
> +#ifndef __MFD_SONY_CRONOS_CORE_H__
> +#define __MFD_SONY_CRONOS_CORE_H__
> +
> +#include <linux/mfd/sony/cronos/registers.h>
> +
> +struct sony_cronos_cpld {
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +#endif /* __MFD_SONY_CRONOS_H__ */
> diff --git a/include/linux/mfd/sony/cronos/registers.h b/include/linux/mfd/sony/cronos/registers.h
> new file mode 100644
> index 000000000000..2bcc3cf17fe5
> --- /dev/null
> +++ b/include/linux/mfd/sony/cronos/registers.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2015-2017  Dialog Semiconductor
> + * Copyright (C) 2022  Raptor Engineering, LLC
> + */
> +
> +#ifndef __SONY_CRONOS_H__
> +#define __SONY_CRONOS_H__
> +
> +#define CRONOS_CPLD_DEVICE_ID		0x0134
> +
> +/*
> + * Registers and control masks / values
> + */
> +
> +#define CRONOS_CPLD_REVISION_HIGH_REG			0x73
> +#define CRONOS_CPLD_REVISION_LOW_REG			0x72
> +#define CRONOS_CPLD_DEVICE_ID_HIGH_REG			0x71
> +#define CRONOS_CPLD_DEVICE_ID_LOW_REG			0x70
> +
> +#define CRONOS_CPLD_BRIGHTNESS_RED_REG			0x17
> +#define CRONOS_CPLD_BRIGHTNESS_GREEN_REG		0x18
> +#define CRONOS_CPLD_BRIGHTNESS_BLUE_REG			0x19
> +
> +#define CRONOS_CPLD_LEDS_BRIGHTNESS_SET_MASK		0x7F
> +#define CRONOS_LEDS_MAX_BRIGHTNESS			0x7F
> +
> +#define CRONOS_LEDS_SMC_STATUS_REG			0x10
> +#define CRONOS_LEDS_SWITCH_STATUS_REG			0x11
> +
> +#define CRONOS_LEDS_CCM1_STATUS_REG			0x15
> +#define CRONOS_LEDS_CCM2_STATUS_REG			0x13
> +#define CRONOS_LEDS_CCM3_STATUS_REG			0x12
> +#define CRONOS_LEDS_CCM4_STATUS_REG			0x14
> +
> +#define CRONOS_LEDS_CCM_POWER_REG			0x16
> +
> +#define CRONOS_CPLD_UART_MUX_REG			0x0e
> +#define CRONOS_CPLD_SWITCH_BOOT_FLASH_SELECT_REG	0x00
> +#define CRONOS_CPLD_SWITCH_RESET_CMD_REG		0x01
> +#define CRONOS_CPLD_BMC_BOOT_FLASH_SELECT_REG		0x02
> +#define CRONOS_CPLD_PAYLOAD_POWER_CTL_REG		0x0a
> +#define CRONOS_CPLD_BMC_MAC_LOW_REG			0x30
> +#define CRONOS_CPLD_BMC_MAC_HIGH_REG			0x35
> +
> +#define CRONOS_WDT_CLR_REG		0x03
> +#define CRONOS_WDT_CTL_REG		0x0c
> +
> +#define CRONOS_CPLD_STATUS_2_REG	0x05
> +
> +#define CRONOS_WDT_CLR_VAL		0xc3
> +#define CRONOS_WDT_ENABLE_MASK		0x80
> +#define CRONOS_WDT_ENABLE_VAL		0x80
> +#define CRONOS_WDT_DISABLE_VAL		0x00
> +#define CRONOS_WDT_TIMEOUT_MASK		0x07
> +#define CRONOS_WDT_CTL_RESET_VAL	0x00
> +
> +
> +#endif /* __SONY_CRONOS_H__ */
> --
> 2.30.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] mfd: sie-cronos-cpld: Add driver for Sony Cronos CPLD

[Yakovlev, Georgy]

On Thu, 2023-12-07 at 15:20 +0000, Lee Jones wrote:
> On Tue, 28 Nov 2023, Shawn Anastasio wrote:
> 
> > From: Timothy Pearson <tpearson@raptorengineering.com>
> > 
> > The Sony Cronos Platform Controller CPLD is a multi-purpose
> > platform
> > controller that provides both a watchdog timer and an LED
> > controller for
> > the Sony Interactive Entertainment Cronos x86 server platform. As
> > both
> > functions are provided by the same CPLD, a multi-function device is
> > exposed as the parent of both functions.
> > 
> > Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> > Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> > ---
> > Changes in v2:
> >   - Change SIE to Sony (SIE's parent company) to be more consistent
> >   with how other subsidiaries are treated in the kernel
> >   - Fix build issue under !CONFIG_OF discovered by kernel test
> > robot
> 
> Does this driver work without Device Tree?
> 
> Why can't you just drop the of_match_ptr()?
> 
> >   by guarding definition of `cronos_cpld_dt_ids` as is done in
> > other
> >   drivers.
> > 
> >  MAINTAINERS                               |   7 +
> >  drivers/mfd/Kconfig                       |  11 +
> >  drivers/mfd/Makefile                      |   1 +
> >  drivers/mfd/sony-cronos-cpld.c            | 591
> > ++++++++++++++++++++++
> >  include/linux/mfd/sony/cronos/core.h      |  17 +
> >  include/linux/mfd/sony/cronos/registers.h |  59 +++
> >  6 files changed, 686 insertions(+)
> >  create mode 100644 drivers/mfd/sony-cronos-cpld.c
> >  create mode 100644 include/linux/mfd/sony/cronos/core.h
> >  create mode 100644 include/linux/mfd/sony/cronos/registers.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6c4cce45a09d..623681826820 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19932,6 +19932,13 @@ S:     Maintained
> >  F:     drivers/ssb/
> >  F:     include/linux/ssb/
> > 
> > +SONY CRONOS CPLD DRIVER
> > +M:     Georgy Yakovlev <Georgy.Yakovlev@sony.com>
> 
> Are they aware of this?
> 
> They do not appear to be in the submission path.

Hello,

I'm aware, just watching and learning.

> 
> > +S:     Maintained
<rest snipped>