Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

Hi,

Le lun., nov. 8 2021 at 11:52:20 +0100, H. Nikolaus Schaller 
<hns@xxxxxxxxxxxxx> a écrit :
> Hi Paul,
>
> > >  Am 08.11.2021 um 10:37 schrieb Paul Cercueil 
> > > <paul@xxxxxxxxxxxxxxx>:
> > >
> > >  Well, it was atomic: "add jz4780+hdmi functionality" or not. Now 
> > > we separate into "preparation for adding jz4780" and "really 
> > > adding". Yes, you can split atoms into quarks...
> >
> >  And that's how it should be done. Lots of small atomic patches are 
> > much easier to review than a few big patches.
>
> I doubt that in this case especially as it has nothing to do with 
> jz4780...

It has nothing to do with JZ4780 and that's exactly why it should be a 
separate patch.

> But I have a proposal for a better solution at the end of this mail.
>
> > > >  Note that you can do even better, set the .max_register field 
> > > > according to the memory resource you get from DTS. Have a look at 
> > > > the pinctrl driver which does exactly this.
> > >  That is an interesting idea. Although I don't see where
> > >  
> > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171
> > >  does make use of the memory resource from DTS. It just reads two 
> > > values from the ingenic_chip_info instead of one I do read from 
> > > soc_info.
> >
> >  It overrides the .max_register from a static regmap_config instance.
>
> To be precise: it overrides .max_register of a copy of a static 
> regmap_config instance (which has .max_register = 0).
>
> >  You can do the same,
>
> We already do the same...
>
> >  calculating the .max_register from the memory resource you get from 
> > DT.
>
> I can't see any code in pinctrl-ingenic.c getting the memory resource 
> that from DT. It calculates it from the ingenic_chip_info tables 
> inside the driver code but not DT.
>
> >  I'm sure you guys can figure it out.
>
> Ah, we have to figure out. You are not sure yourself how to do it? 
> And it is *not* exactly like the pinctrl driver (already) does? 
> Please give precise directions in reviews and not vague research 
> homework. Our time is also valuable. Sorry if I review your reviews...
>
> Anyways I think you roughly intend (untested):
>
> 	struct resource *r;
>
> 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	regmap_config.max_register = r.end - r.start;

Replace the "devm_platform_ioremap_resource" with 
"devm_platform_get_and_ioremap_resource" to get a pointer to the 
resource.

Then the .max_register should be (r.end - r.start - 4) I think.

And lose the aggressivity. It's not going to get you anywhere, 
especially since I'm the one who decides whether or not I should merge 
your patches. You want your code upstream, that's great, but it's your 
responsability to get it to shape so that it's eventually accepted.

> But I wonder how that could work at all (despite adding code 
> execution time) with:

Code execution time? ...

> 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"? ...

> 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.

> 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.

Cheers,
-Paul