Re: [RFC v2 2/3] pinctrl: Implementation of the generic scmi-pinctrl driver

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hello Andy,

On 05.05.23 23:35, andy.shevchenko@xxxxxxxxx wrote:
> 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?
> 
Yes, thanks. Removing

>> +#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.
> 
Thanks, fixed.

>> +struct scmi_pinctrl_funcs {
>> +	unsigned int num_groups;
>> +	const char **groups;
>> +};
> 
> Please, use struct pinfunction.
>
I can't use pincfunction here because it has the following groups 
definition:
const char * const *groups;

Which is meant to be constantly allocated.
So I when I try to gather list of groups in 
pinctrl_scmi_get_function_groups I will receive compilation error.

> ...
> 
>> +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.
> 

The difference is that each function has a list of group names, so we 
have to get name for each group in this function. I'm saving array to 
avoid extra SCMI calls to gather groups in function.

>> +};
> 
> ...
> 
>> +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?
> 

Removed
> ...
> 
>> +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.
>

Fixed.

>> +	if (ret)
>> +		return ret;
>> +
>> +	*config = pinconf_to_config_packed(config_type, config_value);
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> + err:
> 
> err_free.
> 
Fixed

>> +	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.
> 
Removed.

>> +};
> 
> ...
> 
>> +	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
>> +						&ph);
>  > Can be on one line.

Fixed
> 
>> +	if (IS_ERR(pinctrl_ops))
>> +		return PTR_ERR(pinctrl_ops);
> 
> ...
> 
>> +	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.
> 
Thanks, fixed.

>> +		}
>> +	}
>> +
>> +	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:

removed.
> 
>> +	if (pmx) {
>> +		kfree(pmx->functions);
>> +		kfree(pmx->groups);
> 
> Ah, this is simply wrong.
> 
Thanks, removed.
>> +	}
>> +
>> +	kfree(pmx);
>