Initial support for the Silergy SY7636A Power Management chip
driver.
Signed-off-by: Alistair Francis <[email protected]>
---
.../bindings/mfd/silergy,sy7636a.yaml | 37 +++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
new file mode 100644
index 000000000000..37541a7fcc5d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/silergy,sy7636a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: silergy sy7636a PMIC
+
+maintainers:
+ - Alistair Francis <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - silergy,sy7636a
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ regulator@60 {
+ compatible = "silergy,sy7636a";
+ reg = <0x60>;
+ };
+ };
+
+...
--
2.29.2
Enable the silergy,sy7636a and silergy,sy7636a-regulator on the
reMarkable2.
Signed-off-by: Alistair Francis <[email protected]>
---
arch/arm/boot/dts/imx7d-remarkable2.dts | 61 +++++++++++++++++++++++++
arch/arm/configs/remarkable2_defconfig | 2 +
2 files changed, 63 insertions(+)
diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index 8052d884a5e5..f419ab704f06 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -58,6 +58,27 @@ memory {
reg = <0x80000000 0x40000000>;
};
+ thermal-zones {
+ epd-thermal {
+ thermal-sensors = <&epd_pmic>;
+ polling-delay-passive = <30000>;
+ polling-delay = <30000>;
+ trips {
+ trip0 {
+ temperature = <49000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+
+ trip1 {
+ temperature = <50000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+ };
+ };
+
reg_vref_1v8: regulator-vref-1v8 {
compatible = "regulator-fixed";
regulator-name = "vref-1v8";
@@ -174,6 +195,32 @@ digitizer: wacom-i2c@9 {
};
};
+&i2c4 {
+ clock-frequency = <100000>;
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&pinctrl_i2c4>;
+ pinctrl-1 = <&pinctrl_i2c4>;
+ status = "okay";
+
+ epd_pmic: sy7636a@62 {
+ compatible = "silergy,sy7636a";
+ reg = <0x62>;
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_epdpmic>;
+ #thermal-sensor-cells = <0>;
+
+ epd-pwr-good-gpios = <&gpio6 21 GPIO_ACTIVE_HIGH>;
+ regulators {
+ compatible = "silergy,sy7636a-regulator";
+ reg_epdpmic: vcom {
+ regulator-name = "vcom";
+ regulator-boot-on;
+ };
+ };
+ };
+};
+
&sdma {
status = "okay";
};
@@ -277,6 +324,13 @@ MX7D_PAD_I2C1_SCL__I2C1_SCL 0x4000007f
>;
};
+ pinctrl_i2c4: i2c4grp {
+ fsl,pins = <
+ MX7D_PAD_I2C4_SDA__I2C4_SDA 0x4000007f
+ MX7D_PAD_I2C4_SCL__I2C4_SCL 0x4000007f
+ >;
+ };
+
pinctrl_uart1: uart1grp {
fsl,pins = <
MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX 0x79
@@ -389,6 +443,13 @@ MX7D_PAD_SD1_WP__CCM_CLKO2 0x00000014
>;
};
+ pinctrl_epdpmic: epdpmicgrp {
+ fsl,pins = <
+ MX7D_PAD_SAI2_RX_DATA__GPIO6_IO21 0x00000074
+ MX7D_PAD_ENET1_RGMII_TXC__GPIO7_IO11 0x00000014
+ >;
+ };
+
pinctrl_wdog: wdoggrp {
fsl,pins = <
MX7D_PAD_ENET1_COL__WDOG1_WDOG_ANY 0x74
diff --git a/arch/arm/configs/remarkable2_defconfig b/arch/arm/configs/remarkable2_defconfig
index 8c9785555cda..6306568772c3 100644
--- a/arch/arm/configs/remarkable2_defconfig
+++ b/arch/arm/configs/remarkable2_defconfig
@@ -193,6 +193,7 @@ CONFIG_MFD_MC13XXX_SPI=y
CONFIG_MFD_MC13XXX_I2C=y
CONFIG_MFD_SI476X_CORE=y
CONFIG_MFD_STMPE=y
+CONFIG_MFD_SY7636A=y
CONFIG_REGULATOR=y
CONFIG_REGULATOR_FIXED_VOLTAGE=y
CONFIG_REGULATOR_ANATOP=y
@@ -201,6 +202,7 @@ CONFIG_REGULATOR_GPIO=y
CONFIG_REGULATOR_MC13783=y
CONFIG_REGULATOR_MC13892=y
CONFIG_REGULATOR_PFUZE100=y
+CONFIG_REGULATOR_SY7636A=y
CONFIG_MEDIA_SUPPORT=y
CONFIG_MEDIA_USB_SUPPORT=y
CONFIG_USB_VIDEO_CLASS=m
--
2.29.2
Initial support for the Silergy SY7636A-regulator Power Management chip
driver.
Signed-off-by: Alistair Francis <[email protected]>
---
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
drivers/regulator/sy7636a-regulator.c | 233 ++++++++++++++++++++++++++
3 files changed, 240 insertions(+)
create mode 100644 drivers/regulator/sy7636a-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 5abdd29fb9f3..76510a0db4f9 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1097,6 +1097,12 @@ config REGULATOR_STW481X_VMMC
This driver supports the internal VMMC regulator in the STw481x
PMIC chips.
+config REGULATOR_SY7636A
+ tristate "Silergy SY7636A voltage regulator"
+ depends on MFD_SY7636A
+ help
+ This driver supports Silergy SY3686A voltage regulator.
+
config REGULATOR_SY8106A
tristate "Silergy SY8106A regulator"
depends on I2C && (OF || COMPILE_TEST)
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 680e539f6579..fe05347cbf84 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
+obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
obj-$(CONFIG_REGULATOR_SY8827N) += sy8827n.o
diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
new file mode 100644
index 000000000000..a639830298d6
--- /dev/null
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Functions to access SY3686A power management chip voltages
+ *
+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * Author: Lars Ivar Miljeteig <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+
+#include <linux/mfd/sy7636a.h>
+
+static int get_vcom_voltage_op(struct regulator_dev *rdev)
+{
+ int ret = get_vcom_voltage_mv(rdev->regmap);
+
+ if (ret < 0)
+ return ret;
+
+ return ret * 1000;
+}
+
+static int disable_regulator(struct regulator_dev *rdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+ int ret = 0;
+
+ mutex_lock(&sy7636a->reglock);
+ ret = regulator_disable_regmap(rdev);
+ usleep_range(30000, 35000);
+ mutex_unlock(&sy7636a->reglock);
+
+ return ret;
+}
+
+static int sy7636a_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+ int ret;
+
+ mutex_lock(&sy7636a->reglock);
+ ret = regulator_is_enabled_regmap(rdev);
+ mutex_unlock(&sy7636a->reglock);
+
+ return ret;
+}
+
+static int enable_regulator_pgood(struct regulator_dev *rdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+ int pwr_good = 0;
+ int ret = 0;
+ unsigned long t0, t1;
+ const unsigned int wait_time = 500;
+ unsigned int wait_cnt;
+
+ t0 = jiffies;
+
+ mutex_lock(&sy7636a->reglock);
+
+ ret = regulator_enable_regmap(rdev);
+ if (ret)
+ goto finish;
+
+ for (wait_cnt = 0; wait_cnt < wait_time; wait_cnt++) {
+ pwr_good = gpiod_get_value_cansleep(sy7636a->pgood_gpio);
+ if (pwr_good < 0) {
+ dev_err(&rdev->dev, "Failed to read pgood gpio: %d\n", pwr_good);
+ ret = pwr_good;
+ goto finish;
+ } else if (pwr_good)
+ break;
+
+ usleep_range(1000, 1500);
+ }
+
+ t1 = jiffies;
+
+ if (!pwr_good) {
+ dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
+ jiffies_to_msecs(t1 - t0));
+ ret = -ETIME;
+ goto finish;
+ }
+
+ dev_dbg(&rdev->dev, "Power good OK (took %u ms, %u waits)\n",
+ jiffies_to_msecs(t1 - t0),
+ wait_cnt);
+
+finish:
+ mutex_unlock(&sy7636a->reglock);
+ return ret;
+}
+
+static const struct regulator_ops sy7636a_vcom_volt_ops = {
+ .get_voltage = get_vcom_voltage_op,
+ .enable = enable_regulator_pgood,
+ .disable = disable_regulator,
+ .is_enabled = sy7636a_regulator_is_enabled,
+};
+
+struct regulator_desc desc = {
+ .name = "vcom",
+ .id = 0,
+ .ops = &sy7636a_vcom_volt_ops,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = SY7636A_REG_OPERATION_MODE_CRL,
+ .enable_mask = SY7636A_OPERATION_MODE_CRL_ONOFF,
+ .regulators_node = of_match_ptr("regulators"),
+ .of_match = of_match_ptr("vcom"),
+};
+
+static int sy7636a_regulator_init(struct sy7636a *sy7636a)
+{
+ return regmap_write(sy7636a->regmap,
+ SY7636A_REG_POWER_ON_DELAY_TIME,
+ 0x0);
+}
+
+static int sy7636a_regulator_suspend(struct device *dev)
+{
+ int ret;
+ struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
+
+ ret = get_vcom_voltage_mv(sy7636a->regmap);
+
+ if (ret > 0)
+ sy7636a->vcom = (unsigned int)ret;
+
+ return 0;
+}
+
+static int sy7636a_regulator_resume(struct device *dev)
+{
+ int ret;
+
+ struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
+
+ if (!sy7636a->vcom || sy7636a->vcom > 5000) {
+ dev_warn(dev, "Vcom value invalid, and thus not restored\n");
+ } else {
+ ret = set_vcom_voltage_mv(sy7636a->regmap, sy7636a->vcom);
+ if (ret)
+ return ret;
+ }
+
+ return sy7636a_regulator_init(sy7636a);
+}
+
+static int sy7636a_regulator_probe(struct platform_device *pdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
+ struct regulator_config config = { };
+ struct regulator_dev *rdev;
+ struct gpio_desc *gdp;
+ int ret;
+
+ if (!sy7636a)
+ return -EPROBE_DEFER;
+
+ platform_set_drvdata(pdev, sy7636a);
+
+ mutex_init(&sy7636a->reglock);
+
+ gdp = devm_gpiod_get(sy7636a->dev, "epd-pwr-good", GPIOD_IN);
+ if (IS_ERR(gdp)) {
+ dev_err(sy7636a->dev, "Power good GPIO fault %ld\n", PTR_ERR(gdp));
+ return PTR_ERR(gdp);
+ }
+
+ sy7636a->pgood_gpio = gdp;
+ dev_info(sy7636a->dev,
+ "Power good GPIO registered (gpio# %d)\n",
+ desc_to_gpio(sy7636a->pgood_gpio));
+
+ ret = sy7636a_regulator_init(sy7636a);
+ if (ret) {
+ dev_err(sy7636a->dev, "Failed to initialize regulator: %d\n", ret);
+ return ret;
+ }
+
+ config.dev = &pdev->dev;
+ config.dev->of_node = sy7636a->dev->of_node;
+ config.driver_data = sy7636a;
+ config.regmap = sy7636a->regmap;
+
+ rdev = devm_regulator_register(&pdev->dev, &desc, &config);
+ if (IS_ERR(rdev)) {
+ dev_err(sy7636a->dev, "Failed to register %s regulator\n",
+ pdev->name);
+ return PTR_ERR(rdev);
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id sy7636a_regulator_id_table[] = {
+ { "sy7636a-regulator", },
+};
+MODULE_DEVICE_TABLE(platform, sy7636a_regulator_id_table);
+
+static const struct dev_pm_ops sy7636a_pm_ops = {
+ .suspend = sy7636a_regulator_suspend,
+ .resume = sy7636a_regulator_resume,
+};
+
+static struct platform_driver sy7636a_regulator_driver = {
+ .driver = {
+ .name = "sy7636a-regulator",
+ .pm = &sy7636a_pm_ops,
+ },
+ .probe = sy7636a_regulator_probe,
+ .id_table = sy7636a_regulator_id_table,
+};
+module_platform_driver(sy7636a_regulator_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig <[email protected]>");
+MODULE_DESCRIPTION("SY7636A voltage regulator driver");
+MODULE_LICENSE("GPL v2");
--
2.29.2
On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
> --- /dev/null
> +++ b/drivers/regulator/sy7636a-regulator.c
> @@ -0,0 +1,233 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Functions to access SY3686A power management chip voltages
> + *
Please make the entire comment a C++ one so things look more
intentional.
> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> + *
> + * Author: Lars Ivar Miljeteig <[email protected]>
This probably needs an update.
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
This boilerplate is redundant and should be removed.
> +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> +{
> + int ret = get_vcom_voltage_mv(rdev->regmap);
> +
Why is this get_vcom_voltage_mv() function not in the regulator driver,
and why is it not just inline here? It also needs namespacing.
> +static int disable_regulator(struct regulator_dev *rdev)
> +{
> + struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> + int ret = 0;
> +
> + mutex_lock(&sy7636a->reglock);
> + ret = regulator_disable_regmap(rdev);
> + usleep_range(30000, 35000);
> + mutex_unlock(&sy7636a->reglock);
Why do you need this delay here, and what purpose is this lock intended
to serve? I can't understand what it's intended to protect.
> + mutex_lock(&sy7636a->reglock);
> + ret = regulator_is_enabled_regmap(rdev);
> + mutex_unlock(&sy7636a->reglock);
This lock usage in particular looks confused.
> + ret = regulator_enable_regmap(rdev);
> + if (ret)
> + goto finish;
> + if (!pwr_good) {
> + dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
> + jiffies_to_msecs(t1 - t0));
> + ret = -ETIME;
> + goto finish;
> + }
This doesn't undo the underlying enable, leaving the regulator in a
partially enabled state.
> +static const struct regulator_ops sy7636a_vcom_volt_ops = {
> + .get_voltage = get_vcom_voltage_op,
> + .enable = enable_regulator_pgood,
> + .disable = disable_regulator,
> + .is_enabled = sy7636a_regulator_is_enabled,
> +};
The namespacing for functions is very random and prone to clashes.
Given the power good signal I'd also expect a get_status() operation.
> +static int sy7636a_regulator_suspend(struct device *dev)
> +{
> + int ret;
> + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> +
> + ret = get_vcom_voltage_mv(sy7636a->regmap);
> +
> + if (ret > 0)
> + sy7636a->vcom = (unsigned int)ret;
> +
> + return 0;
> +}
What's going on here, and if you are going to store this value over
suspend why not store it in a variable of the correct type? In general
it's surprising to need a suspend operation for a regulator.
> + sy7636a->pgood_gpio = gdp;
> + dev_info(sy7636a->dev,
> + "Power good GPIO registered (gpio# %d)\n",
> + desc_to_gpio(sy7636a->pgood_gpio));
This print is just adding noise to the boot process.
On Sat, 16 Jan 2021 20:25:34 -0800, Alistair Francis wrote:
> Initial support for the Silergy SY7636A Power Management chip
> driver.
>
> Signed-off-by: Alistair Francis <[email protected]>
> ---
> .../bindings/mfd/silergy,sy7636a.yaml | 37 +++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml: $id: relative path/filename doesn't match actual path or filename
expected: http://devicetree.org/schemas/mfd/silergy,sy7636a.yaml#
See https://patchwork.ozlabs.org/patch/1427906
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <[email protected]> wrote:
>
> On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
>
> > --- /dev/null
> > +++ b/drivers/regulator/sy7636a-regulator.c
> > @@ -0,0 +1,233 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Functions to access SY3686A power management chip voltages
> > + *
>
> Please make the entire comment a C++ one so things look more
> intentional.
Fixed.
>
> > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > + *
> > + * Author: Lars Ivar Miljeteig <[email protected]>
>
> This probably needs an update.
>
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
>
> This boilerplate is redundant and should be removed.
Fixed.
>
> > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > +{
> > + int ret = get_vcom_voltage_mv(rdev->regmap);
> > +
>
> Why is this get_vcom_voltage_mv() function not in the regulator driver,
> and why is it not just inline here? It also needs namespacing.
I'm not sure what you mean, can you please explain?
>
> > +static int disable_regulator(struct regulator_dev *rdev)
> > +{
> > + struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> > + int ret = 0;
> > +
> > + mutex_lock(&sy7636a->reglock);
> > + ret = regulator_disable_regmap(rdev);
> > + usleep_range(30000, 35000);
> > + mutex_unlock(&sy7636a->reglock);
>
> Why do you need this delay here, and what purpose is this lock intended
The delay is to allow a power ramp up, I have added a comment.
> to serve? I can't understand what it's intended to protect.
Apparently the mutex is to protect enable/disable, I don't think it's
required and I will remove it.
>
> > + mutex_lock(&sy7636a->reglock);
> > + ret = regulator_is_enabled_regmap(rdev);
> > + mutex_unlock(&sy7636a->reglock);
>
> This lock usage in particular looks confused.
>
> > + ret = regulator_enable_regmap(rdev);
> > + if (ret)
> > + goto finish;
>
> > + if (!pwr_good) {
> > + dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
> > + jiffies_to_msecs(t1 - t0));
> > + ret = -ETIME;
> > + goto finish;
> > + }
>
> This doesn't undo the underlying enable, leaving the regulator in a
> partially enabled state.
Thanks, fixed.
>
> > +static const struct regulator_ops sy7636a_vcom_volt_ops = {
> > + .get_voltage = get_vcom_voltage_op,
> > + .enable = enable_regulator_pgood,
> > + .disable = disable_regulator,
> > + .is_enabled = sy7636a_regulator_is_enabled,
> > +};
>
> The namespacing for functions is very random and prone to clashes.
Fixed.
> Given the power good signal I'd also expect a get_status() operation.
Added.
>
> > +static int sy7636a_regulator_suspend(struct device *dev)
> > +{
> > + int ret;
> > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > +
> > + ret = get_vcom_voltage_mv(sy7636a->regmap);
> > +
> > + if (ret > 0)
> > + sy7636a->vcom = (unsigned int)ret;
> > +
> > + return 0;
> > +}
>
> What's going on here, and if you are going to store this value over
> suspend why not store it in a variable of the correct type? In general
It is part of the vendor's kernel, they specifically added it to
ensure vcom is set on resume.
I have fixed the variable type.
> it's surprising to need a suspend operation for a regulator.
>
> > + sy7636a->pgood_gpio = gdp;
> > + dev_info(sy7636a->dev,
> > + "Power good GPIO registered (gpio# %d)\n",
> > + desc_to_gpio(sy7636a->pgood_gpio));
>
> This print is just adding noise to the boot process.
Removed.
Alistair
On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote:
> On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <[email protected]> wrote:
> > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
> > > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > > +{
> > > + int ret = get_vcom_voltage_mv(rdev->regmap);
> > > +
> > Why is this get_vcom_voltage_mv() function not in the regulator driver,
> > and why is it not just inline here? It also needs namespacing.
> I'm not sure what you mean, can you please explain?
This is a wrapper for a function that has exactly one caller but is not
only a separate function but also in the MFD header, part of a separate
driver. This seems at best pointless.
> > Why do you need this delay here, and what purpose is this lock intended
> The delay is to allow a power ramp up, I have added a comment.
Use the standard ramp_delay, don't open code it.
> > > +static int sy7636a_regulator_suspend(struct device *dev)
> > > +{
> > > + int ret;
> > > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > > +
> > > + ret = get_vcom_voltage_mv(sy7636a->regmap);
> > > +
> > > + if (ret > 0)
> > > + sy7636a->vcom = (unsigned int)ret;
> > > +
> > > + return 0;
> > > +}
> > What's going on here, and if you are going to store this value over
> > suspend why not store it in a variable of the correct type? In general
> It is part of the vendor's kernel, they specifically added it to
> ensure vcom is set on resume.
"I copied this from the vendor" isn't really a great explanation... If
the device is likely to get completely powered off and loosing settings
then presumably the entire register map, not just this one value, needs
to be saved and restored instead of just this one value. If that is the
case it's probably best to use a register cache and just resync it on
resume.
On Fri, Jan 22, 2021 at 5:37 AM Mark Brown <[email protected]> wrote:
>
> On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote:
> > On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <[email protected]> wrote:
> > > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
>
> > > > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > > > +{
> > > > + int ret = get_vcom_voltage_mv(rdev->regmap);
> > > > +
>
> > > Why is this get_vcom_voltage_mv() function not in the regulator driver,
> > > and why is it not just inline here? It also needs namespacing.
>
> > I'm not sure what you mean, can you please explain?
>
> This is a wrapper for a function that has exactly one caller but is not
> only a separate function but also in the MFD header, part of a separate
> driver. This seems at best pointless.
Ah I see. I think I have fixed this.
>
> > > Why do you need this delay here, and what purpose is this lock intended
>
> > The delay is to allow a power ramp up, I have added a comment.
>
> Use the standard ramp_delay, don't open code it.
>
> > > > +static int sy7636a_regulator_suspend(struct device *dev)
> > > > +{
> > > > + int ret;
> > > > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > > > +
> > > > + ret = get_vcom_voltage_mv(sy7636a->regmap);
> > > > +
> > > > + if (ret > 0)
> > > > + sy7636a->vcom = (unsigned int)ret;
> > > > +
> > > > + return 0;
> > > > +}
>
> > > What's going on here, and if you are going to store this value over
> > > suspend why not store it in a variable of the correct type? In general
>
> > It is part of the vendor's kernel, they specifically added it to
> > ensure vcom is set on resume.
>
> "I copied this from the vendor" isn't really a great explanation... If
> the device is likely to get completely powered off and loosing settings
> then presumably the entire register map, not just this one value, needs
> to be saved and restored instead of just this one value. If that is the
> case it's probably best to use a register cache and just resync it on
> resume.
Good point.
I don't have a good answer so I have removed the suspend/resume part.
I'll have to investigate in the future if/why this is required.
Alistair