On Sat, Dec 25, 2021 at 3:44 AM Wells Lu <wellslutw@xxxxxxxxx> wrote:
>
> Add driver for Sunplus SP7021 SoC.
Thanks for an update, my comments below.
...
> +config PINCTRL_SPPCTL
> + bool "Sunplus SP7021 PinMux and GPIO driver"
Why bool and not tristate?
> + depends on SOC_SP7021
> + depends on OF && HAS_IOMEM
...
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>
Can you move this group...
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
...to be somewhere here?
> +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
...
> +/* inline functions */
Useless.
...
> + mask = GENMASK(bit_sz - 1, 0) << (bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);
> + reg = mask | (val << bit_off);
Now you may do one step forward:
mask = GENMASK(bit_sz - 1, 0) << SPPCTL_GROUP_PINMUX_MASK_SHIFT;
reg = (val | mask) << bit_off;
...
> +static void sppctl_first_master_set(struct gpio_chip *chip, unsigned int offset,
> + enum mux_first_reg first, enum mux_master_reg master)
> +{
> + struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> + u32 reg_off, bit_off, reg;
> + int val;
> +
> + /* FIRST register */
> + if (first != mux_f_keep) {
> + /*
> + * Refer to descriptions of function sppctl_first_get()
> + * for usage of FIRST registers.
> + */
> + reg_off = (offset / 32) * 4;
> + bit_off = offset % 32;
> +
> + reg = sppctl_first_readl(spp_gchip, reg_off);
> + val = (reg & BIT(bit_off)) ? 1 : 0;
> + if (first != val) {
first is enum, val is int, are you sure it's good to compare like this?
> + if (first == mux_f_gpio)
> + reg |= BIT(bit_off);
> + else
> + reg &= ~BIT(bit_off);
Since you operate against enums it's better to use switch-case.
> + sppctl_first_writel(spp_gchip, reg, reg_off);
> + }
> + }
> +
> + /* MASTER register */
> + if (master != mux_m_keep) {
> + /*
> + * Refer to descriptions of function sppctl_master_get()
> + * for usage of MASTER registers.
> + */
> + reg_off = (offset / 16) * 4;
> + bit_off = offset % 16;
> +
> + reg = BIT(bit_off) << SPPCTL_MASTER_MASK_SHIFT;
> + if (master == mux_m_gpio)
> + reg |= BIT(bit_off);
> + sppctl_gpio_master_writel(spp_gchip, reg, reg_off);
> + }
> +}
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
Perhaps a macro with definitive name?
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
You can use it even here:
if (val)
reg = MY_MACRO(bit_off)
else
reg = BIT(...) // perhaps another macro
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
Ditto.
And looking into repetition, you may even have a helper which does
this conditional
static inline u32 sppctl_...()
{
...
return reg;
}
...
> + int ret = 0;
Redudant variable, return directly.
> + switch (param) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + /*
> + * Upper 16-bit word is mask. Lower 16-bit word is value.
> + * Refer to descriptions of function sppctl_master_get().
> + */
> + reg_off = (offset / 16) * 4;
> + bit_off = offset % 16;
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
As I commented above use helper function which takes offset as input
and returns you reg and reg_off.
> + sppctl_gpio_od_writel(spp_gchip, reg, reg_off);
> + break;
> +
> + case PIN_CONFIG_INPUT_ENABLE:
> + break;
> +
> + case PIN_CONFIG_OUTPUT:
> + ret = sppctl_gpio_direction_output(chip, offset, 0);
> + break;
> +
> + case PIN_CONFIG_PERSIST_STATE:
> + ret = -ENOTSUPP;
> + break;
> +
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
...
> + if (!of_find_property(pdev->dev.of_node, "gpio-controller", NULL))
> + return dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n");
Why do you need this check for?
...
> + gchip->can_sleep = 0;
Besides that it's already cleared, the type here is boolean.
...
> +/* pinconf operations */
Any value of this comment?
> +static int sppctl_pin_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config)
> +{
> + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int param = pinconf_to_config_param(*config);
> + unsigned int arg = 0;
Move assignment to where it actually makes sense.
> + switch (param) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + if (!sppctl_gpio_output_od_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + break;
> +
> + case PIN_CONFIG_OUTPUT:
> + if (!sppctl_first_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + if (!sppctl_master_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + if (sppctl_gpio_get_direction(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + arg = sppctl_gpio_get(&pctl->spp_gchip->chip, pin);
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> + *config = pinconf_to_config_packed(param, arg);
> +
> + return 0;
> +}
...
> + switch (f->type) {
> + case pinmux_type_fpmx: /* fully-pinmux */
Why do you need these comments?
Shouldn't you rather to kernel doc your enum entries?
> + *num_groups = sppctl_pmux_list_sz;
> + *groups = sppctl_pmux_list_s;
> + break;
> +
> + case pinmux_type_grp: /* group-pinmux */
> + if (!f->grps)
> + break;
> +
> + *num_groups = f->gnum;
> + for (i = 0; i < pctl->unq_grps_sz; i++)
> + if (pctl->g2fp_maps[i].f_idx == selector)
> + break;
> + *groups = &pctl->unq_grps[i];
> + break;
> + }
> +/** sppctl_fully_pinmux_conv - Convert GPIO# to fully-pinmux control-field setting
> + *
> + * Each fully-pinmux function can be mapped to any of GPIO 8 ~ 71 by
> + * settings its control-field. Refer to following table:
> + *
> + * control-field | GPIO
> + * --------------+--------
> + * 0 | No map
> + * 1 | 8
> + * 2 | 9
> + * 3 | 10
> + * : | :
> + * 65 | 71
> + */
> +static inline int sppctl_fully_pinmux_conv(unsigned int offset)
> +{
> + return (offset < 8) ? 0 : offset - 7;
> +}
...
> +static const struct pinmux_ops sppctl_pinmux_ops = {
> + .get_functions_count = sppctl_get_functions_count,
> + .get_function_name = sppctl_get_function_name,
> + .get_function_groups = sppctl_get_function_groups,
> + .set_mux = sppctl_set_mux,
> + .gpio_request_enable = sppctl_gpio_request_enable,
> + .strict = true
+ Comma.
> +};
...
> +static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config,
> + struct pinctrl_map **map, unsigned int *num_maps)
> +{
> + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> + int nmG = of_property_count_strings(np_config, "groups");
> + const struct sppctl_func *f = NULL;
> + u8 pin_num, pin_type, pin_func;
> + struct device_node *parent;
> + unsigned long *configs;
> + struct property *prop;
> + const char *s_f, *s_g;
> +
> + const __be32 *list;
> + u32 dt_pin, dt_fun;
> + int i, size = 0;
> +
> + list = of_get_property(np_config, "sunplus,pins", &size);
> +
> + if (nmG <= 0)
> + nmG = 0;
> +
> + parent = of_get_parent(np_config);
> + *num_maps = size / sizeof(*list);
> +
> + /*
> + * Process property:
> + * sunplus,pins = < u32 u32 u32 ... >;
> + *
> + * Each 32-bit integer defines a individual pin in which:
> + *
> + * Bit 32~24: defines GPIO pin number. Its range is 0 ~ 98.
> + * Bit 23~16: defines types: (1) fully-pinmux pins
> + * (2) IO processor pins
> + * (3) digital GPIO pins
> + * Bit 15~8: defines pins of peripherals (which are defined in
> + * 'include/dt-binging/pinctrl/sppctl.h').
> + * Bit 7~0: defines types or initial-state of digital GPIO pins.
> + */
> + for (i = 0; i < (*num_maps); i++) {
> + dt_pin = be32_to_cpu(list[i]);
> + pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> +
> + /* Check if out of range? */
> + if (pin_num >= sppctl_pins_all_sz) {
> + dev_err(pctldev->dev, "Invalid pin property at index %d (0x%08x)\n",
> + i, dt_pin);
> + return -EINVAL;
> + }
> + }
> +
> + *map = kcalloc(*num_maps + nmG, sizeof(**map), GFP_KERNEL);
> + for (i = 0; i < (*num_maps); i++) {
> + dt_pin = be32_to_cpu(list[i]);
> + pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> + pin_type = FIELD_GET(GENMASK(23, 16), dt_pin);
> + pin_func = FIELD_GET(GENMASK(15, 8), dt_pin);
> + (*map)[i].name = parent->name;
> +
> + if (pin_type == SPPCTL_PCTL_G_GPIO) {
> + /* A digital GPIO pin */
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + (*map)[i].data.configs.num_configs = 1;
> + (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
> + configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> + *configs = FIELD_GET(GENMASK(7, 0), dt_pin);
> + (*map)[i].data.configs.configs = configs;
> +
> + dev_dbg(pctldev->dev, "%s: GPIO (%s)\n",
> + (*map)[i].data.configs.group_or_pin,
> + (*configs & (SPPCTL_PCTL_L_OUT | SPPCTL_PCTL_L_OU1)) ?
> + "OUT" : "IN");
> + } else if (pin_type == SPPCTL_PCTL_G_IOPP) {
> + /* A IO Processor (IOP) pin */
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + (*map)[i].data.configs.num_configs = 1;
> + (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
> + configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> + *configs = SPPCTL_IOP_CONFIGS;
> + (*map)[i].data.configs.configs = configs;
> +
> + dev_dbg(pctldev->dev, "%s: IOP\n",
> + (*map)[i].data.configs.group_or_pin);
> + } else {
> + /* A fully-pinmux pin */
> + (*map)[i].type = PIN_MAP_TYPE_MUX_GROUP;
> + (*map)[i].data.mux.function = sppctl_list_funcs[pin_func].name;
> + (*map)[i].data.mux.group = pin_get_name(pctldev, pin_num);
> +
> + dev_dbg(pctldev->dev, "%s: %s\n", (*map)[i].data.mux.group,
> + (*map)[i].data.mux.function);
> + }
> + }
> +
> + /*
> + * Process properties:
> + * function = "xxx";
> + * groups = "yyy";
> + */
> + if (nmG > 0 && of_property_read_string(np_config, "function", &s_f) == 0) {
> + of_property_for_each_string(np_config, "groups", prop, s_g) {
> + (*map)[*num_maps].type = PIN_MAP_TYPE_MUX_GROUP;
> + (*map)[*num_maps].data.mux.function = s_f;
> + (*map)[*num_maps].data.mux.group = s_g;
> + (*num_maps)++;
> +
> + dev_dbg(pctldev->dev, "%s: %s\n", s_f, s_g);
> + }
> + }
> +
> + /*
> + * Process property:
> + * sunplus,zero_func = < u32 u32 u32 ...>
> + */
> + list = of_get_property(np_config, "sunplus,zero_func", &size);
> + if (list) {
> + for (i = 0; i < (size / sizeof(*list)); i++) {
> + dt_fun = be32_to_cpu(list[i]);
> + if (dt_fun >= sppctl_list_funcs_sz) {
> + dev_err(pctldev->dev, "Zero-func %d out of range!\n",
> + dt_fun);
> + continue;
> + }
> +
> + f = &sppctl_list_funcs[dt_fun];
> + switch (f->type) {
> + case pinmux_type_fpmx:
> + sppctl_func_set(pctl, dt_fun, 0);
> + dev_dbg(pctldev->dev, "%s: No map\n", f->name);
> + break;
> +
> + case pinmux_type_grp:
> + sppctl_gmx_set(pctl, f->roff, f->boff, f->blen, 0);
> + dev_dbg(pctldev->dev, "%s: No map\n", f->name);
> + break;
> +
> + default:
> + dev_err(pctldev->dev, "Wrong zero-group: %d (%s)\n",
> + dt_fun, f->name);
> + break;
> + }
> + }
> + }
> +
> + of_node_put(parent);
> + dev_dbg(pctldev->dev, "%d pins mapped\n", *num_maps);
> + return 0;
> +}
...
> + sppctl->g2fp_maps = devm_kcalloc(&pdev->dev, sppctl->unq_grps_sz + 1,
> + sizeof(*sppctl->g2fp_maps), GFP_KERNEL);
> + if (!sppctl->g2fp_maps)
> + return -ENOMEM;
> + /*
> + * Check only product of n and size of the second devm_kcalloc()
> + * because its size is the largest of the two.
> + */
> + if (unlikely(check_mul_overflow(sppctl->unq_grps_sz + 1,
> + sizeof(*sppctl->g2fp_maps), &prod)))
> + return -EINVAL;
What the point to check it after? What the point to use it with
kcalloc()? Please, do your homework, i.e. read the code which
implements that.
...
> + struct device_node *np = of_node_get(pdev->dev.of_node);
What's the role of of_node_get()?
...
> + /* Initialize pctl_desc */
Useless. Drop all useless comments like this from the code.
...
> + dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech.");
Is it useful?
...
> +#ifndef __SPPCTL_H__
> +#define __SPPCTL_H__
> +
> +#include <linux/bits.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/spinlock.h>
types.h is missed.
...
> +/** enum mux_first_reg - define modes of FIRST register accesses
Fix the multi-line comment style. You mentioned you fixed, but seems
not (not in all places).
> + * - mux_f_mux: Select the pin to a fully-pinmux pin
> + * - mux_f_gpio: Select the pin to a GPIO or IOP pin
> + * - mux_f_keep: Don't change (keep intact)
> + */
> +enum mux_first_reg {
> + mux_f_mux = 0, /* select fully-pinmux */
> + mux_f_gpio = 1, /* select GPIO or IOP pinmux */
> + mux_f_keep = 2, /* keep no change */
> +};
> +struct sppctl_gpio_chip {
> + void __iomem *gpioxt_base; /* MASTER, OE, OUT, IN, I_INV, O_INV, OD */
> + void __iomem *first_base; /* GPIO_FIRST */
> +
> + struct gpio_chip chip;
> + spinlock_t lock; /* lock for accessing OE register */
> +};
Why is this in the header?
...
> +/* SP7021 Pin Controller Driver.
> + * Copyright (C) Sunplus Tech / Tibbo Tech.
> + */
Multi-line comments.
I stopped here, please read my comments for previous versions and here
and try your best.
--
With Best Regards,
Andy Shevchenko
