Tue, Jun 06, 2023 at 04:22:28PM +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.
...
> +++ b/drivers/pinctrl/Kconfig
> @@ -546,4 +546,15 @@ source "drivers/pinctrl/uniphier/Kconfig"
> source "drivers/pinctrl/visconti/Kconfig"
> source "drivers/pinctrl/vt8500/Kconfig"
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver controlled via SCMI interface"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
Sounds to me that u and v should be after S. Decrypting for your convenience,
the above is ordered and proposed change misses that.
> endif
Btw, what is this endif for and how does it affect your Kconfig option?
...
> +++ b/drivers/pinctrl/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o
> obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
> obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
> obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
Ditto.
> obj-y += actions/
> obj-$(CONFIG_ARCH_ASPEED) += aspeed/
...
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/seq_file.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +struct scmi_pinctrl_funcs {
> + unsigned int num_groups;
> + const char **groups;
> +};
struct pinfunction
...
> +struct scmi_pinctrl {
> + struct device *dev;
> + struct scmi_protocol_handle *ph;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_desc pctl_desc;
> + struct scmi_pinctrl_funcs *functions;
> + unsigned int nr_functions;
> + char **groups;
struct pingroup ?
> + unsigned int nr_groups;
> + struct pinctrl_pin_desc *pins;
> + unsigned int nr_pins;
> +};
...
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
Redundant blank line.
> + if (!pmx || !pmx->ph)
> + return NULL;
...
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
Ditto. And so on in a few more places.
...
> + pmx->functions[selector].groups[i] =
> + pinctrl_scmi_get_group_name(pmx->pctldev,
> + group_ids[i]);
It's okay to have this on a single line which takes only 81 character.
...
> +error:
Labels shoud be self-explanatory, i.e. they should tell what _will_ be when goto.
> + devm_kfree(pmx->dev, pmx->functions[selector].groups);
Red Flag. Please, elaborate.
> +
> + return ret;
...
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> + if (!pctldev)
> + return -EINVAL;
Huh?! When this is not a dead code?
Ditto for other places.
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + config_type = pinconf_to_config_param(configs[i]);
> + config_value = pinconf_to_config_argument(configs[i]);
> +
> + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
> + config_value);
> + if (ret) {
> + dev_err(pmx->dev, "Error parsing config %ld\n",
> + configs[i]);
> + break;
> + }
> + }
> +
> + return ret;
> +}
...
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
Why this strange parameter name?
> + unsigned long *config)
...
> + err_free:
This is better, but shows the inconsistency with the other goto label namings.
> + devm_kfree(pmx->dev, pmx->pins);
Red Flag. Please, elaborate.
> + pmx->nr_pins = 0;
> +
> + return ret;
...
> + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret) {
> + dev_err_probe(&sdev->dev, ret, "Failed to register pinctrl\n");
> + return ret;
return dev_err_probe(...);
> + }
...
> + pmx->functions =
> + devm_kcalloc(&sdev->dev, pmx->nr_functions,
This is perfectly a signle line.
Also with
struct device *dev = &sdev->dev;
at the top you may make the entire ->probe() look neater.
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
