Messages in this thread | | | From | Michael Buesch <> | Subject | Re: [PATCH v3] Add bt8xxgpio driver | Date | Thu, 10 Jul 2008 20:44:21 +0200 |
| |
On Thursday 10 July 2008 20:15:38 Jiri Slaby wrote: > > +static int bt8xxgpio_gpio_direction_input(struct gpio_chip *gpio, unsigned nr) > > +{ > > + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); > > + unsigned long flags; > > + u32 outen, data; > > + > > + spin_lock_irqsave(&bg->lock, flags); > > Why all those irq variants? I can't see interrupts anywhere. May gpio call this > from irq?
Yes. Call context is undefined.
> > + > > + data = bgread(BT848_GPIO_DATA); > > + data &= ~(1 << nr); > > + bgwrite(data, BT848_GPIO_DATA); > > + > > + outen = bgread(BT848_GPIO_OUT_EN); > > + outen &= ~(1 << nr); > > + bgwrite(outen, BT848_GPIO_OUT_EN); > > some flushing of posted values here?
> > +static void bt8xxgpio_gpio_set(struct gpio_chip *gpio, > > + unsigned nr, int val) > > +{ > > + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); > > + unsigned long flags; > > + u32 data; > > + > > + spin_lock_irqsave(&bg->lock, flags); > > + > > + data = bgread(BT848_GPIO_DATA); > > + if (val) > > + data |= (1 << nr); > > + else > > + data &= ~(1 << nr); > > + bgwrite(data, BT848_GPIO_DATA); > > and here and further in the code
Well, I'm not sure. This is pretty damn hotpath code. It can be used for bitbanging busdata, for example. I do actually consider optimizing that bgread() in the function above away, by caching it in memory. (First need to do measurements on whether it improves stuff).
So well. What about flushing. In theory it might be needed on some hardware. But unless somebody can show me some hardware, where this actually breaks without flushing, I won't consider adding flushes for performance reasons.
However, an mmiowb() might be added here right before the spinlock to make the locking safe.
> > + spin_unlock_irqrestore(&bg->lock, flags); > > +} > [...] > > +static int bt8xxgpio_probe(struct pci_dev *dev, > > + const struct pci_device_id *pci_id) > > +{ > > + struct bt8xxgpio *bg; > > + int err; > > + > > + bg = kzalloc(sizeof(*bg), GFP_KERNEL); > > + if (!bg) > > + return -ENOMEM; > > + > > + bg->pdev = dev; > > + spin_lock_init(&bg->lock); > > + > > + err = pci_enable_device(dev); > > + if (err) { > > + printk(KERN_ERR "bt8xxgpio: Can't enable device.\n"); > > dev_err() et al. all over here. > > > + goto err_freebg; > > + } > > + if (!request_mem_region(pci_resource_start(dev, 0), > > + pci_resource_len(dev, 0), > > + "bt8xxgpio")) { > > pci_request_region();?
Hell, I shouldn't have derived this from the bttv driver. Almost all style comments origin from that. ;)
> > + printk(KERN_WARNING > > + "bt8xxgpio: Can't request iomem (0x%llx).\n", > > + (unsigned long long)pci_resource_start(dev, 0)); > > You don't need to print the value out, we can see it in lspci.
Also from bttv...
-- Greetings Michael.
| |