linux-kernel - Re: [PATCH 1/2] mfd: Add Freescale's PMIC MC34708 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20120420092007.GA3892@opensource.wolfsonmicro.com>
Date:	Fri, 20 Apr 2012 10:20:08 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	"Ying-Chun Liu (PaulLiu)" <paul.liu@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linaro-dev@...ts.linaro.org, patches@...aro.org,
	Robin Gong <B38343@...escale.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Shawn Guo <shawn.guo@...aro.org>
Subject: Re: [PATCH 1/2] mfd: Add Freescale's PMIC MC34708 support

On Fri, Apr 20, 2012 at 12:38:40AM +0800, Ying-Chun Liu (PaulLiu) wrote:

> +Sub-nodes:
> +- regulators : Contain the regulator nodes.  The MC34708 regulators are
> +  bound using their names as listed below for enabling.
> +
> +    mc34708__sw1a    : regulator SW1A
> +    mc34708__sw1b    : regulator SW1B

There's no point in including the chip name in the properties - the
device has already been bound at the device level, this is just noise
at this level.

> +	int ret;
> +	int i;
> +
> +	memset(buf, 0, 3);
> +	for (i = 0; i < PMIC_I2C_RETRY_TIMES; i++) {
> +		ret = i2c_smbus_read_i2c_block_data(client, offset, 3, buf);

The I2C layer already has a retry mechanism, and obviously if I2C is
failing at all the board generally has serious problems.

In general I'm not 100% sure why you're not using the regmap API here -
it looks like the 24 bit I/O is just a block I/O.  Alternatively you
could use regmap for the register I/O and then open code the 24 bit
access if they really are different.  This would let you
make much more use of framework support.

> +	return mc34708_reg_write(mc_pmic, offmask, mask | irqbit);
> +}
> +EXPORT_SYMBOL(mc34708_irq_mask);

You shouldn't be open coding stuff like this, you should be implementing
it using genirq.  This again gives you better framework support.

> +static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
> +						const struct i2c_client *client)
> +{
> +	while (id->name[0]) {
> +		if (strcmp(client->name, id->name) == 0)
> +			return id;
> +		id++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static const struct i2c_device_id *i2c_get_device_id(const struct i2c_client
> +						     *idev)
> +{
> +	const struct i2c_driver *idrv = to_i2c_driver(idev->dev.driver);
> +
> +	return i2c_match_id(idrv->id_table, idev);
> +}

This stuff should be added as generic I2C helpers if it's useful.

> +	if (pdata && pdata->flags & MC34708_USE_REGULATOR) {
> +		struct mc34708_regulator_platform_data regulator_pdata = {
> +			.num_regulators = pdata->num_regulators,
> +			.regulators = pdata->regulators,
> +		};
> +
> +		mc34708_add_subdevice_pdata(mc_pmic, "%s-regulator",
> +					    &regulator_pdata,
> +					    sizeof(regulator_pdata));
> +	} else if (of_find_node_by_name(np, "regulators")) {
> +		mc34708_add_subdevice(mc_pmic, "%s-regulator");
> +	}

This shouldn't be conditional, the regulators are always physically
present and even if they're not actively managed we can look at their
setup.

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