Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[BALATON Zoltan]

On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
> On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
> > This is a respin of Bernhard's v4 with Freescale eSDHC implemented
> > as an 'UNIMP' region. See v4 cover here:
> > https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
> >
> > Since v5:
> > - Rebased (ppc-next merged)
> > - Properly handle big-endian
> >
> > Since v4:
> > - Do not rename ESDHC_* definitions to USDHC_*
> > - Do not modify SDHCIState structure
> >
> > Supersedes: <20221031115402.91912-1-philmd@linaro.org>
>
> Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
> freeze for 7.2).

Could you please always use ppc-next to queue patches for the next 
upcoming version and ppc-7.2 for the current version? Unless this makes 
your workflow harder in which case ignore this but the reason I ask is 
because then it's enough for me to only track ppc-next if I need to rebase 
patches on that and don't have to add a new branch at every release 
(unless I have some patches to rebase on it during a freeze but that's 
less likely than rebasing on your queued patches for the next release xo 
using version for the current branch and keep next for the future versions 
makes more sense to me).

> BTW, checkpatch complained about this line being too long (83 chars):
>
>
> 3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to 
> e500plat)
> WARNING: line over 80 characters
> #150: FILE: hw/ppc/e500.c:1024:
> +                                    pmc->ccsrbar_base + 
> MPC85XX_ESDHC_REGS_OFFSET,
>
>
> The code except is this:
>
>    if (pmc->has_esdhc) {
>        create_unimplemented_device("esdhc",
>                                    pmc->ccsrbar_base + 
> MPC85XX_ESDHC_REGS_OFFSET,
>                                    MPC85XX_ESDHC_REGS_SIZE);
>
>
> To get rid of the warning we would need to make a python-esque identation 
> (line
> break after "(" ) or create a new variable to hold the sum. Both seems 
> overkill
> so I'll ignore the warning. Phil is welcome to re-send if he thinks it's 
> worth
> it.

Or you could break indentation and not start at the ( but 3 chars back. I.e.:

create_unimplemented_device("esdhc",
                         pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
                         MPC85XX_ESDHC_REGS_SIZE);

But I think it can be just ignored in this case.

> And I'll follow it up with my usual plea in these cases: can we move the line 
> size warning to 100 chars? For QEMU 8.0? Pretty please?

I think the consensus was to keep 80 columns if possible, this is good 
becuase you can open more files side by side (although it does not match 
well with the long _ naming convention of glib and qemu)  but we have a 
distinction between checkpatch warning and error in line length. I think 
it will give error at 90 chars but as long as it's just warns that means: 
fix it if you can but in rare cases if it's more readable with a slightly 
longer line then it is still acceptable. I think that's the case here, 
splitting the line would be less readable than a few chars longer line.

Regards,
BALATON Zoltan