Re: [PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices

[Octavian Purdila <octavian.purdila@xxxxxxxxx>]

On Thu, Oct 23, 2014 at 6:16 PM, Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> On Wed, Oct 15, 2014 at 05:48:08PM +0300, Octavian Purdila wrote:
>
> Here's some late feedback due to travels. You managed to get two updates
> in there so commenting on the diff from v6.
>

Thanks for the review :)

> > +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_cb;
>
> Can you use a name which does not have the same suffix as the actual
> callback function (i.e. "_cb"). Just call it "entry" or something.
>

OK.

> > +     unsigned long flags;
> > +     int ret = 0;
> > +
> > +     new_cb = kmalloc(sizeof(*new_cb), GFP_KERNEL);
>
> Use kzalloc here.

Why kzalloc? All of the fields are initialized below.

>
> > +     if (!new_cb)
> > +             return -ENOMEM;
> > +
> > +     new_cb->id = id;
> > +     new_cb->callback = rx_cb;
> > +     new_cb->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_cb->list, &dln2->event_cb_list);
> > +
> > +     spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> > +
> > +     if (ret)
> > +             kfree(new_cb);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(dln2_register_event_cb);
> > +
> > +void dln2_unregister_event_cb(struct platform_device *pdev, u16 id)
> > +{
> > +     struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> > +     struct dln2_event_cb_entry *i;
> > +     unsigned long flags;
> > +     bool found = false;
> > +
> > +     spin_lock_irqsave(&dln2->event_cb_lock, flags);
> > +
> > +     list_for_each_entry(i, &dln2->event_cb_list, list) {
> > +             if (i->id == id) {
> > +                     list_del_rcu(&i->list);
> > +                     found = true;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> > +
> > +     if (found) {
> > +             synchronize_rcu();
> > +             kfree(i);
> > +     }
> > +}
> > +EXPORT_SYMBOL(dln2_unregister_event_cb);
> > +
>
> Please add a comment describing the return value (i.e. when the urb had
> been saved and shouldn't be resubmitted).
>
> Also could rename this helper so it doesn't appear to be a variant of
> dln2_transfer (e.g. something with "complete" in the name).
>

Ok, I will use dln2_transfer_complete.

> > +static bool 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;
> > +     bool ret = false;
> > +
> > +     rxc = &rxs->slots[rx_slot];
> > +
> > +     /*
> > +      * No need to disable interrupts as this lock is not taken in
> > +      * interrupt context elsewhere in this driver and this
> > +      * function (or its callers) are not exported to other
> > +      * modules.
>
> Why do you break comment lines already at 70 chars?
>

Oops, relic in my .emacs, will fix all comments in v9.

> > +      */
> > +     spin_lock(&rxs->lock);
> > +     if (rxc->in_use && !rxc->urb) {
> > +             rxc->urb = urb;
> > +             complete(&rxc->done);
> > +             ret = true;
> > +     }
> > +     spin_unlock(&rxs->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +static void dln2_run_event_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
> > +                                  void *data, int len)
> > +{
> > +     struct dln2_event_cb_entry *i;
> > +
> > +     rcu_read_lock();
> > +
> > +     list_for_each_entry_rcu(i, &dln2->event_cb_list, list)
> > +             if (i->id == id)
> > +                     i->callback(i->pdev, echo, data, len);
> > +
> > +     rcu_read_unlock();
> > +}
> > +
> > +static void dln2_rx(struct urb *urb)
> > +{
> > +     struct dln2_dev *dln2 = urb->context;
> > +     struct dln2_header *hdr = urb->transfer_buffer;
> > +     struct device *dev = &dln2->interface->dev;
> > +     u16 id, echo, handle, size;
> > +     u8 *data;
> > +     int len;
> > +     int err;
> > +
> > +     switch (urb->status) {
> > +     case 0:
> > +             /* success */
> > +             break;
> > +     case -ECONNRESET:
> > +     case -ENOENT:
> > +     case -ESHUTDOWN:
> > +     case -EPIPE:
> > +             /* this urb is terminated, clean up */
> > +             dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
> > +             return;
> > +     default:
> > +             dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
> > +             goto out;
> > +     }
> > +
> > +     if (urb->actual_length < sizeof(struct dln2_header)) {
> > +             dev_err(dev, "short response: %d\n", urb->actual_length);
> > +             goto out;
> > +     }
> > +
> > +     handle = le16_to_cpu(hdr->handle);
> > +     id = le16_to_cpu(hdr->id);
> > +     echo = le16_to_cpu(hdr->echo);
> > +     size = le16_to_cpu(hdr->size);
> > +
> > +     if (size != urb->actual_length) {
> > +             dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d actual %d\n",
> > +                     handle, id, echo, size, urb->actual_length);
> > +             goto out;
> > +     }
> > +
> > +     if (handle >= DLN2_HANDLES) {
> > +             dev_warn(dev, "RX: invalid handle %d\n", handle);
> > +             goto out;
> > +     }
> > +
> > +     data = urb->transfer_buffer + sizeof(struct dln2_header);
> > +     len = urb->actual_length - sizeof(struct dln2_header);
> > +
> > +     if (handle == DLN2_HANDLE_EVENT) {
> > +             dln2_run_event_callbacks(dln2, id, echo, data, len);
> > +     } else {
> > +             /* URB will be re-submitted in free_rx_slot */
>
> Refer to _dln2_transfer (the only place where free_rx_slot is used) as
> well.
>

OK.

> > +             if (dln2_rx_transfer(dln2, urb, handle, echo))
> > +                     return;
> > +             dev_warn(dev, "bad/late response %d/%d\n", handle, echo);
>
> Move this message back to the helper.
>

OK.

> > +     }
> > +
> > +out:
> > +     err = usb_submit_urb(urb, GFP_ATOMIC);
> > +     if (err < 0)
> > +             dev_err(dev, "failed to resubmit RX URB: %d\n", err);
> > +}
> > +
>
> [...]
>
> > +static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
> > +                           struct usb_host_interface *hostif)
> > +{
> > +     int i;
> > +     const int rx_max_size = DLN2_RX_BUF_SIZE;
> > +
> > +     for (i = 0; i < DLN2_MAX_URBS; i++) {
> > +             int ret;
> > +             struct device *dev = &dln2->interface->dev;
>
> Move these out of the loop.
>
> > +
> > +             dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
> > +             if (!dln2->rx_buf[i])
> > +                     return -ENOMEM;
> > +
> > +             dln2->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> > +             if (!dln2->rx_urb[i])
> > +                     return -ENOMEM;
> > +
> > +             usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
> > +                               usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
> > +                               dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
> > +
> > +             ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to submit RX URB: %d\n", ret);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static struct dln2_platform_data dln2_pdata_gpio = {
> > +     .handle = DLN2_HANDLE_GPIO,
> > +};
> > +
> > +/* Only one I2C port seems to be supported on current hardware */
> > +static struct dln2_platform_data dln2_pdata_i2c = {
> > +     .handle = DLN2_HANDLE_I2C,
> > +     .port = 0,
> > +};
> > +
> > +static const struct mfd_cell dln2_devs[] = {
> > +     {
> > +             .name = "dln2-gpio",
> > +             .platform_data = &dln2_pdata_gpio,
> > +             .pdata_size = sizeof(struct dln2_platform_data),
> > +     },
> > +     {
> > +             .name = "dln2-i2c",
> > +             .platform_data = &dln2_pdata_i2c,
> > +             .pdata_size = sizeof(struct dln2_platform_data),
> > +     },
> > +};
> > +
> > +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);
> > +
> > +             /* 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 = interface->cur_altsetting;
> > +     struct device *dev = &interface->dev;
> > +     struct dln2_dev *dln2;
> > +     int ret;
> > +     int i, j;
> > +
> > +     if (hostif->desc.bInterfaceNumber != 0 ||
> > +         hostif->desc.bNumEndpoints < 2)
> > +             return -ENODEV;
> > +
> > +     dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
> > +     if (!dln2)
> > +             return -ENOMEM;
> > +
> > +     dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
> > +     dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
> > +     dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> > +     dln2->interface = interface;
> > +     usb_set_intfdata(interface, dln2);
> > +     init_waitqueue_head(&dln2->disconnect_wq);
> > +
> > +     for (i = 0; i < DLN2_HANDLES; i++) {
> > +             init_waitqueue_head(&dln2->mod_rx_slots[i].wq);
> > +             spin_lock_init(&dln2->mod_rx_slots[i].lock);
> > +             for (j = 0; j < DLN2_MAX_RX_SLOTS; j++)
> > +                     init_completion(&dln2->mod_rx_slots[i].slots[j].done);
> > +     }
> > +
> > +     spin_lock_init(&dln2->event_cb_lock);
> > +     spin_lock_init(&dln2->disconnect_lock);
> > +     INIT_LIST_HEAD(&dln2->event_cb_list);
> > +
> > +     ret = dln2_setup_rx_urbs(dln2, hostif);
> > +     if (ret)
> > +             goto out_cleanup;
> > +
> > +     ret = dln2_hw_init(dln2);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to initialize hardware\n");
> > +             goto out_cleanup;
> > +     }
> > +
> > +     ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
>
> So this now depends on 15bb4d6e8534 ("mfd: core: Add helper function to
> register hotplug devices") in the MFD tree.
>
> Please mention what tree the patch is against in your cover letter (I
> noticed you had rebased when it no longer applied to 3.17).
>
> You should drop the gpiolib patch now that v3.18-rc1 is out as well.

This series is based against the Lee's for-mfd-next-v3.19 tree that
does not yet contain the gpiolib patch.

>
> > +     if (ret != 0) {
> > +             dev_err(dev, "failed to add mfd devices to core\n");
> > +             goto out_cleanup;
> > +     }
> > +
> > +     return 0;
> > +
> > +out_cleanup:
> > +     dln2_free(dln2);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct usb_device_id dln2_table[] = {
> > +     { USB_DEVICE(0xa257, 0x2013) },
> > +     { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(usb, dln2_table);
> > +
> > +static struct usb_driver dln2_driver = {
> > +     .name = "dln2",
> > +     .probe = dln2_probe,
> > +     .disconnect = dln2_disconnect,
> > +     .id_table = dln2_table,
> > +};
> > +
> > +module_usb_driver(dln2_driver);
> > +
> > +MODULE_AUTHOR("Octavian Purdila <octavian.purdila@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Core driver for the Diolan DLN2 interface adapter");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
> > new file mode 100644
> > index 0000000..57ddc58
> > --- /dev/null
> > +++ b/include/linux/mfd/dln2.h
> > @@ -0,0 +1,69 @@
> > +#ifndef __LINUX_USB_DLN2_H
> > +#define __LINUX_USB_DLN2_H
> > +
> > +#define DLN2_CMD(cmd, id)            ((cmd) | ((id) << 8))
> > +
> > +struct dln2_platform_data {
> > +     u16 handle;             /* sub-driver handle (internally used only) */
> > +     u8 port;                /* I2C/SPI port */
> > +};
> > +
> > +/**
> > + * dln2_event_cb_t - event callback function signature
> > + *
> > + * @pdev - the sub-device that registered this callback
> > + * @echo - the echo header field received in the message
> > + * @data - the data payload
> > + * @len  - the data payload length
> > + *
> > + * The callback function is called in interrupt context and the data
> > + * payload is only valid during the call. If the user needs later
> > + * access of the data, it must copy it.
> > + */
> > +
> > +typedef void (*dln2_event_cb_t)(struct platform_device *pdev, u16 echo,
> > +                             const void *data, int len);
> > +
> > +/**
> > + * dl2n_register_event_cb - register a callback function for an event
> > + *
> > + * @pdev - the sub-device that registers the callback
> > + * @event - the event for which to register a callback
> > + * @event_cb - the callback function
> > + *
> > + * @return 0 in case of success, negative value in case of error
> > + */
> > +int dln2_register_event_cb(struct platform_device *pdev, u16 event,
> > +                        dln2_event_cb_t event_cb);
> > +
> > +/**
> > + * dln2_unregister_event_cb - unregister the callback function for an event
> > + *
> > + * @pdev - the sub-device that registered the callback
> > + * @event - the event for which to register a callback
> > + */
> > +void dln2_unregister_event_cb(struct platform_device *pdev, u16 event);
> > +
> > +/**
> > + * dln2_transfer - issue a DLN2 command and wait for a response and
> > + * the associated data
> > + *
> > + * @pdev - the sub-device which is issuing this transfer
> > + * @cmd - the command to be sent to the device
> > + * @obuf - the buffer to be sent to the device; it can be NULL if the
> > + *   user doesn't need to transmit data with this command
> > + * @obuf_len - the size of the buffer to be sent to the device
> > + * @ibuf - any data associated with the response will be copied here;
> > + *   it can be NULL if the user doesn't need the response data
> > + * @ibuf_len - must be initialized to the input buffer size; it will
> > + *   be modified to indicate the actual data transfered;
> > + * @result - pointer to store the result code received from hardware;
> > + *   it can be NULL if the user doesn't need the result code
> > + *
> > + * @return 0 for success, negative value for errors
> > + */
> > +int dln2_transfer(struct platform_device *pdev, u16 cmd,
> > +               const void *obuf, unsigned obuf_len,
> > +               void *ibuf, unsigned *ibuf_len, u16 *result);
>
> Why add yet another parameter for the result and then never even use it?
> Please remove it. You can add a new function for it (and a wrapper)
> later if it's ever needed.
>

It is needed for setting the frequency, but you are right I should add
it with that patch set.

> You should also consider adding a convenience function for when you
> don't care about any returned data (e.g. dln2_transfer_tx) so you don't
> have to pass all those NULLs (most calls currently have three NULL
> params).
>

OK, will do it in v9.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html