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 <lars.ivar.miljeteig@xxxxxxxxxxxxxx> 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.
Attachment:
signature.asc
Description: PGP signature
