RE: [PATCH 6/6] mfd: da9063: Add DA9063L support

From: Steve Twiss
Date: Thu May 24 2018 - 12:37:49 EST

Next message: Sebastian Reichel: "[PATCH 0/2] serial: imx: more shutdown cleanups"
Previous message: Christian Brauner: "Re: [REVIEW][PATCH 4/6] fs: Allow superblock owner to access do_remount_sb()"
In reply to: Marek Vasut: "Re: [PATCH 6/6] mfd: da9063: Add DA9063L support"
Next in thread: Marek Vasut: "Re: [PATCH 6/6] mfd: da9063: Add DA9063L support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 24 May 2018 15:51 Marek Vasut wrote:

Hi Marek,

> To: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>; Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx>; Lee Jones <lee.jones@xxxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>;
> linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 6/6] mfd: da9063: Add DA9063L support
>
> On 05/24/2018 02:32 PM, Steve Twiss wrote:
> > On 24 May 2018 @ 12:49 Steve Twiss wrote:
> >>> On 23 May 2018 12:43 Marek Vasut wrote,
> >>>
> >>> To: linux-kernel@xxxxxxxxxxxxxxx
> >>> Subject: [PATCH 6/6] mfd: da9063: Add DA9063L support
> >>>
> >>> Add support for DA9063L, which is a reduced variant of the DA9063 with less regulators and without RTC.
> >>>
> >>
> >> There's potentially more to this file. Without an RTC the regmap
> >> access tables would change and the usual DA9063 (BB silicon) tables would become invalid.
> >> The tables for da9063_bb_readable_ranges, da9063_bb_writeable_ranges,
> >> da9063_bb_volatile_ranges, would need to be updated for DA9063L, if a new chip model was needed.
> >>
> >> The new ranges would be this (see below), and would remove any RTC accesses in the new chip model.
> >>
> >> static const struct regmap_range da9063l_bb_readable_ranges[] = {
> >> {
> >> .range_min = DA9063_REG_PAGE_CON,
> >> .range_max = DA9063_REG_MON_A10_RES,
> >> }, {
> >> .range_min = DA9063_REG_SEQ,
> >> .range_max = DA9063_REG_ID_32_31,
> >> }, {
> >> .range_min = DA9063_REG_SEQ_A,
> >> .range_max = DA9063_REG_AUTO3_LOW,
> >> }, {
> >> .range_min = DA9063_REG_T_OFFSET,
> >> .range_max = DA9063_BB_REG_GP_ID_19,
> >> }, {
> >> .range_min = DA9063_REG_CHIP_ID,
> >> .range_max = DA9063_REG_CHIP_VARIANT,
> >> },
> >> };
> >>
> >> static const struct regmap_range da9063l_bb_writeable_ranges[] = {
> >> {
> >> .range_min = DA9063_REG_PAGE_CON,
> >> .range_max = DA9063_REG_PAGE_CON,
> >> }, {
> >> .range_min = DA9063_REG_FAULT_LOG,
> >> .range_max = DA9063_REG_VSYS_MON,
> >> }, {
> >> .range_min = DA9063_REG_SEQ,
> >> .range_max = DA9063_REG_ID_32_31,
> >> }, {
> >> .range_min = DA9063_REG_SEQ_A,
> >> .range_max = DA9063_REG_AUTO3_LOW,
> >> }, {
> >> .range_min = DA9063_REG_CONFIG_I,
> >> .range_max = DA9063_BB_REG_MON_REG_4,
> >> }, {
> >> .range_min = DA9063_BB_REG_GP_ID_0,
> >> .range_max = DA9063_BB_REG_GP_ID_19,
> >> },
> >> };
> >>
> >> static const struct regmap_range da9063l_bb_volatile_ranges[] = {
> >> {
> >> .range_min = DA9063_REG_PAGE_CON,
> >> .range_max = DA9063_REG_EVENT_D,
> >> }, {
> >> .range_min = DA9063_REG_CONTROL_A,
> >> .range_max = DA9063_REG_CONTROL_B,
> >> }, {
> >> .range_min = DA9063_REG_CONTROL_E,
> >> .range_max = DA9063_REG_CONTROL_F,
> >> }, {
> >> .range_min = DA9063_REG_BCORE2_CONT,
> >> .range_max = DA9063_REG_LDO11_CONT,
> >> }, {
> >> .range_min = DA9063_REG_DVC_1,
> >> .range_max = DA9063_REG_ADC_MAN,
> >> }, {
> >> .range_min = DA9063_REG_ADC_RES_L,
> >> .range_max = DA9063_REG_MON_A10_RES,
> >> }, {
> >> .range_min = DA9063_REG_SEQ,
> >> .range_max = DA9063_REG_SEQ,
> >> }, {
> >> .range_min = DA9063_REG_EN_32K,
> >> .range_max = DA9063_REG_EN_32K,
> >> }, {
> >> .range_min = DA9063_BB_REG_MON_REG_5,
> >> .range_max = DA9063_BB_REG_MON_REG_6,
> >> },
> >> };
> >>
> >> However this is a larger and more wide-ranging change compared to the
> >> one proposed by Marek, and would require other alterations to fit
> >> this in. Also I'm undecided to what it would really add apart from a
> >> new chip model: I have been told accessing the DA9063 RTC register locations
> >> has no effect in the DA9063L.
> >
> > Looking at this further, there is also a new IRQ regmap.
> > Again this comes down to whether a full chip model is needed or not.
> > If not, then the IRQ map does not need to be changed as given. Otherwise the
> > removal of the following:
> >
> > [DA9063_IRQ_ALARM] = {
> > .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> > .mask = DA9063_M_ALARM,
> > },
> > [DA9063_IRQ_TICK] = {
> > .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> > .mask = DA9063_M_TICK,
> > },
> >
> > prior to registering the IRQs in the chip model would be needed.
> > The new regmap_irq would be:
> >
> > static const struct regmap_irq da9063l_irqs[] = {
> > /* DA9063 event A register */
> > [DA9063L_IRQ_ONKEY] = {
> > .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> > .mask = DA9063_M_ONKEY,
> > },
> > [DA9063L_IRQ_ADC_RDY] = {
> > .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> > .mask = DA9063_M_ADC_RDY,
> > },
> > [DA9063L_IRQ_SEQ_RDY] = {
> > .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> > .mask = DA9063_M_SEQ_RDY,
> > },
> > /* DA9063 event B register */
> > [DA9063L_IRQ_WAKE] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_WAKE,
> > },
> > [DA9063L_IRQ_TEMP] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_TEMP,
> > },
> > [DA9063L_IRQ_COMP_1V2] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_COMP_1V2,
> > },
> > [DA9063L_IRQ_LDO_LIM] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_LDO_LIM,
> > },
> > [DA9063L_IRQ_REG_UVOV] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_UVOV,
> > },
> > [DA9063L_IRQ_DVC_RDY] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_DVC_RDY,
> > },
> > [DA9063L_IRQ_VDD_MON] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_VDD_MON,
> > },
> > [DA9063L_IRQ_WARN] = {
> > .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> > .mask = DA9063_M_VDD_WARN,
> > },
> > /* DA9063 event C register */
> > [DA9063L_IRQ_GPI0] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI0,
> > },
> > [DA9063L_IRQ_GPI1] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI1,
> > },
> > [DA9063L_IRQ_GPI2] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI2,
> > },
> > [DA9063L_IRQ_GPI3] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI3,
> > },
> > [DA9063L_IRQ_GPI4] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI4,
> > },
> > [DA9063L_IRQ_GPI5] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI5,
> > },
> > [DA9063L_IRQ_GPI6] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI6,
> > },
> > [DA9063L_IRQ_GPI7] = {
> > .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> > .mask = DA9063_M_GPI7,
> > },
> > /* DA9063 event D register */
> > [DA9063L_IRQ_GPI8] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI8,
> > },
> > [DA9063L_IRQ_GPI9] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI9,
> > },
> > [DA9063L_IRQ_GPI10] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI10,
> > },
> > [DA9063L_IRQ_GPI11] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI11,
> > },
> > [DA9063L_IRQ_GPI12] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI12,
> > },
> > [DA9063L_IRQ_GPI13] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI13,
> > },
> > [DA9063L_IRQ_GPI14] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI14,
> > },
> > [DA9063L_IRQ_GPI15] = {
> > .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> > .mask = DA9063_M_GPI15,
> > },
> > };
>
> We can probably do the same trick with the regmaps and irqmaps as with the
> rest, that is, reorder them and register only a smaller portion ?

