Re: [PATCH v3 3/5] mfd: tps65090: Stop caching most registers

[Doug Anderson]

Lee,

On Thu, Apr 17, 2014 at 4:00 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>> Nearly all of the registers in tps65090 combine control bits and
>> status bits.  Turn off caching of all registers except the select few
>> that can be cached.
>>
>> In order to avoid adding more duplicate #defines, we also move some
>> register offset definitions to the mfd driver (and resolve
>> inconsistent names).
>>
>> Signed-off-by: Doug Anderson <dianders@xxxxxxxxxxxx>
>> ---
>> Changes in v3: None
>> Changes in v2:
>> - Leave cache on for the registers that can be cached.
>> - Move register offsets to mfd header file.
>>
>>  drivers/mfd/tps65090.c           | 27 ++++++++++++++-------------
>>  drivers/power/tps65090-charger.c | 11 -----------
>>  include/linux/mfd/tps65090.h     | 14 ++++++++++++++
>>  3 files changed, 28 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
>> index c3cddb4..1c3e6e2 100644
>> --- a/drivers/mfd/tps65090.c
>> +++ b/drivers/mfd/tps65090.c
>> @@ -32,14 +32,6 @@
>>  #define NUM_INT_REG 2
>>  #define TOTAL_NUM_REG 0x18
>>
>> -/* interrupt status registers */
>> -#define TPS65090_INT_STS     0x0
>> -#define TPS65090_INT_STS2    0x1
>> -
>> -/* interrupt mask registers */
>> -#define TPS65090_INT_MSK     0x2
>> -#define TPS65090_INT_MSK2    0x3
>> -
>>  #define TPS65090_INT1_MASK_VAC_STATUS_CHANGE         1
>>  #define TPS65090_INT1_MASK_VSYS_STATUS_CHANGE                2
>>  #define TPS65090_INT1_MASK_BAT_STATUS_CHANGE         3
>> @@ -144,17 +136,26 @@ static struct regmap_irq_chip tps65090_irq_chip = {
>>       .irqs = tps65090_irqs,
>>       .num_irqs = ARRAY_SIZE(tps65090_irqs),
>>       .num_regs = NUM_INT_REG,
>> -     .status_base = TPS65090_INT_STS,
>> -     .mask_base = TPS65090_INT_MSK,
>> +     .status_base = TPS65090_REG_INTR_STS,
>> +     .mask_base = TPS65090_REG_INTR_MASK,
>>       .mask_invert = true,
>>  };
>>
>>  static bool is_volatile_reg(struct device *dev, unsigned int reg)
>>  {
>> -     if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
>> -             return true;
>> -     else
>> +     /* Nearly all registers have status bits mixed in, except a few */
>> +     switch (reg) {
>> +     case TPS65090_REG_INTR_MASK:
>> +     case TPS65090_REG_INTR_MASK2:
>> +     case TPS65090_REG_CG_CTRL0:
>> +     case TPS65090_REG_CG_CTRL1:
>> +     case TPS65090_REG_CG_CTRL2:
>> +     case TPS65090_REG_CG_CTRL3:
>> +     case TPS65090_REG_CG_CTRL4:
>> +     case TPS65090_REG_CG_CTRL5:
>>               return false;
>> +     }
>> +     return true;
>>  }
>
> I'll not force the issue, but if you wanted to do this more succinctly
> you could also do:
>
>   case TPS65090_REG_INTR_MASK ... TPS65090_REG_INTR_MASK:
>   case TPS65090_REG_CG_CTRL0  ... TPS65090_REG_CG_CTRL5:
>
> or
>
>   if (reg >= TPS65090_REG_INTR_MASK && reg <= TPS65090_REG_CG_CTRL5)
>
> Ect.
>
> Otherwise patch looks fine:
>   Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>

If I need to spin the series for another reason I'll make that change.
 Otherwise I won't plan to spin.  Thanks for the review!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/