lkml.org 
[lkml]   [2021]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 2/2] pinctrl: Add driver for Sunplus SP7021
On Tue, Dec 14, 2021 at 5:08 PM Wells Lu <wellslutw@gmail.com> wrote:
>
> Add driver for Sunplus SP7021 SoC.

It needs much more work, my comments below.

...

> +/* SP7021 Pin Controller Driver.
> + * Copyright (C) Sunplus Tech/Tibbo Tech.
> + */

This is wrong style for multi-line comments. Fix it everywhere accordingly.

...

> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/bitfield.h>

Keep them in order. Besides that it seems missed a few headers, such as of.h.

> +
> +#include <dt-bindings/pinctrl/sppctl-sp7021.h>

+ blank line

> +#include "../pinctrl-utils.h"
> +#include "../core.h"

+ blank line

> +#include "sppctl.h"

...

> + mask = GENMASK(bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT + bit_sz - 1,
> + bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);

GENMASK() with non-const arguments may be not a good choice and see, even

mask = GENMASK(bit_sz - 1, 0) << (bit_off +
SPPCTL_GROUP_PINMUX_MASK_SHIFT);

is way much better.

...

> + val = (reg & BIT(bit_off)) ? 1 : 0;

!!(...) may also work, but it's rather style preference.

...

> + reg = readl(spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_MASTER + reg_off);

I noticed a potentially big issue with this driver. Are you sure it's
brave enough to do I/O without any synchronisation? Did I miss a lock?

...

> + reg = readl(spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OD + reg_off);

You may create an I/O wrappers to achieve a much better to read code
(no repetition of this arithmetics, etc).

...

> + for (i = 0; i < chip->ngpio; i++) {
> + label = gpiochip_is_requested(chip, i);
> + if (!label)
> + label = "";

Perhaps to show only requested ones? In such case you may use
for_each_requested_gpio() macro.

> + seq_printf(s, " gpio-%03d (%-16.16s | %-16.16s)", i + chip->base,
> + chip->names[i], label);
> + seq_printf(s, " %c", sppctl_gpio_get_direction(chip, i) == 0 ? 'O' : 'I');
> + seq_printf(s, ":%d", sppctl_gpio_get(chip, i));
> + seq_printf(s, " %s", (sppctl_first_get(chip, i) ? "gpi" : "mux"));
> + seq_printf(s, " %s", (sppctl_master_get(chip, i) ? "gpi" : "iop"));
> + seq_printf(s, " %s", (sppctl_gpio_inv_get(chip, i) ? "inv" : " "));
> + seq_printf(s, " %s", (sppctl_gpio_output_od_get(chip, i) ? "oDr" : ""));

Too many parentheses in a few of above lines.

> + seq_puts(s, "\n");
> + }

...

> + dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n");
> + return -EINVAL;

Unite them in one return statement.
Ditto for zillion similar cases in this driver.

...

> + gchip->parent = &pdev->dev;

> + gchip->of_node = pdev->dev.of_node;

Drop this dup. GPIO library already does it for you.

...

> + int i = 0;

What for this assingment?

> + dev_dbg(pctldev->dev, "%s(%d, %ld, %d)\n", __func__, pin, *configs, num_configs);

Noise. Better to consider to add necessary tracepoints to pin control core.

> + /* Special handling for IOP */
> + if (configs[i] == 0xFF) {

Why out of a sudden capitilazed hex value?

> + sppctl_first_master_set(&pctl->spp_gchip->chip, pin, mux_f_gpio, mux_m_iop);
> + return 0;
> + }

...

> + dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);

Noise. And so on, so on...

...

> + dev_dbg(pctldev->dev, "%s(%d), f_idx: %d, g_idx: %d, freg: %d\n",
> + __func__, selector, g2fpm.f_idx, g2fpm.g_idx, f->freg);

No need to use __func__, and especially in case of _dbg / _debug. It
can be enabled at run-time with help of Dynamic Debug.

...

> + seq_printf(s, "%s", dev_name(pctldev->dev));

Isn't it printed by core?

...

> +static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config,
> + struct pinctrl_map **map, unsigned int *num_maps)
> +{

Looking into this rather quite big function why you can't use what pin
control core provides?

> +}

...

> + /* Fill up unique group names array. */
> + sppctl->unq_grps = devm_kzalloc(&pdev->dev, (sppctl->unq_grps_sz + 1) *
> + sizeof(char *), GFP_KERNEL);

You need to use devm_kcalloc() variant for arrays.

> + if (!sppctl->unq_grps)
> + return -ENOMEM;

> + sppctl->g2fp_maps = devm_kzalloc(&pdev->dev, (sppctl->unq_grps_sz + 1) *
> + sizeof(struct grp2fp_map), GFP_KERNEL);

Ditto, besides the fact of better use of sizeof() of the type of
variable, done by sizeof(*..._maps).

> + if (!sppctl->g2fp_maps)
> + return -ENOMEM;
> +
> + sppctl->groups_name = devm_kzalloc(&pdev->dev, sppctl_list_funcs_sz *
> + SPPCTL_MAX_GROUPS * sizeof(char *), GFP_KERNEL);

Ditto. And check some interesting macros in overflow.h.

> + if (!sppctl->groups_name)
> + return -ENOMEM;

...

> + /* gpio */

GPIO, but either way seems not so valueable comment.

...

> + err = devm_pinctrl_register_and_init(&pdev->dev, &sppctl->pctl_desc,
> + sppctl, &sppctl->pctl_dev);
> + if (err) {

> + dev_err_probe(&pdev->dev, err, "Failed to register pinctrl!\n");
> + of_node_put(np);

Swap them and use the form of
return dev_err_probe();

> + return err;
> + }

...

> + /* MOON2 registers */
> + rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon2");
> + sppctl->moon2_base = devm_ioremap_resource(&pdev->dev, rp);

We have an API that provides two in one.

> + if (IS_ERR(sppctl->moon2_base)) {
> + ret = PTR_ERR(sppctl->moon2_base);

> + goto ioremap_failed;

What is this for? Use return dev_err_probe() directly.

> + }

> + dev_dbg(&pdev->dev, "MOON2: %pr\n", rp);

This cryptic noise has to be removed.

Above comments are applicable to all similar cases.

...

> +ioremap_failed:
> + dev_err_probe(&pdev->dev, ret, "ioremap failed!\n");

This doesn't bring any value, besides the fact that API you have used
already prints a message.

...

> + pdev->dev.platform_data = sppctl;

Don't we have special setter for this field?

...

> + dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech. (c)");

No value.

...

> + { /* zero */ }

Comment with no value.

> +};

...

> + .owner = THIS_MODULE,

It's probably 5+ years that we don't need this (it's applied implicitly).

...

> +#ifndef __SPPCTL_H__
> +#define __SPPCTL_H__

This header misses the inclusions such as bits.h.
And I belive more than that.

...

> +/* FIRST register:
> + * 0: MUX
> + * 1: GPIO/IOP
> + * 2: No change
> + */

For all comments starting from here and for similar cases elsewhere:
- why it is not in kernel doc?
- what the value that add?
(Some of them so cryptic or so obvious)

...

> +static const struct sppctl_grp sp7021grps_spif[] = {
> + EGRP("SPI_FLASH1", 1, pins_spif1),
> + EGRP("SPI_FLASH2", 2, pins_spif2)

Here and everywhere else, leave a comma if it's not a terminator entry.

> +};

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2021-12-15 00:16    [W:0.193 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site