Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2024-11-12 at 17:34 +0200, Nemanov, Michael wrote:
> 
> > > +static int parse_control_message(struct cc33xx *cc,
> > > +				 const u8 *buffer, size_t buffer_length)
> > > +{
> > > +	u8 *const end_of_payload = (u8 *const)buffer + buffer_length;
> > > +	u8 *const start_of_payload = (u8 *const)buffer;
> > 
> > I don't think the "u8 *const" is useful here, and the cast is awkward.
> > If anything you'd want "const u8 *const" (which should make it not need
> > the cast), but the const you have adds no value... do you even know what
> > it means? ;-)
> > 
> 
> My intent was to express that start and end pointers are fixed and will 
> not change in the loop below. When reading this again I agree this hurts 
> more than it helps, I'll drop it.

Well, I don't even mind the const so much rather than the cast, I'd
probably not have commented on it if it were

	const u8 *const end_of_payload = buffer + buffer_length;
	const u8 *const start_of_payload = buffer;

I'd still think the second const (for the variable) isn't all that
useful, but really the lack of first const (for the object pointed to)
makes the casts necessary and (IMHO) that's what hurts.

> const u8 *buffer in the prototype illustrates that parse_control_message 
> will not change the data so I'll keep it if there a re no objections.

Sure.

> > > +	struct NAB_header *nab_header;
> > 
> > surely checkpatch complained about CamelCase or so with the struct name
> > like that?
> > 
> 
> Double-checked, no warnings from checkpatch:

Hah, ok :) I'm surprised because it complained about _Generic in my
patch, and that's something you really can't even change since it's C11
standard ...

johannes