Messages in this thread | | | Date | Tue, 18 Feb 2014 16:45:32 +0900 | From | Milo Kim <> | Subject | Re: [PATCH 01/10] mfd: Add TI LMU driver |
| |
Hi Lee,
On 02/17/2014 06:57 PM, Lee Jones wrote: >> +static const struct resource lm3633_effect_resources[] = { >> + { >> + .name = LM3633_EFFECT_BL0_RAMPUP, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(BL0_RAMPUP), >> + }, >> + { >> + .name = LM3633_EFFECT_BL0_RAMPDOWN, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(BL0_RAMPDN), >> + }, >> + { >> + .name = LM3633_EFFECT_BL1_RAMPUP, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(BL1_RAMPUP), >> + }, >> + { >> + .name = LM3633_EFFECT_BL1_RAMPDOWN, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(BL1_RAMPDN), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN_DELAY, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(DELAY), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN_HIGHTIME, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(HIGHTIME), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN_LOWTIME, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(LOWTIME), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN0_RAMPUP, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(PTN0_RAMPUP), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN0_RAMPDOWN, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(PTN0_RAMPDN), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN1_RAMPUP, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(PTN1_RAMPUP), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN1_RAMPDOWN, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(PTN1_RAMPDN), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN_LOWBRT, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(LOWBRT), >> + }, >> + { >> + .name = LM3633_EFFECT_PTN_HIGHBRT, >> + .flags = IORESOURCE_REG, >> + .start = LM3633_EFFECT_REGISTER(HIGHBRT), >> + }, >> +}; > > Can you define a MACRO to do all of these as one liners?
Yes, resource definitions will be replaced by simple macro, LMU_EFFECT_RESOURCE().
For example,
#define LMU_EFFECT_RESOURCE(chip, effect) \ { \ .name = chip##_EFFECT_##effect, \ .flags = IORESOURCE_REG, \ .start = LMU_EFFECT_REGISTER(chip, effect), \ }
static const struct resource lm3633_effect_resources[] = { LMU_EFFECT_RESOURCE(LM3633, BL0_RAMPUP), LMU_EFFECT_RESOURCE(LM3633, BL0_RAMPDN), LMU_EFFECT_RESOURCE(LM3633, BL1_RAMPUP), LMU_EFFECT_RESOURCE(LM3633, BL1_RAMPDN), LMU_EFFECT_RESOURCE(LM3633, PTN_DELAY), LMU_EFFECT_RESOURCE(LM3633, PTN_HIGHTIME), LMU_EFFECT_RESOURCE(LM3633, PTN_LOWTIME), LMU_EFFECT_RESOURCE(LM3633, PTN0_RAMPUP), LMU_EFFECT_RESOURCE(LM3633, PTN0_RAMPDN), LMU_EFFECT_RESOURCE(LM3633, PTN1_RAMPUP), LMU_EFFECT_RESOURCE(LM3633, PTN1_RAMPDN), LMU_EFFECT_RESOURCE(LM3633, PTN_LOWBRT), LMU_EFFECT_RESOURCE(LM3633, PTN_HIGHBRT), };
static const struct resource lm3697_effect_resources[] = { LMU_EFFECT_RESOURCE(LM3697, BL0_RAMPUP), LMU_EFFECT_RESOURCE(LM3697, BL0_RAMPDN), LMU_EFFECT_RESOURCE(LM3697, BL1_RAMPUP), LMU_EFFECT_RESOURCE(LM3697, BL1_RAMPDN), };
and so on.
> <snip> > >> +static int ti_lmu_parse_dt(struct device *dev, struct ti_lmu *lmu) >> +{ > > <snip> > >> + pdata->en_gpio = of_get_named_gpio(node, "ti,enable-gpio", 0); > > There is a global DT property for this already.
I've not found it yet, but I agree it looks like general property. So I'll replace "ti,enable-gpio" with "ti,lmu-en-gpio".
> >> +static int ti_lmu_probe(struct i2c_client *cl, const struct i2c_device_id *id) >> +{ > > <snip> > >> + lmu->id = id->driver_data; >> + switch (lmu->id) { >> + case LM3532: >> + lmu_regmap_config.max_register = LM3532_MAX_REGISTERS; >> + break; >> + case LM3631: >> + lmu_regmap_config.max_register = LM3631_MAX_REGISTERS; >> + break; >> + case LM3633: >> + lmu_regmap_config.max_register = LM3633_MAX_REGISTERS; >> + break; >> + case LM3695: >> + lmu_regmap_config.max_register = LM3695_MAX_REGISTERS; >> + break; >> + case LM3697: >> + lmu_regmap_config.max_register = LM3697_MAX_REGISTERS; >> + break; >> + default: >> + break; >> + } > > As there are so many of these, it might be nicer to pull these out > into a seperate function.
Lines of code will be moved to new function, ti_lmu_regmap_init().
Thanks for your review.
Best regards, Milo
| |