Messages in this thread | | | Date | Tue, 7 May 2013 10:36:44 +0530 | From | Kishon Vijay Abraham I <> | Subject | Re: [PATCH v4] extcon: Palmas Extcon Driver |
| |
Hi,
On Monday 06 May 2013 07:56 PM, Laxman Dewangan wrote: > 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?
Will fix your comments.
Thanks Kishon
| |