Hi Paul,
> Am 08.11.2021 um 17:30 schrieb Paul Cercueil <p...@crapouillou.net>:
>
> Hi,
>
> Le lun., nov. 8 2021 at 16:29:11 +0100, H. Nikolaus Schaller
> <h...@goldelico.com> a écrit :
>> Bnjour Paul,
>>> Am 08.11.2021 um 13:20 schrieb Paul Cercueil <p...@crapouillou.net>:
>>> Hi,
>>>> e.g. jz4770.dtsi:
>>>> lcd: lcd-controller@13050000 {
>>>> compatible = "ingenic,jz4770-lcd";
>>>> reg = <0x13050000 0x300>;
>>>> or jz4725b.dtsi:
>>>> lcd: lcd-controller@13050000 {
>>>> compatible = "ingenic,jz4725b-lcd";
>>>> reg = <0x13050000 0x1000>;
>>>> So max_register becomes 0x300 or 0x1000 but not
>>>> #define JZ_REG_LCD_SIZE1 0x12c
>>>> .max_reg = JZ_REG_LCD_SIZE1,
>>>> And therefore wastes a lot of regmap memory.
>>> "regmap memory"? ...
>> regmap allocates memory for its cache. Usually the total amount specified in
>> the reg property.
>
> We are not using any register cache here.
>
>>>> Do you want this? DTS should not be reduced (DTS should be kept as stable
>>>> as possible), since the reg property describes address mapping - not how
>>>> many bytes are really used by registers or how big a cache should be
>>>> allocated (cache allocation size requirements are not hardware
>>>> description).
>>> The DTS should list the address and size of the register area. If your last
>>> register is at address 0x12c and there's nothing above, then the size in
>>> DTS should be 0x130.
>> If I look into some .dtsi it is sometimes that way but sometimes not. There
>> seems to be no consistent rule.
>> So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi and
>> jz4725b.dtsi as well (as mentioned above: this is beyond the scope of my
>> project)?
>
> You could update them if you wanted to, but there is no need to do it here.
Hm. Then we are changing the .max_register initialization to a much bigger
value.
>
>>>> But here are good news:
>>>> I have a simpler and less invasive proposal. We keep the
>>>> devm_regmap_init_mmio code as is and just increase its .max_register from
>>>> JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This
>>>> wastes a handful bytes for all non-jz4780 chips but less than using the
>>>> DTS memory region size. And is less code (no entry in soc_info tables, no
>>>> modifyable copy) and faster code execution than all other proposals.
>>>> This is then just a single-line change when introducing the jz4780. And no
>>>> "preparation for adding jz4780" patch is needed at all. No patch to split
>>>> out for separate review.
>>>> Let's go this way to get it eventually finalized. Ok?
>>> No.
>> Look friend, if you explain your "no" and what is wrong with my arguments,
>> it helps to understand your decisions and learn something from them. A plain
>> "no" does not help anyone.
>
> I answered just "no" because I felt like I explained already what I wanted to
> see in the previous email.
>
> By using a huge number as the .max_register, we do *not* waste additional
> memory. Computing the value of the .max_register field does not add any
> overhead, either.
>
> The .max_register is only used for boundary checking. To make sure that
> you're not calling regmap_write() with an invalid register. That's all there
> is to it.
Ah, now I understand our disconnect. So far I have used regmaps mainly for i2c
devices and there is caching to avoid redundant i2c traffic...
So I just assumed wrongly that the regmap driver also allocates some
buffer/cache here. Although it does not initialize .cache_type (default:
REGCACHE_NONE).
>
>> So to summarize: if you prefer something which I consider worse, it is ok
>> for me... In the end you are right - you are the maintainer, not me. So you
>> have to live with your proposals.
>> Therefore, I have prepared new variants so you can choose which one is
>> easier to maintain for you.
>> Note that they are both preparing for full jz4780-lcdc/hdmi support but in
>> very different ways:
>> Variant 1 already adds some jz4780 stuff while Variant 2 just prepares for
>> it.
>> Variant 2 is not tested (except to compile). So it needs some Tested-by:
>> from someone with access to hardware. IMHO it is more invasive.
>> And don't forget: DTB could be in ROM or be provided by a separate
>> bootloader... So we should not change it too often (I had such discussions
>> some years ago with maintainers when I thought it is easier to change DTS
>> instead of code).
>> Variant 3 would be to not separate this. As proposed in [PATCH v5 2/7].
>> (Finally, a Variant 3b would be to combine the simple change from Variant 1
>> with Variant 3).
>> So what is your choice?
>
> Variant 4: the variant #2 without the changes to the DTSI files.
Hm. If there is no cache and we can safely remove tight boundary checking (by
JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) why do we still need
the max_register calculation from DTSI specifically for jz4780 and at all?
So what about:
Variant 5: set .max_register = 0x1800, i.e. "big enough for everyone" (includes
z4780 gamma and vee registers) + no DTSI changes (+ no jz4780 register
constants like Variant 1)
+ no DTSI changes
+ no calculation from DTSI needed
+ single separate patch to prepare for jz4780 but not included in jz4780 patch
BR and thanks,
Nikolaus
