Can't give Atmel motives, but definitely in their recent ASF code they use the .reg stuff instead of the .bit one. If you have a cleaner solution to propose, I'll be happy to apply it.

Well, in fact one comment sheds some light:

// __I to avoid read-modify-write on write-to-clear register

So it's basically optimization vs check enforcing.

Yeah, I saw that comment too. But to me it does not make sense.
When I said "cleaner" I meant that the old code was cleaner.
If Atmel really wanted to avoid something, then why was it only needed for SAML21? And why is it OK to not avoid it when writing to the .reg part of the union?

The reason I bring this all up is that we're now going to modify our (perfectly working and nicely written) code because of something that Atmel did in their updated header files. But we don't know why they changed it.

Please, let's try to get a more practical approach.

My PR adds support for SAM L21 version B CPUs and only modifies some 20-odd lines of the drivers code. I2C support for SAM L21 is pending because of this PR.

I would love someone to test it on real hardware, since I only possess SAML21E18B boards.

@keestux : The comment in Atmel code is extremely clear. If you cannot understand it, then bring out your disassembler and look for yourself where the difference in code is. I personally did not bother - call me an insensible brute, but I really can't see where this supposed lost beauty is. Better yet, if this is so important to you, you might want to produce a prettier working code, or at least hint me on how to do it.

I've tried your PR on saml21 Xplained board which use a SAML21J18A.
I've run tests/periph_rtt, tests/periph_rtc and tests/periph_uart. They all work so far.

But I notice a time offset each 10s in periph_rtt test... I've re-tested with and without this PR and the behavior is the same, so I think this problem is out the scope of this PR.

@dylad , thanks a lot - glad to know that I've not broken anything that worked.

@keestux I can understand your frustration but it seems Atmel does this modification in the whole ASF 3.33 not only for SAML21. Maybe it is time to update all the Atmel CMSIS files of RIOT to the same version ?

If we really must avoid read-modify-write then maybe the current change in this PR is not correct either. Have a look at the SAMD21 code. For example uart.c has this equivalent piece.

static inline void irq_handler(int dev)
{
    SercomUsart *uart = _uart(dev);
...
    else if (uart->INTFLAG.reg & SERCOM_USART_INTFLAG_ERROR) {
        /* clear error flag */
        uart->INTFLAG.reg = SERCOM_USART_INTFLAG_ERROR;
    }

In other words there is no |= but a plain assignment. This guarantees a write without a read first.

BTW, I did just now look at the assembly code and that is why I am beginning to understand why they want the __I on those bitfields. Basically writing to a bitfield is doing (at assembly level) the exact same as the or-ing the .reg part of the union with a constant (say 0x80). So the new code isn't any better, except that the compiler doesn't complain anymore.

@keestux , many thanks for taking your time and dig into this matter further.

I also thought that OR-ing would defeat the optimization, but did not try direct assignment because I really have no clue about what the side-effects are. In principle, assigment could flip other bits too - which is exactly the concern you expressed in the first place.

If this is used correctly elsewhere, as you show for D21, then I can rework my patch in the same fashion.

Antonio

Hello,
is this PR being still regarded as useful? Personally, I'm waiting for it to be integrated in order to add I2C support for SAM L21 on top of it.

Thanks a lot,

Antonio

Seems Murdock build failed.
@ant9000 maybe you should remove those whitespaces like for #5743 ?

@dylad removed. Sorry for the delay - I thought I did it before.

AFAICT, this PR should now be OK: how about actually merging it?

I have already updated my patches for SAML21 I2C support to work on top of it, and I'm just waiting for inclusion before opening a new PR.

Thanks a lot!

@haukepetersen green lights everywhere! Does your ACK hold? Please merge!

Murdock is happy -> let's go!

Just added on top of this PR: SAM L21 i2c support #6332