Re: [PATCH v2 4/5] firmware: tegra: bpmp: Add MBWT BPMP helpers

[Thierry Reding <thierry.reding@kernel.org>]

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

On Thu, Jul 23, 2026 at 02:15:50PM +0900, Mikko Perttunen wrote:
> On Wednesday, July 22, 2026 8:05 PM Aniruddha Rao wrote:
[...]
> > diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
[...]
> > +int tegra_bpmp_mbwt_set(struct tegra_bpmp *bpmp, unsigned int instance,
> > +			unsigned int vc_type, unsigned int bandwidth)
> > +{
> > +	struct mrq_sochub_mbwt_request request = {
> > +		.cmd = CMD_SOCHUB_MBWT_SET_BW,
> > +		.set_bw = {
> > +			.instance = instance,
> > +			.vc_type = vc_type,
> > +			.bw = bandwidth,
> > +		},
> > +	};
> > +	struct tegra_bpmp_message msg = {
> > +		.mrq = MRQ_SOCHUB_MBWT,
> > +		.tx = {
> > +			.data = &request,
> > +			.size = sizeof(request),
> > +		},
> > +	};
> > +	int err;
> > +
> > +	err = tegra_bpmp_transfer(bpmp, &msg);
> > +	if (err) {
> > +		dev_err(bpmp->dev, "MBWT set bandwidth transfer failed: %d\n",
> > +			err);
> > +		return err;
> > +	}
> > +
> > +	if (msg.rx.ret)
> > +		return msg.rx.ret;
> > +
> > +	return 0;
> > +}
> > +
> >  static void tegra_bpmp_mrq_handle_ping(unsigned int mrq,
> >  				       struct tegra_bpmp_channel *channel,
> >  				       void *data)
> > -- 
> > 2.43.0
> > 
> 
> Since these are only used by the sysfs code, I would squash this patch 
> into the sysfs patch and move these functions into the sysfs file.

I don't mind having this in bpmp.c. It's a fairly small file and this
isn't a lot of code, so it doesn't have much of an impact either way.
Adding an extra file is a bit of extra overhead and it would make more
sense if we had a separate symbol to configure this out.

Putting it alongside the sysfs code is bad if there's only even an
inkling of a chance that we might want to call this from somewhere else,
like maybe setting some default policy during boot or something.

That said, I would place this somewhere different within bpmp.c. Maybe
atop the probe function would be more appropriate instead of in the
middle of these other utility functions, maybe slap a multi-line comment
on top and describe what these new functions do.

Thierry

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