linux-kernel - Re: RFC 2: bq2415x

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 27 Jan 2012 16:24:55 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Pali Roh?r <pali.rohar@...il.com>
Cc:	linux-main <linux-kernel@...r.kernel.org>,
	linux-omap <linux-omap@...r.kernel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Aliaksei Katovich <aliaksei.katovich@...ia.com>,
	Vladimir Zapolskiy <vz@...ia.com>,
	Felipe Contreras <felipe.contreras@...il.com>,
	Anton Vorontsov <cbouatmailru@...il.com>,
	Joerg Reisenweber <joerg@...nmoko.org>,
	Sebastian Reichel <sre@...ian.org>,
	???????????? ???????????????? <freemangordon@....bg>
Subject: Re: RFC 2: bq2415x_charger driver

On Fri, Jan 27, 2012 at 03:40:43AM +0100, Pali Roh?r wrote:

> +static char *bq2415x_rev_name[] = {
> +	"1.0",
> +	"1.1",
> +	"1.2",
> +	"1.3",
> +	"1.4",
> +	"1.5",
> +	"1.6",
> +	"1.7",

Looks like you can just store this as a number?

> +struct bq2415x_device {
> +	struct device *dev;
> +	struct bq2415x_platform_data *platform_data;

You should take a copy of the platform data so it can be marked
initdata.

> +static DEFINE_MUTEX(bq2415x_id_mutex);
> +static DEFINE_MUTEX(bq2415x_timer_mutex);
> +static DEFINE_MUTEX(bq2415x_type_mutex);
> +static DEFINE_MUTEX(bq2415x_i2c_mutex);

Why are these global?

> +/* i2c read functions */
> +
> +static int bq2415x_i2c_read(struct bq2415x_device *bq, u8 reg)
> +{
> +	struct i2c_client *client = to_i2c_client(bq->dev);

You could save a bunch of code by moving all this I2C register I/O over
to regmap.

> +		case BQ2415X_BOOST_MODE_ENABLE:
> +			return bq2415x_i2c_write_bit(bq, BQ2415X_REG_CONTROL, 1, BQ2415X_BIT_OPA_MODE);

Keep things under 80 columns - see CodingStyle.

> +	if (client->addr == 0x6b) {

> +	} else if (client->addr == 0x6a) {

This looks like a switch statement.

> +	switch (chip) {
> +		case BQUNKNOWN:
> +			return bq2415x_rev_name[8];

Magic numbers ahoy...

> +static int bq2415x_vender_code(struct bq2415x_device *bq)
> +{
> +	int ret = bq2415x_exec_command(bq, BQ2415X_VENDER_CODE);
> +	if (ret < 0)
> +		return 0;
> +	else /* convert to binary */
> +		return (ret & 0x1) + ((ret >> 1) & 0x1) * 10 + ((ret >> 2) & 0x1) * 100;

Just print it as hex?

> +static int bq2415x_set_weak_battery_voltage(struct bq2415x_device *bq, int mV)
> +{
> +	int val = mV/100 + (mV%100 > 0 ? 1 : 0) - 34;

This could probably be written more clearly?

> +static int bq2415x_get_charge_current_sense_voltage(struct bq2415x_device *bq)
> +{
> +	/* TODO */
> +	return -ENOSYS;
> +}

Just don't provide things that aren't there, the frameworks should do
appropriate handling.

> +static enum power_supply_property bq2415x_power_supply_props[] = {
> +	/* TODO */
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_MODEL_NAME,

TODO?

> +static int bq2415x_power_supply_set_mode(struct bq2415x_device *bq, enum bq2415x_mode mode)
> +{
> +	int ret = 0;
> +
> +	switch (mode) {
> +
> +		case BQ2415X_MODE_NONE: /* N/A */

CodingStyle for the indentation.

> +			if (mode == BQ2415X_MODE_NONE) {
> +				dev_info(bq->dev, "mode: N/A\n");
> +				ret = bq2415x_set_current_limit(bq, 100);
> +			} else if (mode == BQ2415X_MODE_HOST_CHARGER) {
> +				dev_info(bq->dev, "mode: Host/HUB charger\n");
> +				ret = bq2415x_set_current_limit(bq, 500);
> +			} else {
> +				dev_info(bq->dev, "mode: Dedicated charger\n");
> +				ret = bq2415x_set_current_limit(bq, 1800);
> +			}

This should be a switch statement.

> +		case BQ2415X_MODE_BOOST: /* Boost mode */
> +			dev_info(bq->dev, "mode: Boost\n");

Is dev_info() really appropriate for this stuff?

> +			break;
> +
> +	}

No default case?

> +static void bq2415x_power_supply_set_charger_type(int type, void *data)

> +	if (type == 0)
> +		bq->charger_mode = BQ2415X_MODE_NONE;
> +	else if (type == 1)
> +		bq->charger_mode = BQ2415X_MODE_HOST_CHARGER;
> +	else if (type == 2)
> +		bq->charger_mode = BQ2415X_MODE_DEDICATED_CHARGER;
> +	else
> +		return;

Switch statement.  I never understood why this is such a common idiom...

> +		switch (error) {
> +			case 0: /* No error */
> +				break;
> +			case 6: /* Timer expired */
> +				dev_info(bq->dev, "Timer expired\n");
> +				break;

Should we really be logging this at anything more than dev_dbg()?

> +			case 7: /* N/A */
> +				bq2415x_power_supply_error(bq, "Unknow error");
> +				return;

Typo: unknown.

> +			case 5: /* Termal shutdown (too hot) */
> +				bq2415x_power_supply_error(bq, "Termal shutdown (too hot)");

Typo: thermal.

> +static int bq2415x_power_supply_get_property(struct power_supply *psy, enum power_supply_property psp, union power_supply_propval *val)

CodingStyle - wrapping (lots of this 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/