Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

On Wed, Jul 21, 2021 at 12:53 AM Lee Jones [off-list ref] wrote:

On Thu, 08 Jul 2021, Alistair Francis wrote:

quoted

Initial support for the Silergy SY7636A Power Management chip.

Signed-off-by: Alistair Francis <redacted>
---
 drivers/mfd/Kconfig         |  9 +++++
 drivers/mfd/Makefile        |  1 +
 drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
 4 files changed, 138 insertions(+)
 create mode 100644 drivers/mfd/sy7636a.c
 create mode 100644 include/linux/mfd/sy7636a.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6a3fd2d75f96..7b59aa0fd3f2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig

@@ -1352,6 +1352,15 @@ config MFD_SYSCON
        Select this option to enable accessing system control registers
        via regmap.

+config MFD_SY7636A
+     tristate "Silergy SY7636A Power Management chip"

s/chip/IC/

quoted

+     select MFD_CORE
+     select REGMAP_I2C
+     depends on I2C
+     help
+       Select this option to enable support for the Silergy SY7636A
+       Power Management chip.

As above.

quoted

 config MFD_DAVINCI_VOICECODEC
      tristate
      select MFD_CORE

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8116c19d5fd4..cbe581e87fa9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile

@@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_KHADAS_MCU)      += khadas-mcu.o
 obj-$(CONFIG_MFD_ACER_A500_EC)       += acer-ec-a500.o
 obj-$(CONFIG_MFD_QCOM_PM8008)        += qcom-pm8008.o

+obj-$(CONFIG_MFD_SY7636A)    += sy7636a.o
 obj-$(CONFIG_SGI_MFD_IOC3)   += ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)     += simple-mfd-i2c.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o

diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
new file mode 100644
index 000000000000..345892e11221
--- /dev/null
+++ b/drivers/mfd/sy7636a.c

@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// MFD parent driver for SY7636A chip
+//
+// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
+//
+// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+//          Alistair Francis <alistair@alistair23.me>

Only C++ comments for the SPDX please.

quoted

+// Based on the lp87565 driver by Keerthy [off-list ref]
+
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <linux/mfd/sy7636a.h>
+
+static const struct regmap_config sy7636a_regmap_config = {
+     .reg_bits = 8,
+     .val_bits = 8,
+};
+
+static const struct mfd_cell sy7636a_cells[] = {
+     { .name = "sy7636a-regulator", },
+     { .name = "sy7636a-temperature", },
+     { .name = "sy7636a-thermal", },
+};

If you put these in the Device Tree, you can use "simple-mfd-i2c"

quoted

+static const struct of_device_id of_sy7636a_match_table[] = {
+     { .compatible = "silergy,sy7636a", },
+     {}
+};
+MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);

Place this near the i2c_device_id table please.

quoted

+static int sy7636a_probe(struct i2c_client *client,
+                      const struct i2c_device_id *ids)
+{
+     struct sy7636a *sy7636a;

Please call this ddata.

quoted

+     int ret;
+
+     sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);
+     if (!sy7636a)
+             return -ENOMEM;
+
+     sy7636a->dev = &client->dev;

What are you using 'dev' for?

You can normally just use 'dev->parent' from the child device.

I didn't realise that, I have removed `dev`.

quoted

+     sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
+     if (IS_ERR(sy7636a->regmap)) {
+             ret = PTR_ERR(sy7636a->regmap);
+             dev_err(sy7636a->dev,
+                     "Failed to initialize register map: %d\n", ret);
+             return ret;
+     }
+
+     i2c_set_clientdata(client, sy7636a);
+
+     return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
+                                     sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
+                                     NULL, 0, NULL);
+}
+
+static const struct i2c_device_id sy7636a_id_table[] = {
+     { "sy7636a", 0 },
+     { },
+};
+MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);

Use probe_new below, then you can omit this table.

quoted

+static struct i2c_driver sy7636a_driver = {
+     .driver = {
+             .name   = "sy7636a",
+             .of_match_table = of_sy7636a_match_table,
+     },
+     .probe = sy7636a_probe,
+     .id_table = sy7636a_id_table,
+};
+module_i2c_driver(sy7636a_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig [off-list ref]");
+MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
+MODULE_LICENSE("GPL v2");

diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
new file mode 100644
index 000000000000..b6845a3572b8
--- /dev/null
+++ b/include/linux/mfd/sy7636a.h

@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Functions to access SY3686A power management chip.
+ *
+ * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
+ */
+
+#ifndef __MFD_SY7636A_H
+#define __MFD_SY7636A_H
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define SY7636A_REG_OPERATION_MODE_CRL               0x00
+#define SY7636A_OPERATION_MODE_CRL_VCOMCTL   BIT(6)
+#define SY7636A_OPERATION_MODE_CRL_ONOFF     BIT(7)
+#define SY7636A_REG_VCOM_ADJUST_CTRL_L               0x01
+#define SY7636A_REG_VCOM_ADJUST_CTRL_H               0x02
+#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK    0x01ff
+#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL        0x03
+#define SY7636A_REG_POWER_ON_DELAY_TIME              0x06
+#define SY7636A_REG_FAULT_FLAG                       0x07
+#define SY7636A_FAULT_FLAG_PG                        BIT(0)
+#define SY7636A_REG_TERMISTOR_READOUT                0x08
+
+#define SY7636A_REG_MAX                              0x08
+
+#define VCOM_MIN             0
+#define VCOM_MAX             5000
+
+#define VCOM_ADJUST_CTRL_MASK        0x1ff
+// Used to shift the high byte
+#define VCOM_ADJUST_CTRL_SHIFT       8
+// Used to scale from VCOM_ADJUST_CTRL to mv
+#define VCOM_ADJUST_CTRL_SCAL        10000
+
+#define FAULT_FLAG_SHIFT     1
+
+struct sy7636a {
+     struct device *dev;
+     struct regmap *regmap;
+     struct gpio_desc *pgood_gpio;

This looks unused?

It is used in the syy636a-regulator

Alistair

quoted

+};
+
+#endif /* __LINUX_MFD_SY7636A_H */

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog