From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 184B4C6FD1F for ; Fri, 29 Mar 2024 19:01:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:Date:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=jtv164RCoU/Mlqv1wQ2eeQdW0tAfog9kcD0yueLWDis=; b=FLJaJNNmeg5ytn ioQekxsWAzY7zPwwLGvD4NikusWb0YVBL9b0w2WfGvzeoKNfMyFTQLcxSzidssfUmyU0h1Rj9cXWl FFPpLjR0VlU0pjKUr/9xtIgRj3FKms9RvIeXolSc9+ypxcmDoV+5GfhVvL/d/ITt2Xe/TR70PYWHO Ue2gLcj3qtXijOVirENDFbUIpDjo419ri9XGxUbHzv8/anMuGDvRMZnERhN9R63TiUAfEYwu1vQF7 /051uQKwccX1QUxGbQwZ2EnnGWZAqJ03rfJfeHk0quo20lSpBF1HXE+yJYp08buB3J7HiNUuBb3eX u/GQn08dxnK4Df3Xwyvw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rqHTF-00000001h9r-3rsv; Fri, 29 Mar 2024 19:00:57 +0000 Received: from fgw20-7.mail.saunalahti.fi ([62.142.5.81]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rqHTC-00000001h7r-3ocn for linux-arm-kernel@lists.infradead.org; Fri, 29 Mar 2024 19:00:56 +0000 Received: from localhost (88-113-26-217.elisa-laajakaista.fi [88.113.26.217]) by fgw20.mail.saunalahti.fi (Halon) with ESMTP id a7c36e9b-edfe-11ee-b3cf-005056bd6ce9; Fri, 29 Mar 2024 21:00:49 +0200 (EET) From: Andy Shevchenko Date: Fri, 29 Mar 2024 21:00:48 +0200 To: "Peng Fan (OSS)" Cc: Sudeep Holla , Cristian Marussi , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Linus Walleij , Dan Carpenter , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, Peng Fan , Oleksii Moisieiev Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Message-ID: References: <20240323-pinctrl-scmi-v6-0-a895243257c0@nxp.com> <20240323-pinctrl-scmi-v6-3-a895243257c0@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240323-pinctrl-scmi-v6-3-a895243257c0@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240329_120055_301085_E343C4FC X-CRM114-Status: GOOD ( 21.56 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti: > From: Peng Fan > > Add basic implementation of the SCMI v3.2 pincontrol protocol. ... > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o Actually you want to have := here. > +scmi-protocols-y += pinctrl.o > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y) Side note: The -objs has to be -y ... > +#include > +#include > +#include This is semi-random list of headers. Please, follow IWYU principle (include what you use). There are a lot of inclusions I see missing (just in the context of this page I see bits.h, types.h, and asm/byteorder.h). ... > +enum scmi_pinctrl_protocol_cmd { > + PINCTRL_ATTRIBUTES = 0x3, > + PINCTRL_LIST_ASSOCIATIONS = 0x4, > + PINCTRL_SETTINGS_GET = 0x5, > + PINCTRL_SETTINGS_CONFIGURE = 0x6, > + PINCTRL_REQUEST = 0x7, > + PINCTRL_RELEASE = 0x8, > + PINCTRL_NAME_GET = 0x9, > + PINCTRL_SET_PERMISSIONS = 0xa Leave trailing comma as it's not a termination. > +}; ... > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph, > + struct scmi_pinctrl_info *pi) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_pinctrl_protocol_attributes *attr; > + > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), > + &t); This looks much better on a single line. > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high); > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low); > + pi->nr_pins = GET_PINS_NR(attr->attributes_low); > + } > + > + ph->xops->xfer_put(ph, t); > + return ret; > +} ... > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), > + sizeof(*rx), &t); Possible to have on a single line (if you use relaxed 100 limit). Or (re)split it more logically: ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), sizeof(*rx), &t); > + if (ret) > + return ret; ... > + /* > + * If supported overwrite short name with the extended one; > + * on error just carry on and use already provided short name. > + */ > + if (!ret && ext_name_flag) Please, use standard pattern, i.e. if (ret) return ret; > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector, > + (u32 *)&type, name, Why is an explicit casting needed? > + SCMI_MAX_STR_SIZE); > + return ret; ... > + iter = ph->hops->iter_response_init(ph, &ops, size, > + PINCTRL_LIST_ASSOCIATIONS, > + sizeof(struct scmi_msg_pinctrl_list_assoc), > + &ipriv); > + Redundant blank line. > + if (IS_ERR(iter)) > + return PTR_ERR(iter); ... > + if (p->flag == 1) > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index); > + else if (!p->flag) Be consistent, i.e. if (p->flag == 0) > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]); ... > + st->num_remaining = le32_get_bits(r->num_configs, > + GENMASK(31, 24)); One line? ... > + if (!p->flag) { > + if (p->config_types[0] != > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0))) > + return -EINVAL; > + } else if (p->flag == 1) { > + p->config_types[st->desc_index + st->loop_idx] = > + le32_get_bits(r->configs[st->loop_idx * 2], > + GENMASK(7, 0)); With a temporary variable for r->configs[st->loop_idx * 2] the above can be written in much better way. > + } else if (p->flag == 2) { > + return 0; > + } > + p->config_values[st->desc_index + st->loop_idx] = > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]); For the sake of consistency with the above suggestion also temporary for next config value. ... > + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET, > + sizeof(struct scmi_msg_settings_get), > + &ipriv); > + Redundant blank line. > + if (IS_ERR(iter)) > + return PTR_ERR(iter); ... > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph, > + u32 selector, > + struct scmi_group_info *group) > +{ > + int ret; > + if (!group) > + return -EINVAL; When the above is not a dead code? > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, > + group->name, > + &group->nr_pins); > + if (ret) > + return ret; > + > + if (!group->nr_pins) { > + dev_err(ph->dev, "Group %d has 0 elements", selector); > + return -ENODATA; > + } > + > + group->group_pins = kmalloc_array(group->nr_pins, > + sizeof(*group->group_pins), > + GFP_KERNEL); > + if (!group->group_pins) > + return -ENOMEM; > + > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE, > + group->nr_pins, group->group_pins); > + if (ret) { > + kfree(group->group_pins); > + return ret; > + } > + > + group->present = true; > + return 0; > +} ... > + ret = scmi_pinctrl_get_group_info(ph, selector, > + &pi->groups[selector]); One line? > + if (ret) > + return ret; ... > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, > + func->name, > + &func->nr_groups); At least last two lines can be joined. > + if (ret) > + return ret; ... > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, > + pin->name, NULL); It's pleany of room on the previous line. > + if (ret) > + return ret; ... > + ret = scmi_pinctrl_get_pin_info(ph, selector, > + &pi->pins[selector]); Ditto. > + if (ret) > + return ret; ... > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph) > +{ > + int ret; > + u32 version; > + struct scmi_pinctrl_info *pinfo; > + > + ret = ph->xops->version_get(ph, &version); > + if (ret) > + return ret; > + > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n", > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > + > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL); Huh?! Please, get yourself familiar with the scope of devm APIs. > + if (!pinfo) > + return -ENOMEM; > + > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > + if (ret) > + return ret; > + > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins, > + sizeof(*pinfo->pins), > + GFP_KERNEL); > + if (!pinfo->pins) > + return -ENOMEM; > + > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups, > + sizeof(*pinfo->groups), > + GFP_KERNEL); > + if (!pinfo->groups) > + return -ENOMEM; > + > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions, > + sizeof(*pinfo->functions), > + GFP_KERNEL); > + if (!pinfo->functions) > + return -ENOMEM; > + > + pinfo->version = version; > + > + return ph->set_priv(ph, pinfo, version); > +} > + > +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph) > +{ > + int i; > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > + > + for (i = 0; i < pi->nr_groups; i++) { > + if (pi->groups[i].present) { > + kfree(pi->groups[i].group_pins); > + pi->groups[i].present = false; > + } > + } > + > + for (i = 0; i < pi->nr_functions; i++) { > + if (pi->functions[i].present) { > + kfree(pi->functions[i].groups); This is wrong in conjunction with the above. > + pi->functions[i].present = false; > + } > + } > + > + return 0; > +} ... > +static const struct scmi_protocol scmi_pinctrl = { > + .id = SCMI_PROTOCOL_PINCTRL, > + .owner = THIS_MODULE, > + .instance_init = &scmi_pinctrl_protocol_init, > + .instance_deinit = &scmi_pinctrl_protocol_deinit, > + .ops = &pinctrl_proto_ops, > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, > +}; > + Redundant blank line. > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl) -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel