lkml.org 
[lkml]   [2013]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] extcon: Palmas Extcon Driver
On Monday 06 May 2013 06:47 PM, Kishon Vijay Abraham I wrote:
> +
> +static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)

Can we name the function to palams_vbus_irq_handler() for better
understanding? Reserve the wakeup word for the suspend-wakeups.


> +
> +
> +static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)

Same here for better name.


> +
> +static void palmas_set_vbus_work(struct work_struct *data)
> +{
> + int ret;
> + struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
> + set_vbus_work);
> +
> + if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
> + dev_err(palmas_usb->dev, "invalid regulator\n");
> + return;
> + }

This error will keep coming if the vbus is not require as workqueue get
scheduled always. I think we should remove it.


> +

> +static void palmas_dt_to_pdata(struct device_node *node,
> + struct palmas_usb_platform_data *pdata)
> +{
> + pdata->no_control_vbus = of_property_read_bool(node,
> + "ti,no_control_vbus");


Can we change the variable names to enable_control_bus and logic
accordingly as it looks more appropriate and easy to understand?


> +
> + palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
> + PALMAS_ID_OTG_IRQ);
> + palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
> + PALMAS_ID_IRQ);
> + palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
> + PALMAS_VBUS_OTG_IRQ);
> + palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
> + PALMAS_VBUS_IRQ);
> +

Better to name irq1, irq2 in more logical names for easy understanding.


> +
> + if (device_create_file(&pdev->dev, &dev_attr_vbus))
> + dev_warn(&pdev->dev, "could not create sysfs file\n");
> +
> + palmas_usb->edev.name = "palmas_usb";
> + palmas_usb->edev.supported_cable = palmas_extcon_cable;
> + palmas_usb->edev.mutually_exclusive = mutually_exclusive;
> +
> + ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register extcon device\n");
> + return ret;

It need to destroy sysfs also.

> + }
> +
> + /* init spinlock for workqueue */
> + spin_lock_init(&palmas_usb->lock);

It is already done above.

> +
> + INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);

Better to create the workqueu when control_vbus is require.


> +
> +
> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
> new file mode 100644
> index 0000000..a5119c9
> --- /dev/null
> +++ b/include/linux/extcon/extcon_palmas.h

I think it can be use palama.h only. No need to have one more header for
this.

> @@ -0,0 +1,26 @@
>
>
> - u8 linkstat;
> + int mailboxstat;
Do we really require mailboxstat?





\
 
 \ /
  Last update: 2013-05-06 17:21    [W:0.212 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site