Re: [PATCH] media: i2c: cvs: Add NVMem-based firmware update support

[Andy Shevchenko]

On Thu, Oct 01, 2026 at 04:03:50PM -0700, Vadillo, Miguel wrote:
> On 10/1/26 11:32 AM, Andy Shevchenko wrote:
> > On Wed, Sep 30, 2026 at 11:21:42AM -0700, Miguel Vadillo wrote:

...

> > > +	mutex_lock(&ctx->lock);
> > 
> > Why not guard()()? Also how ACQUIRE() macros are co-habit with goto:s?
> 
> You are right scoped_guard() should be the case here and to get rid of the
> gotos, this could be done like:
> ...
> 	scoped_guard(mutex, &ctx->lock) {

Why scoped_guard()? If you need something to be outside of the regular
guard()(), but double check that it's indeed the case, refactor to have to
functions, one with guard()() in it and one that wraps it.

> 		switch (val) {
> 		...
> 		}
> 
> 		nvm->auth_status = -ret;
> 	}

> 	if (ret)
> 		return ret;
> 
> 	if (do_uevent)
> 		kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> 
> 	return count;

...

> > >   struct icvs {
> > >   	struct i2c_client *i2c_client;
> > 
> > >   	int irq;
> > >   	wait_queue_head_t hostwake_event;
> > >   	bool hostwake_event_arg;
> > > +	struct icvs_nvm nvm;
> > >   };
> > 
> > Is `pahole` happy with the layout?
> 
> Yes. struct icvs_nvm is itself hole-free and fits in one cacheline.
> That being said, there seems to be other holes in the full struct from the
> existing implementation, this order could make it better
> ...

Better by `pahole` doesn't always mean better in all aspects. You have to also
check it in conjunction with the output of `bloat-o-meter`. And in some
(performance-critical) cases with the runtime performance tests.

> 	struct media_pad pads[ICVS_CSI_NUM_PADS];
> 	struct device_link *ipu_link;
> 	unsigned long quirks;
> 	struct gpio_desc *rst;
> 	struct gpio_desc *req;
> 	struct gpio_desc *resp;
> 	wait_queue_head_t hostwake_event;
> 	struct icvs_nvm nvm;
> 	struct icvs_dev_capabilities caps;
> 	u32 nr_of_lanes;
> 	enum icvs_resources res;
> 	int irq;
> 	bool prefix;
> 	bool hostwake_event_arg;
> 
> but maybe send as a separate patch since it is not related to the patch
> intent (?)

-- 
With Best Regards,
Andy Shevchenko