Re: [PATCH 1/2] firmware: scmi: Fix setting the function

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, Mar 27, 2026 at 12:45:00PM +0100, Michal Simek wrote:
> 
> 
> On 3/26/26 13:08, Dan Carpenter wrote:
> > Set BIT(10) when the function needs to be set, otherwise the setting is
> > ignored.
> > 
> > Fixes: 1048331f5d3c ("scmi: pinctrl: add pinctrl driver for SCMI")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >   drivers/firmware/scmi/pinctrl.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/firmware/scmi/pinctrl.c b/drivers/firmware/scmi/pinctrl.c
> > index 47f7a8ad9b81..e670538c87f9 100644
> > --- a/drivers/firmware/scmi/pinctrl.c
> > +++ b/drivers/firmware/scmi/pinctrl.c
> > @@ -259,6 +259,8 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
> >   	in->attr = 0;
> >   	in->attr |= FIELD_PREP(GENMASK(9, 2), num_configs);
> >   	in->attr |= FIELD_PREP(GENMASK(1, 0), select_type);
> > +	if (function_id != SCMI_PINCTRL_FUNCTION_NONE)
> > +		in->attr |= BIT(10);
> 
> hm shouldn't be macros to use for holding that value instead?
> 
> As I see GENMASK above is the same case.
> 

I think adding macros here would make the code less readable...  If we
were going to re-use them then, sure.  But really, the code is shoving
the num_configs into bits 2-9, there isn't any special meaning beyond
that.

> >   	memcpy(in->configs, configs, num_configs * sizeof(u32) * 2);
> 
> Another magic here.

Sure, I'll send this patch on Monday.

regards,
dan carpenter

diff --git a/drivers/firmware/scmi/pinctrl.c b/drivers/firmware/scmi/pinctrl.c
index e670538c87f9..2ae02ffc81b8 100644
--- a/drivers/firmware/scmi/pinctrl.c
+++ b/drivers/firmware/scmi/pinctrl.c
@@ -245,7 +245,9 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
 		.out_msg = (u8 *)&out,
 		.out_msg_sz = sizeof(out),
 	};
-	size_t in_sz = sizeof(*in) + (num_configs * sizeof(u32) * 2);
+	/* configs are type/value pairs of size u32 */
+	size_t config_size = (num_configs * sizeof(u32) * 2);
+	size_t in_sz = sizeof(*in) + config_size;
 	int ret;
 
 	in = kzalloc(in_sz, GFP_KERNEL);
@@ -261,7 +263,7 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
 	in->attr |= FIELD_PREP(GENMASK(1, 0), select_type);
 	if (function_id != SCMI_PINCTRL_FUNCTION_NONE)
 		in->attr |= BIT(10);
-	memcpy(in->configs, configs, num_configs * sizeof(u32) * 2);
+	memcpy(in->configs, configs, config_size);
 
 	ret = devm_scmi_process_msg(dev, &msg);
 	if (ret)