Wed, Jul 26, 2023 at 05:04:03PM +0800, TY Chang kirjoitti:
> The RTD SoCs share a similar design for pinmux and pinconfig.
> This common pinctrl driver supports different variants within the RTD
> SoCs.
> +config PINCTRL_RTD
> + tristate "Realtek DHC core pin controller driver"
Why is it user-visible?
> + depends on ARCH_REALTEK
> + default y
Why?
> + select PINMUX
> + select GENERIC_PINCONF
...
> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
Not used or not needed, use proper platform driver APIs
> +#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>
Move this group outside generic headers...
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
+ Blank line. You have missing headers (like types.h)/
...here.
> +#include "../core.h"
> +#include "../pinctrl-utils.h"
+ Blank line.
> +#include "pinctrl-rtd.h"
...
> +static const struct pinconf_generic_params rtd_custom_bindings[] = {
> + {"realtek,pdrive", RTD_P_DRIVE, 0},
> + {"realtek,ndrive", RTD_N_DRIVE, 0},
> + {"realtek,dcycle", RTD_D_CYCLE, 0},
Use C99 initializers.
> +};
...
> +static void rtd_pinctrl_dbg_show(struct pinctrl_dev *pcdev,
> + struct seq_file *s,
> + unsigned int offset)
> +{
> + struct rtd_pinctrl *data = pinctrl_dev_get_drvdata(pcdev);
> + const struct rtd_pin_desc *mux = &data->info->muxes[offset];
> + const struct rtd_pin_mux_desc *func;
> + u32 val;
> + u32 mask;
> + u32 pin_val;
> + int is_map;
Actually it's boolean. Why int?
> + if (!mux->name) {
> + seq_puts(s, "[not defined]");
> + return;
> + }
> + val = readl_relaxed(data->base + mux->mux_offset);
> + mask = mux->mux_mask;
> + pin_val = val & mask;
> +
> + is_map = 0;
> + func = &mux->functions[0];
> + seq_puts(s, "function: ");
> + while (func->name) {
> + if (func->mux_value == pin_val) {
> + is_map = 1;
> + seq_printf(s, "[%s] ", func->name);
> + } else {
> + seq_printf(s, "%s ", func->name);
> + }
> + func++;
> + }
This can be a long list, why not simply print only mapped function?
> + if (!is_map)
> + seq_puts(s, "[not defined]");
> +}
...
> +static const struct rtd_pin_desc *rtd_pinctrl_find_mux(struct rtd_pinctrl *data, unsigned int pin)
> +{
> + if (!data->info->muxes[pin].name)
> + return &data->info->muxes[pin];
> +
> + return NULL;
Can the error (?) case be handled first?
if (data->info->muxes[pin].name)
return NULL;
> +}
...
> +static void rtd_pinctrl_update_bits(struct rtd_pinctrl *data, unsigned int offset,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned int reg = readl_relaxed(data->base + offset);
> + reg &= ~mask;
> + reg |= (mask & val);
Please, use idiomatic one liner:
reg = (reg & ~mask) | (val & mask);
> + writel_relaxed(reg, data->base + offset);
> +}
...
> +static const struct pinctrl_pin_desc
> + *rtd_pinctrl_get_pin_by_number(struct rtd_pinctrl *data, int number)
Weird indentation.
> +{
> + int i;
> +
> + for (i = 0; i < data->info->num_pins; i++) {
> + if (data->info->pins[i].number == number)
> + return &data->info->pins[i];
> + }
> +
> + return NULL;
> +}
...
> +static const struct rtd_pin_config_desc
> + *rtd_pinctrl_find_config(struct rtd_pinctrl *data, unsigned int pin)
> +{
> + if (!data->info->configs[pin].name)
> + return &data->info->configs[pin];
> +
> + return NULL;
if (...)
return NULL;
> +}
...
> + switch ((u32)param) {
Why casting?!
> + case PIN_CONFIG_INPUT_SCHMITT:
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + if (config_desc->smt_offset == NA) {
> + dev_err(data->dev, "Not support input schmitt for pin: %s\n", name);
In all cases like this, why do you need the message to be printed?
> + return -ENOTSUPP;
> + }
> + smt_off = config_desc->base_bit + config_desc->smt_offset;
> + set_val = arg;
> +
> + mask = BIT(smt_off);
> + val = set_val ? BIT(smt_off) : 0;
> + rtd_pinctrl_update_bits(data, config_desc->reg_offset, mask, val);
> + break;
...
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + curr_off = config_desc->base_bit + config_desc->curr_offset;
> + strength = arg;
> + val = 0;
How is this assignment being used?
> + switch (config_desc->curr_type) {
> + case PADDRI_4_8:
> + if (strength == 4)
BIT() ?
> + val = 0;
> + else if (strength == 8)
Ditto.
> + val = BIT(curr_off);
> + else
> + return -EINVAL;
> + break;
> + case PADDRI_2_4:
> + if (strength == 2)
Ditto.
> + val = 0;
> + else if (strength == 4)
Ditto.
> + val = BIT(curr_off);
> + else
> + return -EINVAL;
> + break;
> + case NA:
> + dev_err(data->dev, "Not support drive strength for pin: %s\n", name);
> + return -ENOTSUPP;
> + default:
> + return -EINVAL;
> + }
> + mask = BIT(curr_off);
> + rtd_pinctrl_update_bits(data, config_desc->reg_offset, mask, val);
> + break;
> +
> + case PIN_CONFIG_POWER_SOURCE:
> + if (config_desc->power_offset == NA) {
> + dev_err(data->dev, "Not support power source for pin: %s\n", name);
> + return -ENOTSUPP;
> + }
> + reg_off = config_desc->reg_offset;
> + pow_off = config_desc->base_bit + config_desc->power_offset;
> + if (pow_off >= 32) {
> + reg_off += 0x4;
> + pow_off -= 32;
> + }
Less error prone is to use division and multiplication. Something like
pow_bit = config_desc->base_bit + config_desc->power_offset;
reg_off = config_desc->reg_offset + 4 * (pow_bit / 32);
pow_off = pow_bit % 32;
> + set_val = arg;
> + mask = BIT(pow_off);
> + val = set_val ? mask : 0;
> + rtd_pinctrl_update_bits(data, reg_off, mask, val);
> + break;
> +
> + case RTD_P_DRIVE:
> + sconfig_desc = rtd_pinctrl_find_sconfig(data, pinnr);
> + if (!sconfig_desc) {
> + dev_err(data->dev, "Not support P driving for pin: %s\n", name);
> + return -ENOTSUPP;
> + }
> + set_val = arg;
> + reg_off = sconfig_desc->reg_offset;
> + p_off = sconfig_desc->pdrive_offset;
> + if (p_off >= 32) {
> + reg_off += 0x4;
> + p_off -= 32;
> + }
Ditto.
> + mask = GENMASK(p_off + sconfig_desc->pdrive_maskbits - 1, p_off);
No, this is suboptimal, better
mask = GENMASK(sconfig_desc->pdrive_maskbits - 1, 0) << p_off;
> + val = set_val << p_off;
> + rtd_pinctrl_update_bits(data, reg_off, mask, val);
> + break;
> +
> + case RTD_N_DRIVE:
> + sconfig_desc = rtd_pinctrl_find_sconfig(data, pinnr);
> + if (!sconfig_desc) {
> + dev_err(data->dev, "Not support N driving for pin: %s\n", name);
> + return -ENOTSUPP;
> + }
> + set_val = arg;
> + reg_off = sconfig_desc->reg_offset;
> + n_off = sconfig_desc->ndrive_offset;
> + if (n_off >= 32) {
> + reg_off += 0x4;
> + n_off -= 32;
> + }
As per above.
> + mask = GENMASK(n_off + sconfig_desc->ndrive_maskbits - 1, n_off);
As per above.
> + val = set_val << n_off;
> + rtd_pinctrl_update_bits(data, reg_off, mask, val);
> + break;
> + case RTD_D_CYCLE:
> + sconfig_desc = rtd_pinctrl_find_sconfig(data, pinnr);
> + if (!sconfig_desc || sconfig_desc->dcycle_offset == NA) {
> + dev_err(data->dev, "Not support duty cycle for pin: %s\n", name);
> + return -ENOTSUPP;
> + }
> + set_val = arg;
> + mask = GENMASK(sconfig_desc->dcycle_offset +
> + sconfig_desc->dcycle_maskbits - 1, sconfig_desc->dcycle_offset);
Broken indentation and see above.
> + val = set_val << sconfig_desc->dcycle_offset;
> + rtd_pinctrl_update_bits(data, sconfig_desc->reg_offset, mask, val);
> + break;
> +
> + default:
> + break;
> + }
> +
> + return 0;
> +}
...
> +static int rtd_pin_config_get(struct pinctrl_dev *pcdev, unsigned int pinnr,
> + unsigned long *config)
> +{
> + unsigned int param = pinconf_to_config_param(*config);
> + unsigned int arg = 0;
How is this assignment being used?
> +
> + switch (param) {
> + default:
> + return -ENOTSUPP;
> + }
> + *config = pinconf_to_config_packed(param, arg);
> + return 0;
This is a dead code, why?!
> +}
...
> +static int rtd_pin_config_set(struct pinctrl_dev *pcdev, unsigned int pinnr,
> + unsigned long *configs, unsigned int num_configs)
> +{
> + struct rtd_pinctrl *data = pinctrl_dev_get_drvdata(pcdev);
> + int i;
> + int ret = 0;
How is this assignment being used?
> + for (i = 0; i < num_configs; i++) {
> + ret = rtd_pconf_parse_conf(data, pinnr,
> + pinconf_to_config_param(configs[i]),
> + pinconf_to_config_argument(configs[i]));
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +int rtd_pinctrl_probe(struct platform_device *pdev, const struct rtd_pinctrl_desc *desc)
> +{
> + struct rtd_pinctrl *data;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->base = of_iomap(pdev->dev.of_node, 0);
Use proper platform driver API.
Also, why not devm_*()?
> + if (IS_ERR(data->base))
> + return PTR_ERR(data->base);
> +
> + data->dev = &pdev->dev;
> + data->info = desc;
> + data->desc.name = dev_name(&pdev->dev);
> + data->desc.pins = data->info->pins;
> + data->desc.npins = data->info->num_pins;
> + data->desc.pctlops = &rtd_pinctrl_ops;
> + data->desc.pmxops = &rtd_pinmux_ops;
> + data->desc.confops = &rtd_pinconf_ops;
> + data->desc.custom_params = rtd_custom_bindings;
> + data->desc.num_custom_params = ARRAY_SIZE(rtd_custom_bindings);
> + data->desc.owner = THIS_MODULE;
> +
> + data->pcdev = pinctrl_register(&data->desc, &pdev->dev, data);
Why not devm_*()?
> + if (!data->pcdev)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, data);
Is this used anyhow?
> + dev_dbg(&pdev->dev, "probed\n");
> +
> + return 0;
> +}
...
> +EXPORT_SYMBOL(rtd_pinctrl_probe);
Use namespace.
...
> +/*
> + * Copyright (c) 2023 Realtek Semiconductor Corp.
> + */
One line.
...
No ifdeffery guard?
...
> +#define NA 0xffffffff
What is this? Bit mask? U32_MAX? -1?
> +#define PADDRI_4_8 1
> +#define PADDRI_2_4 0
...
> +struct rtd_pin_group_desc {
> + const char *name;
> + const unsigned int *pins;
> + unsigned int num_pins;
> +};
> +
> +struct rtd_pin_func_desc {
> + const char *name;
> + const char * const *groups;
> + unsigned int num_groups;
> +};
NIH struct pingroup, struct pinfunction.
--
With Best Regards,
Andy Shevchenko
