Hi Andy,
On 30/10/2019 10:39 PM, Andy Shevchenko wrote:
> 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.
>>
>> +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?
Hard to justify. I can remove inline if you recommend so..
>> +{
>> + 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?
Do you mean like below ? (set still needs to be left shifted by offset)
mask = mask << offset;
val = (val & ~mask) | ((set << offset) & 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?
I don't really see any complicated register programming in this driver.
Just forpin configuration, mux setting & GPIO IRQ configuration..
>> +}
>> +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.
Yes, you are right. GPIO IRQ ops also needs locking. I was incorrectly
assuming that gpiolib will provide locking when it calls these ops. I
will fix it in v3. Thanks.
>> +}
>> +static inline void eqbr_cfg_bit(void __iomem *addr,
>> + unsigned int offset, unsigned int set)
>> +{
>> + if (!set)
> Why not to use positive condition?
Well noted.
>> + 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.
Well noted. Copy paste mistake. Will fix it. Thanks.
>> + 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?
Agree it is redundant. Will remove it. Thanks.
>> + gc->label = desc->name;
>> + gc->of_node = desc->node;
>> +
>> + if (!of_property_read_bool(desc->node, "interrupt-controller")) {
> Why is it fatal?
It is not fatal, that's why it returns 0 instead of error i.e. if this
gpio chip is notintended to be used as interrupt controller then do
not setup gpio_irq_chipand proceed to register gpio chip without
interrupt support.
>> + 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.
Well noted.
>> + if (!girq->parents)
>> + return -ENOMEM;
>> +
>> + girq->default_type = IRQ_TYPE_NONE;
>> + girq->handler = handle_level_irq;
> Not bad IRQ handler?
Yes, it should be handle_bad_irq(). Will fix, thanks.
>> + 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.
Well noted.
>> + return PTR_ERR(desc->membase);
>> + }
> Is it per descriptor?!
Yes, it is per descriptor i.e. per gpio_chip. Each gpio_chip has different
membase.
>> +
>> + 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.
Well noted. Thanks.
Regards,
Rahul
