On Wed, Oct 30, 2019 at 12:23:59PM +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.
Thanks for an update, my comments below.
> +#define PIN_NAME_FMT "io-%d"
> +#define PIN_NAME_LEN 10
> +static inline void eqbr_set_val(void __iomem *addr, u32 offset,
> + u32 mask, u32 set, raw_spinlock_t *lock)
Why is it marked with inline?
> +{
> + u32 val;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(lock, flags);
> + val = readl(addr);
> + val = (val & ~(mask << offset)) | ((set & mask) << offset);
This is unusual, why offset can't be applied once to the mask?
> + writel(val, addr);
> + raw_spin_unlock_irqrestore(lock, flags);
Hmm... Don't you have more complicated workflow that requires few
reads/writes/updates to be called atomically?
> +}
> +static void eqbr_gpio_disable_irq(struct irq_data *d)
> +{
> + unsigned int offset = irqd_to_hwirq(d);
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct eqbr_gpio_desc *desc = gpiochip_get_data(gc);
> + writel(BIT(offset), desc->membase + GPIO_IRNENCLR);
Is it okay to be without spin lock?
Same Q to the rest similar places.
> +}
> +static inline void eqbr_cfg_bit(void __iomem *addr,
> + unsigned int offset, unsigned int set)
> +{
> + if (!set)
Why not to use positive condition?
> + writel(readl(addr) & ~BIT(offset), addr);
> + else
> + writel(readl(addr) | BIT(offset), addr);
> +}
> + struct gpio_irq_type it;
Not sure if this is used properly. Linus may clarify this.
> +static void eqbr_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct eqbr_gpio_desc *gpio_desc = gpiochip_get_data(gc);
> + struct irq_chip *ic = irq_desc_get_chip(desc);
> + u32 pins, offset;
> +
> + chained_irq_enter(ic, desc);
> + pins = readl(gpio_desc->membase + GPIO_IRNCR);
> +
> + for_each_set_bit(offset, (unsigned long *)&pins, gc->ngpio)
This casting is no go.
> + generic_handle_irq(irq_find_mapping(gc->irq.domain, offset));
> +
> + chained_irq_exit(ic, desc);
> +}
> +static int gpiochip_setup(struct device *dev, struct eqbr_gpio_desc *desc)
> +{
> + struct gpio_irq_chip *girq;
> + struct gpio_chip *gc;
> +
> + gc = &desc->chip;
> + gc->owner = THIS_MODULE;
Do we still need this in the drivers?
> + gc->label = desc->name;
> + gc->of_node = desc->node;
> +
> + if (!of_property_read_bool(desc->node, "interrupt-controller")) {
Why is it fatal?
> + dev_info(dev, "gc %s: doesn't act as interrupt controller!\n",
> + desc->name);
> + return 0;
> + }
> + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
> + GFP_KERNEL);
I believe it's fine to have it on one line.
> + if (!girq->parents)
> + return -ENOMEM;
> +
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_level_irq;
Not bad IRQ handler?
> + girq->parents[0] = desc->virq;
> +
> + return 0;
> +}
> +static int gpiolib_reg(struct eqbr_pinctrl_drv_data *drvdata)
> +{
> + struct device_node *np;
> + struct eqbr_gpio_desc *desc;
> + struct device *dev;
> + int i, ret;
> + struct resource res;
> +
> + dev = drvdata->dev;
> + for (i = 0; i < drvdata->nr_gpio_descs; i++) {
> + desc = drvdata->gpio_desc + i;
> + np = desc->node;
> +
> + desc->name = devm_kasprintf(dev, GFP_KERNEL, "gpiochip%d", i);
> + if (!desc->name)
> + return -ENOMEM;
> +
> + if (of_address_to_resource(np, 0, &res)) {
> + dev_err(dev, "Failed to get GPIO register address\n");
> + return -ENXIO;
> + }
> +
> + desc->membase = devm_ioremap_resource(dev, &res);
> + if (IS_ERR(desc->membase)) {
> + dev_err(dev, "ioremap fail\n");
Redundant.
> + return PTR_ERR(desc->membase);
> + }
Is it per descriptor?!
> +
> + desc->virq = irq_of_parse_and_map(np, 0);
> + if (!desc->virq) {
> + dev_err(dev, "%s: failed to parse and map irq\n",
> + desc->name);
> + return -ENXIO;
> + }
> + raw_spin_lock_init(&desc->lock);
> + }
> +
> + return 0;
> +}
> +static const struct pinmux_ops eqbr_pinmux_ops = {
> + .get_functions_count = pinmux_generic_get_function_count,
> + .get_function_name = pinmux_generic_get_function_name,
> + .get_function_groups = pinmux_generic_get_function_groups,
> + .set_mux = eqbr_pinmux_set_mux,
> + .gpio_request_enable = eqbr_pinmux_gpio_request,
> + .strict = true,
> +};
TABs/spaces mix.
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
