| Message ID | 20231215143906.3651122-1-emil.renner.berthing@canonical.com |
|---|---|
| Headers | show |
| Series | Add T-Head TH15020 SoC pin control | expand |
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); > +}