Re: [PATCH 1/2] leds: bcm6328: improve write and read functions

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/23/2021 1:05 AM, Álvaro Fernández Rojas wrote:
> 
> 
>> El 23 feb 2021, a las 9:58, Pavel Machek <pavel@ucw.cz> escribió:
>>
>> Hi!
>>
>>>>> This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng
>>>>> and bcmgenet drivers.
>>>>> Both should also be inline functions.
>>>>
>>>>
>>>>
>>>>> -#ifdef CONFIG_CPU_BIG_ENDIAN
>>>>> -	iowrite32be(data, reg);
>>>>> -#else
>>>>> -	writel(data, reg);
>>>>> -#endif
>>>>> +	/* MIPS chips strapped for BE will automagically configure the
>>>>> +	 * peripheral registers for CPU-native byte order.
>>>>> +	 */
>>>>
>>>> Bad comment style.
>>>
>>> I just wanted to copy the same comment as the one in bcm2835-rng and bcmgenet…
>>> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60
>>> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88
>>>
>>
>> Yeah, but ideally you should not be copying comments; there should be
>> one central place which does it and does it right.
> 
> I’m open to suggestions :).
> Which central place would be a good place for you?

I did consider creating an include/linux/brcm/brcm_io.h header or
something like that but I am really not sure what the benefit would be.

As far as using _relaxed() this is absolutely correct because the bus
logic that connects the CPU to its on-chip registers is non re-ordering
non posted. That is true on the MIPS BE/LE and ARM when configured in LE
or BE.

We need the swapping for ARM because when running in ARM BE32, the data
is going to be in the host CPU endian, but the register bus is hard
wired to little endian.
-- 
Florian