Re: [RFC v2 2/3] pinctrl: Implementation of the generic scmi-pinctrl driver
From: <hidden>
Date: 2023-05-05 20:35:29
Also in:
linux-gpio, lkml
Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
scmi-pinctrl driver implements pinctrl driver interface and using SCMI protocol to redirect messages from pinctrl subsystem SDK to SCP firmware, which does the changes in HW. This setup expects SCP firmware (or similar system, such as ATF) to be installed on the platform, which implements pinctrl driver for the specific platform. SCMI-Pinctrl driver should be configured from the device-tree and uses generic device-tree mappings for the configuration.
...
+#include <linux/device.h> +#include <linux/err.h>
+#include <linux/of.h>
I do not see any user of this header. Do you?
quoted hunk
+#include <linux/module.h> +#include <linux/seq_file.h> + +#include <linux/pinctrl/machine.h> +#include <linux/pinctrl/pinconf.h> +#include <linux/pinctrl/pinconf-generic.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinmux.h>
+#include <linux/scmi_protocol.h> +#include <linux/slab.h>
Please, move these two to the upper group of the generic headers.
quoted hunk
+struct scmi_pinctrl_funcs { + unsigned int num_groups; + const char **groups; +};
Please, use struct pinfunction. ...
+struct scmi_pinctrl {+ struct scmi_pinctrl_funcs *functions; + unsigned int nr_functions;
+ char **groups; + unsigned int nr_groups;
I'm not sure what is the difference to what "functions" above represent.
+};
...
quoted hunk
+static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev, + struct seq_file *s, + unsigned int offset) +{ + seq_puts(s, DRV_NAME); +}
What is the usefulness of this method? ...
quoted hunk
+static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, + unsigned int _pin, + unsigned long *config) +{ + int ret; + struct scmi_pinctrl *pmx; + enum pin_config_param config_type; + unsigned long config_value; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph || !config) + return -EINVAL; + + config_type = pinconf_to_config_param(*config); + + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE, + config_type, (u32 *)&config_value);
Endianess issue. This is, while likely working code, still ugly.
quoted hunk
+ if (ret) + return ret; + + *config = pinconf_to_config_packed(config_type, config_value); + + return 0; +}
...
+ err:
err_free.
quoted hunk
+ kfree(pmx->pins); + pmx->nr_pins = 0; + + return ret;
...
+static const struct scmi_device_id scmi_id_table[] = {
+ { SCMI_PROTOCOL_PINCTRL, "pinctrl" },+ { },No comma for the terminator entry.
+};
...
+ pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, + &ph);
Can be on one line.
+ if (IS_ERR(pinctrl_ops)) + return PTR_ERR(pinctrl_ops);
...
quoted hunk
+ if (pmx->nr_functions) { + pmx->functions = + devm_kcalloc(&sdev->dev, pmx->nr_functions, + sizeof(*pmx->functions), + GFP_KERNEL); + if (!pmx->functions) { + ret = -ENOMEM; + goto clean;
Interleaving devm_*() with non-devm_*() in such order is not a good idea.
quoted hunk
+ } + } + + if (pmx->nr_groups) { + pmx->groups = + devm_kcalloc(&sdev->dev, pmx->nr_groups, + sizeof(*pmx->groups), + GFP_KERNEL); + if (!pmx->groups) { + ret = -ENOMEM; + goto clean; + } + } + + return pinctrl_enable(pmx->pctldev); + +clean:
err_free:
quoted hunk
+ if (pmx) { + kfree(pmx->functions); + kfree(pmx->groups);
Ah, this is simply wrong.
quoted hunk
+ } + + kfree(pmx);
-- 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