On Tue, Nov 05, 2019 at 02:49:42PM +0800, Rahul Tanwar wrote:
> Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which
> controls pin multiplexing & configuration including GPIO functions selection &
> GPIO attributes configuration.
>
> This IP is not based on & does not have anything in common with Chassis
> specification. The pinctrl drivers under pinctrl/intel/* are all based upon
> Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use
> pinctrl framework under pinctrl/intel/* and it requires a separate new driver.
>
> Add a new GPIO & pin control framework based driver for this IP.
> +static void eqbr_set_val(void __iomem *addr, u32 offset,
> + u32 mask, u32 set, raw_spinlock_t *lock)
This lock parameter is quite unusual. Can't you supply a pointer to a data
structure which has lock along with MMIO address?
> +{
> + u32 val;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(lock, flags);
> + mask = mask << offset;
Same Q. Why do you need these...
> + val = readl(addr);
> + val = (val & ~mask) | ((set << offset) & mask);
...offset shifts? It's unusual.
> + writel(val, addr);
> + raw_spin_unlock_irqrestore(lock, flags);
> +}
> +static int gpiochip_setup(struct device *dev, struct eqbr_gpio_desc *desc)
> +{
> + struct gpio_irq_chip *girq;
> + struct gpio_chip *gc;
> +#if defined(CONFIG_OF_GPIO)
> + gc->of_node = desc->node;
> +#endif
Isn't it what GPIO library does for everybody?
> +
> + if (!of_property_read_bool(desc->node, "interrupt-controller")) {
> + dev_info(dev, "gc %s: doesn't act as interrupt controller!\n",
> + desc->name);
Is it fatal or non-fatal?
> + return 0;
Ditto.
> + }
> +}
> +static int gpiolib_reg(struct eqbr_pinctrl_drv_data *drvdata)
> +{
> + struct device_node *np;
> + struct eqbr_gpio_desc *desc;
desc is very confusing here, since GPIO library uses this term for GPIO
descriptors.
> + struct device *dev;
> + int i, ret;
> + struct resource res;
> +
> + ret = bgpio_init(&desc->chip, dev, desc->bank->nr_pins/8,
'nr_pins / 8,'
> + desc->membase + GPIO_IN,
> + desc->membase + GPIO_OUTSET,
> + desc->membase + GPIO_OUTCLR,
> + desc->membase + GPIO_DIR,
> + NULL,
> + 0);
> + if (ret) {
> + dev_err(dev, "unable to init generic GPIO\n");
> + return ret;
> + }
> + return 0;
> +}
> +static int eqbr_pinmux_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int selector, unsigned int group)
> +{
> + struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
> + struct function_desc *func;
> + struct group_desc *grp;
> + unsigned int *pinmux;
> + int i;
> + pinmux = grp->data;
> + for (i = 0; i < grp->num_pins; i++)
> + eqbr_set_pin_mux(pctl, pinmux[i], grp->pins[i]);
Shouldn't be this part serialized?
Same Q to all similar places. I guess I already mentioned this in previous
review.
> + return 0;
> +}
> +static int is_func_exist(struct eqbr_pmx_func *funcs, const char *name,
Looks like it better to be boolean.
> + unsigned int nr_funcs, unsigned int *idx)
> +{
> + int i;
> +
> + if (!funcs || !nr_funcs)
> + return 0;
> +
> + for (i = 0; i < nr_funcs; i++) {
> +
Redundant blank line.
> + if (funcs[i].name && (strcmp(funcs[i].name, name) == 0) ) {
> + *idx = i;
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +static int funcs_utils(struct device *dev, struct eqbr_pmx_func *funcs,
> + unsigned int *nr_funcs, funcs_util_ops op)
> +{
> + struct device_node *node = dev->of_node;
> + struct device_node *np;
> + struct property *prop;
> + unsigned int fid;
> + const char *fn_name;
> + int i, j;
> +
> + i = 0;
> + for_each_child_of_node(node, np) {
> + prop = of_find_property(np, "groups", NULL);
> + if (prop) {
Why not
if (!prop)
continue;
?
> + if (of_property_read_string(np, "function",
> + &fn_name)) {
It's perfectly one line. Perhaps you may need to configure your text editor.
> + }
> + }
> + }
> +
> + return 0;
> +}
> + for (i=0; i < nr_funcs; i++) {
The better style is 'i = 0' and so on.
Simple be consistent. Or do everywhere 'i=0; i<nr_func; i++', etc. But remember
that this is for sure will be declined by most of the maintainers.
> + }
> +static int eqbr_build_groups(struct eqbr_pinctrl_drv_data *drvdata)
> +{
> + struct device *dev = drvdata->dev;
> + struct device_node *node = dev->of_node;
> + struct device_node *np;
> + struct property *prop;
> + int j, err;
> + unsigned int *pinmux, pin_id, pinmux_id;
> + struct group_desc group;
> +
> + for_each_child_of_node(node, np) {
> + prop = of_find_property(np, "groups", NULL);
> + if (prop) {
if (!prop)
continue;
?
> + }
> + memset(&group, 0, sizeof(group));
> + pinmux = NULL;
> + }
> +
> + return 0;
> +}
> +static int pinbank_init(struct device_node *np,
> + struct eqbr_pinctrl_drv_data *drvdata,
> + struct eqbr_pin_bank *bank, unsigned int id)
> +{
> + struct device *dev = drvdata->dev;
> + struct of_phandle_args spec;
> +
> + bank->membase = drvdata->membase + id * PAD_REG_OFF;
> +
> + if (of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &spec)) {
> + dev_err(dev, "gpio-range not available!\n");
> + return -EFAULT;
Shadowing error code with actually unsuitable one.
> + }
> + return 0;
> +}
> + int i=0, nr_gpio=0;
Style.
Besides the fact that better to put assignments closer to their usage.
> +static int eqbr_pinctrl_probe(struct platform_device *pdev)
> +{
> + struct eqbr_pinctrl_drv_data *drvdata;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->dev = dev;
> + platform_set_drvdata(pdev, drvdata);
I think this makes sense to do as last call in the function.
> + drvdata->membase = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(drvdata->membase))
> + return PTR_ERR(drvdata->membase);
> +
> + ret = pinbank_probe(drvdata);
> + if (ret)
> + return ret;
> +
> + ret = pinctrl_reg(drvdata);
> + if (ret)
> + return ret;
> +
> + ret = gpiolib_reg(drvdata);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
