Re: [PATCH 3/4] regulator: Add labibb driver

[Mark Brown <broonie@kernel.org>]

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

On Wed, Jun 12, 2019 at 04:30:51PM +0530, Nisha Kumari wrote:

> +static int qcom_labibb_read(struct qcom_labibb *labibb, u16 address,
> +			    u8 *val, int count)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_read(labibb->regmap, address, val, count);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi read failed ret=%d\n", ret);
> +
> +	return ret;
> +}

This (and the write function) are utterly trivial wrappers around the
corresponding regmap functions...

> +static int qcom_labibb_masked_write(struct qcom_labibb *labibb, u16 address,
> +				    u8 mask, u8 val)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(labibb->regmap, address, mask, val);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
> +
> +	return ret;
> +}

...as is this but it changes the name for some reason.

> +static int qcom_enable_ibb(struct qcom_labibb *labibb, bool enable)
> +{
> +	int ret;
> +	u8 val = enable ? IBB_CONTROL_ENABLE : 0;

Please write normal conditional statements, it makes things easier to
read.  Though this function is so trivial it seems better to just inline
it into the callers.

> +static int qcom_lab_regulator_enable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	val = LAB_ENABLE_CTL_EN;
> +	ret = qcom_labibb_write(labibb,
> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
> +				&val, 1);

Why not just use regmap_write()?  It'd be clearer.

> +	labibb->lab_vreg.vreg_enabled = 1;

What function does this serve?  It never seems to be read.

> +	ret = qcom_labibb_write(labibb,
> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
> +				&val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +	/* after this delay, lab should get disabled */
> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}

I'm not clear that these status checks are actually a good idea, and if
they are it feels like they should be factored out into the framework -
these are just regular enable or disable followed by the usual dead
reckoning delay for completion and then a get_status() call to confirm
if the operation worked.  That's not at all driver specific so if it's
useful the core should do it for all regulators with status readback and
if you didn't do it you could use the standard regmap helpers for these
operations.  

> +static int qcom_lab_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	return val & LAB_STATUS1_VREG_OK_BIT;
> +}

Please use the standard helper for this, and this is a get_status()
operation not an is_enabled() - it checks if the regulator is working,
not what status was requested.

> +	while (retries--) {
> +		/* Wait for a small period before reading IBB_STATUS1 */
> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +
> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
> +				       REG_IBB_STATUS1, &val, 1);
> +		if (ret < 0) {
> +			dev_err(labibb->dev,
> +				"Read register failed ret = %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (val & IBB_STATUS1_VREG_OK_BIT) {
> +			labibb->ibb_vreg.vreg_enabled = 1;
> +			return 0;
> +		}
> +	}

This is doing more than the other regulator was but it's not clear why -
is it just that the delays are different for the two regulators?

> +static int register_lab_regulator(struct qcom_labibb *labibb,
> +				  struct device_node *of_node)
> +{
> +	int ret = 0;
> +	struct regulator_init_data *init_data;
> +	struct regulator_config cfg = {};
> +
> +	cfg.dev = labibb->dev;
> +	cfg.driver_data = labibb;
> +	cfg.of_node = of_node;
> +	init_data =
> +		of_get_regulator_init_data(labibb->dev,
> +					   of_node, &lab_desc);
> +	if (!init_data) {
> +		dev_err(labibb->dev,
> +			"unable to get init data for LAB\n");
> +		return -ENOMEM;
> +	}

The core will parse the DT for you.

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