linux-kernel - Re: [NEW DRIVER V1 7/7] DA9058 REGULATOR driver

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] [day] [month] [year] [list]

Message-ID: <20120802103005.GH29157@opensource.wolfsonmicro.com>
Date:	Thu, 2 Aug 2012 11:30:05 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Anthony Olech <anthony.olech.opensource@...semi.com>
Cc:	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [NEW DRIVER V1 7/7] DA9058 REGULATOR driver

On Thu, Aug 02, 2012 at 09:48:58AM +0100, Anthony Olech wrote:

Overall the big issue here is that the driver isn't making much use of
framework features, there should be a *lot* less code here.

> +static unsigned int da9058_regulator_val_to_mvolts(unsigned int val,
> +						struct regulator_dev *rdev)

regualtor_map_voltage_linear()

> +	ret = da9058_reg_read(da9058, regulator->control_register, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (regulator->control_enable_mask&val)
> +		return true;
> +	else
> +		return false;

regulator_is_enabled_regmap().

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

Should be set_voltage_sel().

> +	if (regulator->ramp_register && regulator->ramp_enable_mask)
> +		ret =
> +			da9058_set_bits(da9058, regulator->ramp_register,
> +					regulator->ramp_enable_mask);

What is this doing?

> +	if (regulator->control_voltage_step == 0) {
> +		if (da9058_regulator_is_enabled(rdev))
> +			return regulator->fixed_voltage;
> +		else
> +			return 0;
> +	}

No, don't do stuff like this - implement a separate op (or just use the
standard ops in this case).

> +
> +	ret = da9058_reg_read(da9058, regulator->control_register, &val);
> +	if (ret)
> +		return ret;

This should just be regulator_get_voltage_sel_regmap().

> +static int da9058_regulator_enable(struct regulator_dev *rdev)
> +{

regulator_enable_regmap() and similarly for all the others.

> +static unsigned int da9058_regulator_get_mode(struct regulator_dev *rdev)
> +{
> +	return REGULATOR_MODE_NORMAL;
> +}

If you don't support modes don't implement the op.

> +	regulator_pdata = cell->platform_data;

> +	if (regulator_pdata == NULL) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}

The driver should be able to start without any platform data.

> +	dev_info(&pdev->dev, "Starting REGULATOR %d = %s\n",
> +					regulator_pdata->regulator_id,
> +					regulator_pdata->regulator_name);

There's a couple of issues here.  One is that you shouldn't be logging
anything here, log messages on driver load are only useful if you're
saying something you've identified from the hardware - for example,
logging the device revision on startup is very helpful.  The other is
that you shouldn't need to supply a name and ID as platform data since
that's exactly how the platform bus binds drivers.

> +	regulator->control_voltage_step = regulator_pdata->control_voltage_step;
> +	regulator->control_register = regulator_pdata->control_register;
> +	regulator->control_enable_mask = regulator_pdata->control_enable_mask;
> +	regulator->ramp_register = regulator_pdata->ramp_register;
> +	regulator->ramp_enable_mask = regulator_pdata->ramp_enable_mask;
> +	regulator->fixed_voltage = regulator_pdata->fixed_voltage;

Your MFD didn't visibly provide any of this stuff, though I may have
missed it, and honestly I'd expect most of this to be in the driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/