Re: [PATCH v2 02/22] congatec: Include env.h to permit reading the environment

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

On Sat, May 03, 2025 at 03:27:50PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 3 May 2025 at 10:37, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, May 03, 2025 at 10:30:35AM -0600, Tom Rini wrote:
> > > On Fri, May 02, 2025 at 08:10:30PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 2 May 2025 at 09:04, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Fri, May 02, 2025 at 08:52:37AM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Fri, 2 May 2025 at 08:06, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Fri, May 02, 2025 at 07:11:56AM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Thu, 1 May 2025 at 09:33, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, May 01, 2025 at 09:04:45AM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Thu, 1 May 2025 at 08:06, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Apr 30, 2025 at 07:04:27PM -0600, Simon Glass wrote:
> > > > > > > > > > > > This file reads from the environment but does not include the correct
> > > > > > > > > > > > header. Update it.
> > > > > > > > > > > >
> > > > > > > > > > > > Drop the unnecessary config.h while we are here.
> > > > > > > > > > >
> > > > > > > > > > > Are you sure about that? It was explicitly added in commit
> > > > > > > > > > > ab61cc7d98f6 ("board: congatec: Remove <common.h> and add needed
> > > > > > > > > > > includes") probably because of CFG_SYS_... usage.
> > > > > > > > > >
> > > > > > > > > > Well I assumed it wouldn't build if it had it, but perhaps that is
> > > > > > > > > > incorrect? I understood that CFG_ things had to be values and you
> > > > > > > > > > couldn't #idef them?
> > > > > > > > >
> > > > > > > > > The cases where in that series I added config.h were for good reason,
> > > > > > > > > either failure to build or tricky size change reasons. All were
> > > > > > > > > intentional. An audit-and-remove of config.h would be it's own series as
> > > > > > > > > yes, there's perhaps not the need now as whatever complex chain was
> > > > > > > > > unwound.
> > > > > > > >
> > > > > > > > Oh dear, OK. I'll respin the series.
> > > > > > > >
> > > > > > > > Hmm but I see you have sent a series that does some similar tweak[1].
> > > > > > > > What was the goal there?
> > > > > > >
> > > > > > > I fixed your problem. Please don't bother re-spinning this series, it's
> > > > > > > not clear what problems it's solving correctly.
> > > > > >
> > > > > > OK thanks. The problem was trying to include net.h in include/efi.h:
> > > > > >
> > > > > > https://patchwork.ozlabs.org/project/uboot/cover/20250501010456.3930701-1-sjg@chromium.org/
> > > > >
> > > > > Adding headers to headers needs extra care (so that we don't end up with
> > > > > massive implicit include chains).
> > > > >
> > > > > > So long as that works, we're fine. Having said that, even if it were a
> > > > > > problem, we could always refactor things to put the Ethernet decls in
> > > > > > a separate header.
> > > > >
> > > > > Yes, if there's some problem down the line then some refactoring is
> > > > > likely the thing to look at, not adding more headers to C files (the
> > > > > right path is to unwind the header chain and fix C files, your series
> > > > > doesn't remove env.h from any header).
> > > >
> > > > I hope we haven't got confused here.
> > > >
> > > > My series removed env.h from being *transitively* included. I had to
> > > > add it to C files due to that.
> > >
> > > Yes, which may or may not be helpful. A transitive include in a C file
> > > is not terrible, otherwise many files will have 20+ includes. A
> > > transitive include in header file, which in turn pulls in another which
> > > pulls in another, is a problem. And the header should be unwound and C
> > > files then fixed.
> > >
> > > > Your series doesn't help with including net.h in efi.h - you can try
> > > > that and see that it still fails.
> > >
> > > Perhaps I took it out too quickly then, I'd have sworn I build sandbox
> > > with that change added.
> >
> > Yeah, I guess I removed it too quick and missed fixing
> > drivers/usb/gadget/epautoconf.c still, along with whatever the right
> > answer is for unwinding the exfat oddity. Likely seeing what the pain is
> > over using the normal byteswap headers instead.
> 
> OK, are you thinking of taking a look at that? Or do you think it
> would be better to change exfat to use the linux/byteorder stuff
> (which I somewhat prefer)?

Yes, I would like to hear Marek's thoughts on changing exfat to use the
normal byteswap functions.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]