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",®ulator_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.