| 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
| ||
|
Message-ID: <E88A1D35AF5437468E5B14C7C4BA61C702D37CAAE8@HQMAIL01.nvidia.com>
Date: Thu, 21 Oct 2010 10:22:12 -0700
From: Rhyland Klein <rklein@...dia.com>
To: Anton Vorontsov <cbouatmailru@...il.com>,
Lars-Peter Clausen <lars@...afoo.de>
CC: "broonie@...nsource.wolfsonmicro.com"
<broonie@...nsource.wolfsonmicro.com>,
Andrew Chew <AChew@...dia.com>,
"olof@...om.net" <olof@...om.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] POWER: Add gpio chager driver
> From: Anton Vorontsov [mailto:cbouatmailru@...il.com]
> Sent: Thursday, October 21, 2010 7:14 AM
> To: Lars-Peter Clausen
> Cc: Rhyland Klein; broonie@...nsource.wolfsonmicro.com; Andrew Chew;
> olof@...om.net; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] POWER: Add gpio chager driver
>
> On Thu, Oct 21, 2010 at 02:25:57PM +0200, Lars-Peter Clausen wrote:
> > This patch adds a simple driver for chargers indicating their online
> status
> > through a GPIO pin.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
>
> I agree, the GPIO name fits better. Rhyland, are you OK with that
> driver, does it fit your needs?
This would be great. I will give it a test but from reading over it, it sounds perfect for my needs.
>
> Some comments down below.
>
> [...]
> > +struct gpio_charger {
> > + const struct gpio_charger_platform_data *pdata;
> > + int irq;
>
> This can be unsigned.
>
> > +
> > + struct power_supply charger;
> > +};
> > +
> > +static irqreturn_t gpio_charger_irq(int irq, void *devid)
> > +{
> > + struct power_supply *charger = devid;
>
> Please add an empty line here.
>
> > + power_supply_changed(charger);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static inline struct gpio_charger *psy_to_gpio_charger(struct
> power_supply *psy)
> > +{
> > + return container_of(psy, struct gpio_charger, charger);
> > +}
> > +
> > +static int gpio_charger_get_property(struct power_supply *psy,
> > + enum power_supply_property psp, union power_supply_propval *val)
>
> One more tab for here?
>
> > +{
> > + struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
> > + const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
> > +
> > + switch (psp) {
> > + case POWER_SUPPLY_PROP_ONLINE:
> > + val->intval = gpio_get_value(pdata->gpio);
> > + val->intval ^= pdata->gpio_active_low;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static enum power_supply_property gpio_charger_properties[] = {
> > + POWER_SUPPLY_PROP_ONLINE,
> > +};
> > +
> > +static int __devinit gpio_charger_probe(struct platform_device *pdev)
> > +{
> > + const struct gpio_charger_platform_data *pdata = pdev-
> >dev.platform_data;
> > + struct gpio_charger *gpio_charger;
> > + struct power_supply *charger;
> > + int ret;
> > +
> > + if (!pdata) {
> > + dev_err(&pdev->dev, "No platform data");
> > + return -EINVAL;
> > + }
> > +
> > + if (!gpio_is_valid(pdata->gpio)) {
> > + dev_err(&pdev->dev, "Invalid gpio pin\n");
> > + return -EINVAL;
> > + }
> > +
> > + gpio_charger = kzalloc(sizeof(*gpio_charger), GFP_KERNEL);
> > +
> > + charger = &gpio_charger->charger;
> > +
> > + charger->name = pdata->name;
> > + charger->type = pdata->type;
> > + charger->properties = gpio_charger_properties;
> > + charger->num_properties = ARRAY_SIZE(gpio_charger_properties);
> > + charger->get_property = gpio_charger_get_property;
> > + charger->supplied_to = pdata->supplied_to;
> > + charger->num_supplicants = pdata->num_supplicants;
> > +
> > + ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
> > + goto err_free;
> > + }
> > + ret = gpio_direction_input(pdata->gpio);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
> > + goto err_gpio_free;
> > + }
> > +
> > + gpio_charger->irq = gpio_to_irq(pdata->gpio);
> > + if (gpio_charger->irq >= 0) {
>
> 0 isn't valid IRQ number. The check should be just 'if (gpio_charger-
> >irq)'.
>
> > + ret = request_irq(gpio_charger->irq, gpio_charger_irq,
> > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > + dev_name(&pdev->dev), charger);
> > + if (ret) {
> > + dev_warn(&pdev->dev, "Failed to request gpio irq: %d\n",
> ret);
> > + gpio_charger->irq = -1;
> > + }
> > + }
> > +
> > + gpio_charger->pdata = pdata;
> > +
> > + ret = power_supply_register(&pdev->dev, charger);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Failed to register power supply: %d\n",
> ret);
> > + goto err_irq_free;
> > + }
> > +
> > + platform_set_drvdata(pdev, gpio_charger);
> > +
> > + return 0;
> > +
> > +err_irq_free:
> > + if (gpio_charger->irq >= 0)
> > + free_irq(gpio_charger->irq, charger);
> > +err_gpio_free:
> > + gpio_free(pdata->gpio);
> > +err_free:
> > + kfree(gpio_charger);
> > + return ret;
> > +}
> > +
> > +static int __devexit gpio_charger_remove(struct platform_device *pdev)
> > +{
> > + struct gpio_charger *gpio_charger = platform_get_drvdata(pdev);
> > + const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
> > +
> > + power_supply_unregister(&gpio_charger->charger);
> > +
> > + if (gpio_charger->irq >= 0)
> > + free_irq(gpio_charger->irq, &gpio_charger->charger);
> > + gpio_free(pdata->gpio);
> > +
> > + platform_set_drvdata(pdev, NULL);
> > + kfree(gpio_charger);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver gpio_charger_driver = {
> > + .probe = gpio_charger_probe,
> > + .remove = __devexit_p(gpio_charger_remove),
> > + .driver = {
> > + .name = "gpio-charger",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static int __init gpio_charger_init(void)
> > +{
> > + return platform_driver_register(&gpio_charger_driver);
> > +}
> > +module_init(gpio_charger_init);
> > +
> > +static void __exit gpio_charger_exit(void)
> > +{
> > + platform_driver_unregister(&gpio_charger_driver);
> > +}
> > +module_exit(gpio_charger_exit);
> > +
> > +MODULE_AUTHOR("Lars-Peter Clausen <lars@...afoo.de>");
> > +MODULE_DESCRIPTION("Driver for chargers which report their online status
> through a GPIO");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:gpio-charger");
> > diff --git a/include/linux/power/gpio-charger.h
> b/include/linux/power/gpio-charger.h
> > new file mode 100644
> > index 0000000..404d5e6
> > --- /dev/null
> > +++ b/include/linux/power/gpio-charger.h
> > @@ -0,0 +1,39 @@
> > +/*
> > + * Copyright (C) 2010, Lars-Peter Clausen <lars@...afoo.de>
> > + *
> > + * This program is free software; you can redistribute it and/or
> modify it
> > + * under the terms of the GNU General Public License as published
> by the
> > + * Free Software Foundation; either version 2 of the License, or (at
> your
> > + * option) any later version.
> > + *
> > + * You should have received a copy of the GNU General Public License
> along
> > + * with this program; if not, write to the Free Software Foundation,
> Inc.,
> > + * 675 Mass Ave, Cambridge, MA 02139, USA.
> > + *
> > + */
> > +
> > +#ifndef __LINUX_POWER_GPIO_CHARGER_H__
> > +#define __LINUX_POWER_GPIO_CHARGER_H__
> > +
> > +/*
>
> Should be /**
>
> > + * struct gpio_charger_platform_data - platform_data for gpio_charger
> devices
> > + * @name: Name for the chargers power_supply device
> > + * @type: Type of the charger
> > + * @gpio: GPIO which is used to indicate the chargers status
> > + * @gpio_active_low: Should be set to 1 if the GPIO is active low
> otherwise 0
> > + * @supplied_to: Array of battery names to which this chargers
> supplies power
> > + * @num_supplicants: Number of entries in the supplied_to array
> > + */
> > +
>
> No need for this empty line.
>
> > +struct gpio_charger_platform_data {
> > + const char *name;
> > + enum power_supply_type type;
>
> #include <linux/power_supply.h> is needed for this.
>
> > +
> > + int gpio;
> > + int gpio_active_low;
> > +
> > + char **supplied_to;
> > + size_t num_supplicants;
>
> And #include <linux/types.h> for this.
>
> > +};
> > +
> > +#endif
>
> Thanks!
>
> --
> Anton Vorontsov
> email: cbouatmailru@...il.com
> irc://irc.freenode.net/bd2
Powered by blists - more mailing lists