Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 08, 2014 at 03:33:28PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold wrote:
> > On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> >> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold wrote:
> >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >> >
> >> >> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> >> >> + u16 handle, u16 rx_slot)
> >> >> +{
> >> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> >> >> + struct dln2_rx_context *rxc;
> >> >> + struct device *dev = &dln2->interface->dev;
> >> >> +
> >> >> + spin_lock(&rxs->lock);
> >> >
> >> > You must use spin_lock_irqsave here as you call it from the completion
> >> > handler.
> >>
> >> Why? AFAICS the completion handler gets called from the HCD irq handler:
> >
> > The completion handler is currently called with local interrupts
> > disabled but that is about to change once all drivers have been updated:
> >
> > http://marc.info/?l=linux-usb&m=137353360511003&w=2
> >
> > In this case you could probably get away with not disabling interrupts
> > anyway, but using the irqsave versions would make it obvious.
> >
>
> I was not assuming that interrupts are disabled while running the
> completion handler. Since that spinlock is not touched by any other
> interrupt context code I don't think irqsave is necessary.
No, it isn't necessary so leave it as it is.
But you are exporting interfaces to other drivers and it may appear to
someone that those could possibly end up indirectly calling a function
taking that lock in IRQ context. We know that isn't the case now, but I
bet someone will post conversion patch for that spinlock at some point.
;)
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold wrote:
> On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
>> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold wrote:
>> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
>> >
>> >> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
>> >> + u16 handle, u16 rx_slot)
>> >> +{
>> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
>> >> + struct dln2_rx_context *rxc;
>> >> + struct device *dev = &dln2->interface->dev;
>> >> +
>> >> + spin_lock(&rxs->lock);
>> >
>> > You must use spin_lock_irqsave here as you call it from the completion
>> > handler.
>>
>> Why? AFAICS the completion handler gets called from the HCD irq handler:
>
> The completion handler is currently called with local interrupts
> disabled but that is about to change once all drivers have been updated:
>
> http://marc.info/?l=linux-usb&m=137353360511003&w=2
>
> In this case you could probably get away with not disabling interrupts
> anyway, but using the irqsave versions would make it obvious.
>
I was not assuming that interrupts are disabled while running the
completion handler. Since that spinlock is not touched by any other
interrupt context code I don't think irqsave is necessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold wrote:
> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >
> >> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> >> + u16 handle, u16 rx_slot)
> >> +{
> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> >> + struct dln2_rx_context *rxc;
> >> + struct device *dev = &dln2->interface->dev;
> >> +
> >> + spin_lock(&rxs->lock);
> >
> > You must use spin_lock_irqsave here as you call it from the completion
> > handler.
>
> Why? AFAICS the completion handler gets called from the HCD irq handler:
The completion handler is currently called with local interrupts
disabled but that is about to change once all drivers have been updated:
http://marc.info/?l=linux-usb&m=137353360511003&w=2
In this case you could probably get away with not disabling interrupts
anyway, but using the irqsave versions would make it obvious.
> >> +static void dln2_disconnect(struct usb_interface *interface)
> >> +{
> >> + struct dln2_dev *dln2 = usb_get_intfdata(interface);
> >> + int i, j;
> >> +
> >> + /* don't allow starting new transfers */
> >> + spin_lock(&dln2->disconnect_lock);
> >> + dln2->disconnect = true;
> >> + spin_unlock(&dln2->disconnect_lock);
> >> +
> >> + /* cancel in progress transfers */
> >> + for (i = 0; i < DLN2_HANDLES; i++) {
> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&rxs->lock, flags);
> >
> > Just stick to spin_lock in this function.
>
> AFAICS disconnect is called from a kernel thread. Are there guarantees
> that we can't get a call to the completion routine while we are
> running it?
Brain fart, nevermind.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold wrote:
> On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
>
>> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
>> + u16 handle, u16 rx_slot)
>> +{
>> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
>> + struct dln2_rx_context *rxc;
>> + struct device *dev = &dln2->interface->dev;
>> +
>> + spin_lock(&rxs->lock);
>
> You must use spin_lock_irqsave here as you call it from the completion
> handler.
Why? AFAICS the completion handler gets called from the HCD irq handler:
[2.503945] Call Trace:
[2.503945][] dump_stack+0x4e/0x7a
[2.503945] [] dln2_rx+0x1eb/0x2d0
[2.503945] [] __usb_hcd_giveback_urb+0x72/0x120
[2.503945] [] usb_hcd_giveback_urb+0x3f/0x120
[2.503945] [] uhci_giveback_urb+0xaa/0x290
[2.503945] [] ? dma_pool_free+0xa7/0xd0
[2.503945] [] uhci_scan_schedule+0x493/0xb20
[2.503945] [] uhci_irq+0x9e/0x180
[2.503945] [] usb_hcd_irq+0x26/0x40
[2.503945] [] handle_irq_event_percpu+0x3e/0x1e0
[2.503945] [] handle_irq_event+0x3d/0x60
[2.503945] [] handle_fasteoi_irq+0x99/0x160
[2.503945] [] handle_irq+0x102/0x190
[2.503945] [] ? atomic_notifier_call_chain+0x3a/0x50
[2.503945] [] do_IRQ+0x5b/0x100
[2.503945] [] common_interrupt+0x6f/0x6f
[2.503945][] ? default_idle+0x1d/0x100
>
>> +
>> + rxc = &rxs->slots[rx_slot];
>> +
>> + if (rxc->in_use && !rxc->urb) {
>> + rxc->urb = urb;
>> + complete(&rxc->done);
>> + /* URB will be resubmitted in free_rx_slot */
>> + } else {
>> + dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
>> + dln2_submit_urb(dln2, urb, GFP_ATOMIC);
>> + }
>> +
>> + spin_unlock(&rxs->lock);
>> +}
>
>> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
>> + const void *obuf, unsigned obuf_len,
>> + void *ibuf, unsigned *ibuf_len)
>> +{
>> + int ret = 0;
>> + u16 result;
>> + int rx_slot;
>> + struct dln2_response *rsp;
>> + struct dln2_rx_context *rxc;
>> + struct device *dev;
>> + const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
>> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
>> +
>> + spin_lock(&dln2->disconnect_lock);
>
> How did you test this version? You never initialise disconnect_lock so
> lockdep complains loudly when calling _dln2_transfer during probe.
>
Oops, missed that as lockdep was not enable in the lastest test setup.
>> + if (!dln2->disconnect)
>> + dln2->active_transfers++;
>> + else
>> + ret = -ENODEV;
>> + spin_unlock(&dln2->disconnect_lock);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + rx_slot = alloc_rx_slot(dln2, handle);
>> + if (rx_slot < 0) {
>> + ret = rx_slot;
>> + goto out_decr;
>> + }
>> +
>> + dev = &dln2->interface->dev;
>> +
>> + ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
>> + if (ret < 0) {
>> + dev_err(dev, "USB write failed: %d\n", ret);
>> + goto out_free_rx_slot;
>> + }
>> +
>> + rxc = &rxs->slots[rx_slot];
>> +
>> + ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
>> + if (ret <= 0) {
>> + if (!ret)
>> + ret = -ETIMEDOUT;
>> + goto out_free_rx_slot;
>> + }
>> +
>> + if (!rxc->urb) {
>> + ret = -ENODEV;
>> + goto out_free_rx_slot;
>> + }
>> +
>> + /* if we got here we know that the response header has been checked */
>> + rsp = rxc->urb->transfer_buffer;
>> +
>> + if (rsp->hdr.size < sizeof(*rsp)) {
>> + ret = -EPROTO;
>> + goto out_free_rx_slot;
>> + }
>> +
>> + result = le16_to_cpu(rsp->result);
>> + if (result) {
>> + dev_dbg(dev, "%d received response with error %d\n",
>> + handle, result);
>> + ret = -EREMOTEIO;
>> + goto out_free_rx_slot;
>> + }
>> +
>> + if (!ibuf) {
>> + ret = 0;
>> + goto out_free_rx_slot;
>> + }
>> +
>> + if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
>> + *ibuf_len = rsp->hdr.size - sizeof(*rsp);
>> +
>> + memcpy(ibuf, rsp + 1, *ibuf_len);
>> +
>> +out_free_rx_slot:
>> + free_rx_slot(dln2, handle, rx_slot);
>> +out_decr:
>> + spin_lock(&dln2->disconnect_lock);
>> + dln2->active_transfers--;
>> + spin_unlock(&dln2->disconnect_lock);
>> + if (dln2->disconnect)
>> + wake_up(&dln2->disconnect_wq);
>> +
>> + return ret;
>> +}
>
>> +static void dln2_disconnect(struct usb_interface *interface)
>> +{
>> + struct dln2_dev *dln2 = usb_get_intfdata(interface);
>> + int i, j;
>> +
>> + /* don't allow starting new transfers */
>> +
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Tue, Oct 07, 2014 at 09:01:27PM +0300, Octavian Purdila wrote: > On Tue, Oct 7, 2014 at 8:10 PM, Johan Hovold wrote: > > On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote: > >> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold wrote: > >> > > >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote: > >> > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > >> > > Master Adapter DLN-2. Details about the device can be found here: > >> > > > >> > >> > >> > >> > > + > >> > > + ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL); > >> > > + if (ret < 0) > >> > > + return ret; > >> > > >> > Is it really worth having this helper only to save a couple of lines on > >> > a dev_err? If you do all resubmissions on completion inline in the > >> > handler, you only have three places where usb_submit_urb is called. > >> > >> I moved the completion in the handler as you suggested. I have kept > >> the helper, would you prefer to remove it? > > > > Moved the "completion"? I was suggesting that the URB resubmission > > should be done inline the URB completion handler. > > > > [ "Completion" may be a little ambiguous. The URB callback is called an > > URB completion handler. Not be confused with the completion structures > > you use to wait for responses. ] > > > > Sorry, I meant to say resubmission instead of completion. > > > It's fine to keep the helper as long as it's clear that the urb has been > > "cached" and should not be resubmitted (inline) in the completion > > handler in that case. > > Not sure I follow you here. I kept the helper and I call it from the > completion handler, from free_rx_slot and from dln2_setup_rx_ubs. Ah sorry, I was referring to your other helper dln2_rx_transfer(). I still think you should do away with the dln2_submit_urb() helper as it needlessly hides what's on going without any real gain. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> + u16 handle, u16 rx_slot)
> +{
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> + struct dln2_rx_context *rxc;
> + struct device *dev = &dln2->interface->dev;
> +
> + spin_lock(&rxs->lock);
You must use spin_lock_irqsave here as you call it from the completion
handler.
> +
> + rxc = &rxs->slots[rx_slot];
> +
> + if (rxc->in_use && !rxc->urb) {
> + rxc->urb = urb;
> + complete(&rxc->done);
> + /* URB will be resubmitted in free_rx_slot */
> + } else {
> + dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> + dln2_submit_urb(dln2, urb, GFP_ATOMIC);
> + }
> +
> + spin_unlock(&rxs->lock);
> +}
> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> + const void *obuf, unsigned obuf_len,
> + void *ibuf, unsigned *ibuf_len)
> +{
> + int ret = 0;
> + u16 result;
> + int rx_slot;
> + struct dln2_response *rsp;
> + struct dln2_rx_context *rxc;
> + struct device *dev;
> + const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> + spin_lock(&dln2->disconnect_lock);
How did you test this version? You never initialise disconnect_lock so
lockdep complains loudly when calling _dln2_transfer during probe.
> + if (!dln2->disconnect)
> + dln2->active_transfers++;
> + else
> + ret = -ENODEV;
> + spin_unlock(&dln2->disconnect_lock);
> +
> + if (ret)
> + return ret;
> +
> + rx_slot = alloc_rx_slot(dln2, handle);
> + if (rx_slot < 0) {
> + ret = rx_slot;
> + goto out_decr;
> + }
> +
> + dev = &dln2->interface->dev;
> +
> + ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> + if (ret < 0) {
> + dev_err(dev, "USB write failed: %d\n", ret);
> + goto out_free_rx_slot;
> + }
> +
> + rxc = &rxs->slots[rx_slot];
> +
> + ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> + if (ret <= 0) {
> + if (!ret)
> + ret = -ETIMEDOUT;
> + goto out_free_rx_slot;
> + }
> +
> + if (!rxc->urb) {
> + ret = -ENODEV;
> + goto out_free_rx_slot;
> + }
> +
> + /* if we got here we know that the response header has been checked */
> + rsp = rxc->urb->transfer_buffer;
> +
> + if (rsp->hdr.size < sizeof(*rsp)) {
> + ret = -EPROTO;
> + goto out_free_rx_slot;
> + }
> +
> + result = le16_to_cpu(rsp->result);
> + if (result) {
> + dev_dbg(dev, "%d received response with error %d\n",
> + handle, result);
> + ret = -EREMOTEIO;
> + goto out_free_rx_slot;
> + }
> +
> + if (!ibuf) {
> + ret = 0;
> + goto out_free_rx_slot;
> + }
> +
> + if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
> + *ibuf_len = rsp->hdr.size - sizeof(*rsp);
> +
> + memcpy(ibuf, rsp + 1, *ibuf_len);
> +
> +out_free_rx_slot:
> + free_rx_slot(dln2, handle, rx_slot);
> +out_decr:
> + spin_lock(&dln2->disconnect_lock);
> + dln2->active_transfers--;
> + spin_unlock(&dln2->disconnect_lock);
> + if (dln2->disconnect)
> + wake_up(&dln2->disconnect_wq);
> +
> + return ret;
> +}
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> + struct dln2_dev *dln2 = usb_get_intfdata(interface);
> + int i, j;
> +
> + /* don't allow starting new transfers */
> + spin_lock(&dln2->disconnect_lock);
> + dln2->disconnect = true;
> + spin_unlock(&dln2->disconnect_lock);
> +
> + /* cancel in progress transfers */
> + for (i = 0; i < DLN2_HANDLES; i++) {
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rxs->lock, flags);
Just stick to spin_lock in this function.
> +
> + /* cancel all response waiters */
> + for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
> + struct dln2_rx_context *rxc = &rxs->slots[j];
> +
> + if (rxc->in_use)
> + complete(&rxc->done);
> + }
> +
> + spin_unlock_irqrestore(&rxs->lock, flags);
> + }
> +
> + /* wait for transfers to end */
> + wait_event(dln2->disconnect_wq, !dln2->active_transfers);
> +
> + mfd_remove_devices(&interface->dev);
> +
> + dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> + const struct usb_device_id *usb_id)
> +{
> + struct usb_host_interface *hostif = i
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Tue, Oct 7, 2014 at 8:10 PM, Johan Hovold wrote: > On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote: >> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold wrote: >> > >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote: >> > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO >> > > Master Adapter DLN-2. Details about the device can be found here: >> > > >> >> >> >> > > + >> > > + ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL); >> > > + if (ret < 0) >> > > + return ret; >> > >> > Is it really worth having this helper only to save a couple of lines on >> > a dev_err? If you do all resubmissions on completion inline in the >> > handler, you only have three places where usb_submit_urb is called. >> > >> >> I moved the completion in the handler as you suggested. I have kept >> the helper, would you prefer to remove it? > > Moved the "completion"? I was suggesting that the URB resubmission > should be done inline the URB completion handler. > > [ "Completion" may be a little ambiguous. The URB callback is called an > URB completion handler. Not be confused with the completion structures > you use to wait for responses. ] > Sorry, I meant to say resubmission instead of completion. > It's fine to keep the helper as long as it's clear that the urb has been > "cached" and should not be resubmitted (inline) in the completion > handler in that case. > Not sure I follow you here. I kept the helper and I call it from the completion handler, from free_rx_slot and from dln2_setup_rx_ubs. > >> >> >> > > + id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum; >> > >> > After giving this some more thought, I think you should just >> > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the >> > other USB MFD drivers and add a convenience helper to register >> > hotpluggable MFDs here: >> > >> >> OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges >> first and I have to resubmit mine, I will switch to using >> mfd_add_hotplug_devices. > > Lee merged the helper function earlier today. > Yep, noticed that, I did a rebase earlier. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote: > On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold wrote: > > > > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote: > > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > > > Master Adapter DLN-2. Details about the device can be found here: > > > > > > > > > + > > > + ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL); > > > + if (ret < 0) > > > + return ret; > > > > Is it really worth having this helper only to save a couple of lines on > > a dev_err? If you do all resubmissions on completion inline in the > > handler, you only have three places where usb_submit_urb is called. > > > > I moved the completion in the handler as you suggested. I have kept > the helper, would you prefer to remove it? Moved the "completion"? I was suggesting that the URB resubmission should be done inline the URB completion handler. [ "Completion" may be a little ambiguous. The URB callback is called an URB completion handler. Not be confused with the completion structures you use to wait for responses. ] It's fine to keep the helper as long as it's clear that the urb has been "cached" and should not be resubmitted (inline) in the completion handler in that case. > > > > > + id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum; > > > > After giving this some more thought, I think you should just > > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the > > other USB MFD drivers and add a convenience helper to register > > hotpluggable MFDs here: > > > > OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges > first and I have to resubmit mine, I will switch to using > mfd_add_hotplug_devices. Lee merged the helper function earlier today. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold wrote: > > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote: > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > > Master Adapter DLN-2. Details about the device can be found here: > > > > + > > + ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL); > > + if (ret < 0) > > + return ret; > > Is it really worth having this helper only to save a couple of lines on > a dev_err? If you do all resubmissions on completion inline in the > handler, you only have three places where usb_submit_urb is called. > I moved the completion in the handler as you suggested. I have kept the helper, would you prefer to remove it? > > + id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum; > > After giving this some more thought, I think you should just > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the > other USB MFD drivers and add a convenience helper to register > hotpluggable MFDs here: > OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges first and I have to resubmit mine, I will switch to using mfd_add_hotplug_devices. > I'll try to take a quick look on the subdrivers on Monday. > Thanks Johan. I have addressed the rest of the comments as well and I will send another series after your reviews on the sub-drivers. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/dln2.c | 751
> +++
> include/linux/mfd/dln2.h | 67 +
> 4 files changed, 828 insertions(+)
> create mode 100644 drivers/mfd/dln2.c
> create mode 100644 include/linux/mfd/dln2.h
[...]
> +#define DLN2_HW_ID 0x200
> +#define DLN2_USB_TIMEOUT 200 /* in ms */
> +#define DLN2_MAX_RX_SLOTS16
> +#define DLN2_MAX_URBS16
> +#define DLN2_RX_BUF_SIZE 512
> +
> +enum dln2_handle {
> + DLN2_HANDLE_EVENT,
This is the only handle that was fixed (0), right? Initialise this one
explicitly in case any one ever tries to reorder them.
> + DLN2_HANDLE_CTRL,
> + DLN2_HANDLE_GPIO,
> + DLN2_HANDLE_I2C,
> + DLN2_HANDLES
> +};
> +
> +/*
> + * Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data.
> + */
> +struct dln2_rx_context {
> + /* completion used to wait for a response */
> + struct completion done;
> +
> + /* if non-NULL the URB contains the response */
> + struct urb *urb;
> +
> + /* if true then this context is used to wait for a response */
> + bool in_use;
> +};
> +
> +/*
> + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to identify the module in
> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> + * slots field and find the receive context for a particular request.
> + */
> +struct dln2_mod_rx_slots {
> + /* RX slots bitmap */
> + unsigned long bmap;
> +
> + /* used to wait for a free RX slot */
> + wait_queue_head_t wq;
> +
> + /* used to wait for an RX operation to complete */
> + struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> +
> + /* avoid races between alloc/free_rx_slot and dln2_rx_transfer */
> + spinlock_t lock;
> +};
> +
> +struct dln2_dev {
> + struct usb_device *usb_dev;
> + struct usb_interface *interface;
> + u8 ep_in;
> + u8 ep_out;
> +
> + struct urb *rx_urb[DLN2_MAX_URBS];
> + void *rx_buf[DLN2_MAX_URBS];
> +
> + struct dln2_mod_rx_slots mod_rx_slots[DLN2_HANDLES];
> +
> + struct list_head event_cb_list;
> + spinlock_t event_cb_lock;
> +
> + bool disconnect;
> + int active_transfers;
> + wait_queue_head_t disconnect_wq;
> + spinlock_t disconnect_lock;
> +};
> +
> +struct dln2_event_cb_entry {
> + struct list_head list;
> + u16 id;
> + struct platform_device *pdev;
> + dln2_event_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +dln2_event_cb_t rx_cb)
> +{
> + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> + struct dln2_event_cb_entry *i, *new;
> + unsigned long flags;
> + int ret = 0;
> +
> + new = kmalloc(sizeof(*new), GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + new->id = id;
> + new->callback = rx_cb;
> + new->pdev = pdev;
> +
> + spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> + list_for_each_entry(i, &dln2->event_cb_list, list) {
> + if (i->id == id) {
> + ret = -EBUSY;
> + break;
> + }
> + }
> +
> + if (!ret)
> + list_add_rcu(&new->list, &dln2->event_cb_list);
> +
> + spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> + if (ret)
> + kfree(new);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(dln2_register_event_c
