linux-kernel - Re: [PATCH] regulator: cpcap: Add basic regulator support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170131211256.l3e53y2mkzylfjnw@sirena.org.uk>
Date:   Tue, 31 Jan 2017 21:12:56 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, devicetree@...r.kernel.org,
        Marcel Partap <mpartap@....net>,
        Michael Scott <michael.scott@...aro.org>
Subject: Re: [PATCH] regulator: cpcap: Add basic regulator support

On Mon, Jan 30, 2017 at 04:12:41PM -0800, Tony Lindgren wrote:

> +static int cpcap_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	unsigned int value = 0;
> +
> +	regmap_read(rdev->regmap, rdev->desc->enable_reg, &value);
> +
> +	return ((value & rdev->desc->enable_mask) == rdev->desc->enable_reg);
> +}

Use the core support for this, reulator_is_enabled_regmap().

> +	error = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> +				   rdev->desc->enable_mask,
> +				   rdev->desc->enable_val);
> +	if (error)
> +		return error;
> +
> +	if (rdev->desc->enable_val & CPCAP_REG_OFF_MODE_SEC) {
> +		error = regmap_update_bits(rdev->regmap, regulator->assign_reg,
> +					   regulator->assign_mask,
> +					   regulator->assign_mask);
> +		if (error)
> +			return error;
> +	}

What does this second register bit do?

> +static int cpcap_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct cpcap_regulator *regulator = rdev_get_drvdata(rdev);
> +	unsigned int value = 0;
> +	int voltage;

regulator_get_voltage_sel_regmap() (the driver should just use selectors
and let the mapping happen in the core).

> +	regmap_read(rdev->regmap, rdev->desc->enable_reg, &value);
> +
> +	if (!(value & rdev->desc->enable_mask))
> +		return 0;

This is buggy, the driver should report the voltage the regulator will
have when powered.

> +static int cpcap_regulator_set_voltage(struct regulator_dev *rdev,
> +				       int min_uV, int max_uV,
> +				       unsigned int *selector)

regulator_map_voltage_iterate() and regulator_set_voltage_regmap()

> +	switch (mode) {
> +	case REGULATOR_MODE_NORMAL:
> +		value = CPCAP_BIT_AUDIO_LOW_PWR;
> +		break;
> +	case REGULATOR_MODE_IDLE:
> +	case REGULATOR_MODE_STANDBY:
> +		value = 0;
> +		break;

There should be a 1:1 mapping between modes and register values, if the
device only supports two modes so should the driver.

> +	init_data.constraints.valid_modes_mask =
> +		REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
> +	init_data.constraints.valid_ops_mask =
> +		REGULATOR_CHANGE_MODE | REGULATOR_CHANGE_STATUS;
> +	config.init_data = &init_data;

Drivers should not configure constraints, they're part of the system
integration and come from the firmware or board file.

> +		dev_info(&pdev->dev, "registering regulator %s\n",
> +			 regulator->rdesc.name);

This is needless spam, please remove it.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)