On Fri, Dec 15, 2023 at 03:39:00PM +0100, Emil Renner Berthing wrote:
> Add pinctrl driver for the T-Head TH1520 RISC-V SoC.
...
+ array_size.h
+ bits.h
+ device.h
(and so on, please make sure you follow IWYU principle --
"include what you use")
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
Can you use device property API instead?
(I briefly checked, all of the used of_ ones have the respective generic
implementations either in fwnode_property_ or device_property_ namespace).
OTOH, it's used in xlate/map functions which have device_node as a parameter...
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/spinlock.h>
...
> +#include "core.h"
> +#include "pinmux.h"
> +#include "pinconf.h"
All of them are needed?
...
> +static unsigned int th1520_padcfg_shift(unsigned int pin)
> +{
> + return 16 * (pin & 0x1U);
BIT(0) ?
> +}
...
> +static unsigned int th1520_muxcfg_shift(unsigned int pin)
> +{
> + return 4 * (pin & 0x7U);
GENMASK() ?
> +}
...
> + return dev_err_probe(thp->pctl->dev, -EINVAL,
> + "no pins selected for %pOFn.%pOFn\n",
> + np, child);
> + dev_err(thp->pctl->dev, "error parsing pin config of group %pOFn.%pOFn\n",
> + np, child);
In the very same function you are using dev_err_probe(), please make sure
you use the same for all error messages as it will be a unified format
(in case of dev_err_probe() or if you explicitly do that with dev_err()
calls).
> + }
...
> +static const struct pinctrl_ops th1520_pinctrl_ops = {
> + .get_groups_count = th1520_pinctrl_get_groups_count,
> + .get_group_name = th1520_pinctrl_get_group_name,
> + .get_group_pins = th1520_pinctrl_get_group_pins,
> + .pin_dbg_show = th1520_pin_dbg_show,
Is ifdeffery needed for this one?
> + .dt_node_to_map = th1520_pinctrl_dt_node_to_map,
> + .dt_free_map = th1520_pinctrl_dt_free_map,
Is ifdeffery needed for these two?
> +};
...
> + mask = 0xfU << shift;
> + value = ((uintptr_t)func->data & 0xfU) << shift;
GENMASK() in both cases.
> + raw_spin_lock_irqsave(&thp->lock, flags);
> + value |= readl_relaxed(muxcfg) & ~mask;
Instead of above, use the traditional pattern
value = read()
value = (value & ~mask) | (newval & mask);
write()
where newval is defined with a proper type and you get rid of all those ugly
castings at once.
> + writel_relaxed(value, muxcfg);
> + raw_spin_unlock_irqrestore(&thp->lock, flags);
...
> +static u16 th1520_drive_strength_from_mA(u32 arg)
> +{
> + u16 v;
> +
> + for (v = 0; v < ARRAY_SIZE(th1520_drive_strength_in_mA) - 1; v++) {
You may drop -1 here AFAIU (see below).
> + if (arg <= th1520_drive_strength_in_mA[v])
> + break;
return directly.
> + }
> + return v;
return explicit value which will be robust against changes in the for-loop or
elsewhere in the code.
> +}
...
> +static int th1520_padcfg_rmw(struct th1520_pinctrl *thp, unsigned int pin,
> + u16 _mask, u16 _value)
Why not naming them without underscores?
> +{
> + void __iomem *padcfg = th1520_padcfg(thp, pin);
> + unsigned int shift = th1520_padcfg_shift(pin);
> + u32 mask = (u32)_mask << shift;
> + u32 value = (u32)_value << shift;
Oh, no castings, please.
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&thp->lock, flags);
Use cleanup.h.
> + value |= readl_relaxed(padcfg) & ~mask;
> + writel_relaxed(value, padcfg);
> + raw_spin_unlock_irqrestore(&thp->lock, flags);
> + return 0;
> +}
...
> +#define PIN_CONFIG_THEAD_STRONG_PULL_UP (PIN_CONFIG_END + 1)
Oh, custom flag! Linus, what is the expected approach for custom flags like this?
I believe this is quite error prone.
...
> + value = readl_relaxed(th1520_padcfg(thp, pin));
> + value = (value >> th1520_padcfg_shift(pin)) & 0x3ffU;
GENMASK() and in many other places like this.
...
> + enabled = value & TH1520_PADCFG_IE;
> + arg = enabled;
Assigning boolean to integer... Hmm...
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + enabled = value & TH1520_PADCFG_ST;
> + arg = enabled;
> + break;
> + case PIN_CONFIG_SLEW_RATE:
> + enabled = value & TH1520_PADCFG_SL;
> + arg = enabled;
> + break;
...
> +static int th1520_pinctrl_probe(struct platform_device *pdev)
> +{
struct device *dev = &pdev->dev;
may give you some benefits.
> + const struct th1520_padgroup *group = device_get_match_data(&pdev->dev);
> + struct th1520_pinctrl *thp;
> + int ret;
> +
> + thp = devm_kzalloc(&pdev->dev, sizeof(*thp), GFP_KERNEL);
> + if (!thp)
> + return -ENOMEM;
> +
> + thp->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(thp->base))
> + return PTR_ERR(thp->base);
> +
> + thp->desc.name = group->name;
> + thp->desc.pins = group->pins;
> + thp->desc.npins = group->npins;
> + thp->desc.pctlops = &th1520_pinctrl_ops;
> + thp->desc.pmxops = &th1520_pinmux_ops;
> + thp->desc.confops = &th1520_pinconf_ops;
> + thp->desc.owner = THIS_MODULE;
> + thp->desc.num_custom_params = ARRAY_SIZE(th1520_pinconf_custom_params);
> + thp->desc.custom_params = th1520_pinconf_custom_params;
> + thp->desc.custom_conf_items = th1520_pinconf_custom_conf_items;
> + mutex_init(&thp->mutex);
> + raw_spin_lock_init(&thp->lock);
> +
> + ret = devm_pinctrl_register_and_init(&pdev->dev, &thp->desc, thp, &thp->pctl);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "could not register pinctrl driver\n");
> +
> + return pinctrl_enable(thp->pctl);
> +}
--
With Best Regards,
Andy Shevchenko
