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

On Mon, Jan 18, 2021 at 4:32 AM Mark Brown [off-list ref] wrote:

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

quoted

--- /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.

quoted

+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * Author: Lars Ivar Miljeteig [off-list ref]

This probably needs an update.

quoted

+ * 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.

quoted

+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?

quoted

+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.

quoted

+     mutex_lock(&sy7636a->reglock);
+     ret = regulator_is_enabled_regmap(rdev);
+     mutex_unlock(&sy7636a->reglock);

This lock usage in particular looks confused.

quoted

+     ret = regulator_enable_regmap(rdev);
+     if (ret)
+             goto finish;

quoted

+     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.

quoted

+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.

quoted

+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.

quoted

+     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