I like the "reorder and only register a smaller portion" trick. But it wouldn't work
with what I gave earlier today, without some modification.
For instance, the first register readable entry range in the DA9063 BB is:

static const struct regmap_range da9063_bb_readable_ranges[] = {
{
.range_min = DA9063_REG_PAGE_CON,
.range_max = DA9063_BB_REG_SECOND_D,
}, {

But for the DA9063L, this first range entry would be changed, not removed:

static const struct regmap_range da9063l_bb_readable_ranges[] = {
{
.range_min = DA9063_REG_PAGE_CON,
.range_max = DA9063_REG_MON_A10_RES,
}, {

So it's not all-or-nothing. But possibly it could be made to work if those ranges were split
into two pieces.

However, it might get messy to maintain in future -- sometimes register ranges need to be
updated with new components or if a new feature is added -- usually I need to work it
all out on paper with the full register map. Splitting up ranges might make it a little
messier. But, it's not impossible.

For the DA9062 and DA9061 this was done using separate ranges and using the macro
regmap_reg_range(). It's not that messy to read, e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mfd/da9062-core.c?h=next-20180517#n367

Regards,
Steve

Next message: Sebastian Reichel: "[PATCH 0/2] serial: imx: more shutdown cleanups"
Previous message: Christian Brauner: "Re: [REVIEW][PATCH 4/6] fs: Allow superblock owner to access do_remount_sb()"
In reply to: Marek Vasut: "Re: [PATCH 6/6] mfd: da9063: Add DA9063L support"
Next in thread: Marek Vasut: "Re: [PATCH 6/6] mfd: da9063: Add DA9063L support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]