Re: [PATCH 4/6] regulator: Initial commit of sy7636a

On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:

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 [off-list ref]

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.