On Thu, Oct 9, 2014 at 10:44 PM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Thu, 2014-10-09 at 22:22 +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:
>
> trivia:
>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> []
>> +config MFD_DLN2
>> + tristate "Diolan DLN2 support"
>> + select MFD_CORE
>> + depends on USB
>> + help
>> + This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter DLN-2.
>> + Additional drivers must be enabled in order to use the functionality
>> + of the device.
>
> additional drivers like...
>
I will rephrase.
>> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> []
> +struct dln2_mod_rx_slots {
> + /* RX slots bitmap */
> + unsigned long bmap;
>
> Probably better as:
> DECLARE_BITMAP(bmap, DLN2_MAX_RX_SLOTS);
>
> Then a lot of &ptr->bmap uses can be ptr->bmap
>
Originally I was using DECLARE_BITMAP, but during the review process
Johan suggested to use unsigned long. Now that I think about it, it
sounds better to use DECLARE_BITMAP and of couse keep using
find_first_bit, set_bit, etc. Johan do you see any issue with that?
>> +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;
>> +};
>
> Maybe reorder the bools and u8s to pack this a bit better?
>
I prefer to keep it this way, it's not wasting a lot since you will
only have a handful of these devices, and it keeps the related data
together.
>> +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;
>
> new isn't a very good name
>
Yes, new_cb should be better.
>> +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);
>
> probably nicer with braces
>
>> +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;
>> +
>> + dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
>> + if (!dln2->rx_buf[i])
>> + return -ENOMEM;
>
> memory leaks on failure?
>
No, dln2_free_rx_urbs will do the cleanup (even in case of failures above).
Thanks for the review Joe.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
