linux-kernel - Re: [PATCH v4] extcon: Palmas Extcon Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51888BE4.8090502@ti.com>
Date:	Tue, 7 May 2013 10:36:44 +0530
From:	Kishon Vijay Abraham I <kishon@...com>
To:	Laxman Dewangan <ldewangan@...dia.com>
CC:	"myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>,
	"cw00.choi@...sung.com" <cw00.choi@...sung.com>,
	"balbi@...com" <balbi@...com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"rob@...dley.net" <rob@...dley.net>,
	"gg@...mlogic.co.uk" <gg@...mlogic.co.uk>,
	"ruchika@...com" <ruchika@...com>,
	"tony@...mide.com" <tony@...mide.com>,
	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/