Re: [PATCH v4 3/5] regulator: sy7636a: Initial commit

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

On Thu, Mar 25, 2021 at 09:55:09PM -0400, Alistair Francis wrote:

> Initial support for the Silergy SY7636A-regulator Power Management chip.

In general this driver seems like it's trying to implement a bunch of
policy and extra features beyond standard regulator support - please
strip it back more, if you feel that there's things that really do need
to be added in the driver itself post them as separate patches.  It's
also open coding some features the core provides.  This should all make
the driver much smaller and simpler.

> +++ b/Documentation/ABI/testing/sysfs-driver-sy7636a-regulator
> @@ -0,0 +1,21 @@
> +What:		/sys/bus/regulator/drivers/sy7636a-regulator/state
> +Date:		April 2021
> +KernelVersion:	5.12

None of these sysfs files are appropriate for a driver, if they are
useful they should be added to the core (but some of them seem like
they duplicate files that already exist, this one being an example).
There's absolutely nothing device specific about any of them.

> +static int sy7636a_disable_regulator(struct regulator_dev *rdev)
> +{
> +	int ret = 0;
> +
> +	ret = regulator_disable_regmap(rdev);
> +	// Delay for ~35ms after disabling the regulator, to allow power ramp
> +	// down to go undisturbed
> +	usleep_range(30000, 35000);

If this is needed add it to the core, but really this sort of stuff is
going to be very board specific - it'll depend on what the load on the
regulator is - and it's pretty rare for anything to care, you don't have
the same issues you have on enable.

> +static int sy7636a_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	return regulator_is_enabled_regmap(rdev);
> +}

Just use the generic operation, this wrapper is not adding anything
except code size.

> +static int sy7636a_get_status(struct regulator_dev *rdev)
> +{
> +	struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> +	int pwr_good = 0;
> +	const unsigned int wait_time = 500;
> +	unsigned int wait_cnt;
> +	int ret = 0;
> +
> +	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;
> +			return ret;
> +		} else if (pwr_good)
> +			break;

This should just read the status, if the caller wants to retry for a
while then the caller can make that decision.

> +	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);

Use poll_enabled_time to check the status, no need for a custom enable
operation.

> +			ret = pwr_good;
> +			goto finish;
> +		} else if (pwr_good)
> +			break;

As per coding-style.rst both sides of the if statement should use braces
if one does.

> +	ret = sysfs_create_group(&rdev->dev.kobj, &sy7636a_sysfs_attr_group);
> +	if (ret) {
> +		dev_err(sy7636a->dev, "Failed to create sysfs attributes\n");
> +		return ret;
> +	}

*If* the driver is creating sysfs devices it *definitely* shouldn't be
creating and destroying them dynamically at runtime.

> +	ret = sy7636a_regulator_init(sy7636a);
> +	if (ret) {
> +		dev_err(sy7636a->dev, "Failed to initialize regulator: %d\n", ret);
> +		return ret;
> +	}

This function has one caller and one statement in it, just inline it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]