On Mon, 4 Dec 2023 19:16:57 +0000
Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Mon, Dec 04, 2023 at 01:59:22PM -0500, Hugo Villeneuve wrote:
>
> > Based on tas2770.c, I am assuming I could redefine the code to look
> > like this:
>
> > /* SC16IS7XX register definitions */
> > #define SC16IS7XX_REG(page, reg) ((page * 128) + reg)
> >
> > #define SC16IS7XX_RHR_REG SC16IS7XX_REG(0, 0x00) /* RX FIFO */
> > #define SC16IS7XX_THR_REG SC16IS7XX_REG(0, 0x00) /* TX FIFO */
> > #define SC16IS7XX_IER_REG SC16IS7XX_REG(0, 0x01) /* Interrupt enable */
> > #define SC16IS7XX_IIR_REG SC16IS7XX_REG(0, 0x02) /* Interrupt Identification (read) */
> > #define SC16IS7XX_FCR_REG SC16IS7XX_REG(0, 0x02) /* FIFO control (write) */
> > #define SC16IS7XX_MCR_REG SC16IS7XX_REG(0, 0x04) /* Modem Control */
> > #define SC16IS7XX_LSR_REG SC16IS7XX_REG(0, 0x05) /* Line Status */
> > #define SC16IS7XX_MSR_REG SC16IS7XX_REG(0, 0x06) /* Modem Status */
> > #define SC16IS7XX_SPR_REG SC16IS7XX_REG(0, 0x07) /* Scratch Pad */
> > ...
> > #define SC16IS7XX_EFR_REG SC16IS7XX_REG(1, 0x02) /* Enhanced Features */
> > #define SC16IS7XX_XON1_REG SC16IS7XX_REG(1, 0x04) /* Xon1 word */
> > #define SC16IS7XX_XON2_REG SC16IS7XX_REG(1, 0x05) /* Xon2 word */
> > #define SC16IS7XX_XOFF1_REG SC16IS7XX_REG(1, 0x06) /* Xoff1 word */
> > #define SC16IS7XX_XOFF2_REG SC16IS7XX_REG(1, 0x07) /* Xoff2 word */
>
> > static const struct regmap_range_cfg sc16is7xx_regmap_ranges[] = {
> > {
> > .range_min = 0,
> > .range_max = 1 * 128,
> > .selector_reg = SC16IS7XX_LCR_REG,
> > .selector_mask = 0xff,
> > .selector_shift = 0,
> > .window_start = 0,
> > .window_len = 128,
> > },
> > };
>
> That looks plausible - I'd tend to make the range just completely
> non-physical (eg, start at some unrepresentable value) so there's no
> possible ambiguity with a physical address. I'm also not clear why
> you've made the window be 128 registers wide if it's just this range
I simply took what was in tas2770.c as a starting point.
> from 2-7 that gets switched around, I'd do something more like
>
> #define SC16IS7XX_RANGE_BASE 0x1000
> #define SC16IS7XX_WINDOW_LEN 6
> #define SC16IS7XX_PAGED_REG(page, reg) \
> (SC16IS7XX_RANGE_BASE + (SC16IS7XX_WINDOW_LEN * page) + reg)
>
> .range_min = SC16IS7XX_RANGE_BASE,
> .range_max = SC16IS7XX_RANGE_BASE + (2 * SC16IS7XX_WINDOW_LEN),
> .window_start = 2,
> .window_len = SC16IS7XX_WINDOW_LEN,
>
> You could apply a - 2 offset to reg as well to make the defines for the
> registers clearer. The above means that any untranslated register you
> see in I/O traces should be immediately obvious (and more likely to trip
> up error handling and tell you about it if it goes wrong) and hopefully
> also makes it easier to reason about what's going on.
Ok.
> > But here, selecting the proper "page" is not obvious,
> > because to select page 1, we need to write a fixed value of 0xBF to
> > the LCR register.
>
> > And when selecting page 0, we must write the previous value that was
> > in LCR _before_ we made the switch to page 1...
>
> > How do we do that?
>
> That's one reason why having the range cover all the registers gets
> confusing. That said the code does have special casing for a selector
> register in the middle of the range since there were some devices that
> just put the paging register in the middle of the range it controls for
> some innovative region, the core will notice that this is a selector
> register and not do any paging for that register.
You are talking about the selector being inside the range, and I
understand that because I have looked at _regmap_select_page()
comments and logic.
But that is not was my question was about. Here a pseudo code
example to select "page" 1:
1. save original value of LCR register.
2. write 0xBF to LCR register
3. access desired register in page 1
4. restore original LCR value saved in step 1
How do you do that with regmap range?
Hugo.
