lkml.org 
[lkml]   [2012]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-04-20 11:25    [W:0.269 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site