linux-kernel - Re: [Qualcomm PM8921 MFD v2 3/6] gpio: pm8xxx-gpio: Add pm8xxx gpio 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]

Date:	Wed, 16 Mar 2011 13:54:44 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Abhijeet Dharmapurikar <adharmap@...eaurora.org>
Cc:	davidb@...eaurora.org, "David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	Daniel Walker <dwalker@...o99.com>,
	David Collins <collinsd@...eaurora.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Joe Perches <joe@...ches.com>,
	Russell King <linux@....linux.org.uk>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Stepan Moskovchenko <stepanm@...eaurora.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Linus Walleij <linux.walleij@...rricsson.com>,
	Thomas Glexiner <tglx@...utronix.de>,
	linux-arm-kernel@...ts.infradead.org,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Qualcomm PM8921 MFD v2 3/6] gpio: pm8xxx-gpio: Add pm8xxx gpio
 driver

On Wed, Mar 16, 2011 at 11:55:19AM -0700, Abhijeet Dharmapurikar wrote:
> 
> >>
> >>diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> >>index 664660e..c5e6f51 100644
> >>--- a/drivers/gpio/Kconfig
> >>+++ b/drivers/gpio/Kconfig
> >>@@ -411,4 +411,14 @@ config GPIO_JANZ_TTL
> >> 	  This driver provides support for driving the pins in output
> >> 	  mode only. Input mode is not supported.
> >>+comment "SSBI GPIO expanders:"
> >
> >SSBI?  Also, the comment seems rather out of place when there
> >currently appears to only be one of such devices.
> 
> SSBI is a bus architecture used to access this device's register
> (actually this is a subdevice in the pmic and ssbi is used to access
> all the registers in the pmic).The bus driver can be found here
> https://patchwork.kernel.org/patch/601771/
> 
> It didnt occur to me that the comment is only meant for buses which
> have more than one gpio devices. I saw
> comment "MODULbus GPIO expanders:"
> comment "AC97 GPIO expanders:"
> both containing single devices and thought it is a norm to add a
> comment  if a device running on a new bus is introduced.
> 
> Let me know if you still think I should remove
> comment "SSBI GPIO expanders:" ?

Yes, remove the comment.  I'll probably also remove the comment for
MODULbus and AC97.

g.

> 
> >>+
> >>+struct pm_gpio_chip {
> >>+	struct list_head	link;
> >>+	struct gpio_chip	gpio_chip;
> >>+	struct mutex		pm_lock;
> >>+	u8			*bank1;
> >>+	int			irq_base;
> >>+};
> >>+
> >>+static LIST_HEAD(pm_gpio_chips);
> >
> >Looks like you need a mutex for protecting this list from mutual access.
> 
> Yes will fix this.
> 
> >
> >>+
> 
> >>+#ifndef __PM8XXX_GPIO_H
> >>+#define __PM8XXX_GPIO_H
> >>+
> >>+#include <linux/errno.h>
> >>+
> >>+#define PM8XXX_GPIO_DEV_NAME	"pm8xxx-gpio"
> >>+
> >>+struct pm8xxx_gpio_core_data {
> >>+	u32	rev;
> >>+	int	ngpios;
> >>+};
> >>+
> >>+struct pm8xxx_gpio_platform_data {
> >>+	struct pm8xxx_gpio_core_data	gpio_cdata;
> >>+	int				gpio_base;
> >>+};
> >
> >There doesn't seem to be any value it splitting pm8xxx_gpio_core_data
> >into a separate structure from what I see in this patch.  How is this
> >going to be used?
> 
> gpio_base comes from the platform data because the board file knows
> where in the global gpio numbers the pm8xxx gpio start. For example
> if the MSM code supports 150 gpios(0 through 149), the board file
> will set gpio_base to indicate pm8xxx gpios should start from 150.
> 
> The pm8xxx_gpio_core_data is meant to be filled in by the pm8921
> core. We have different pmic chips with similar gpio
> implementations. For example 8058 pmic, 8901 pmic and 8921 pmic, all
> have the same gpio implementation but different number of gpio
> lines. The pm8xxx-gpio.c is used for all these pmics. Hence we
> separated core specific gpio information (number of gpio lines
> supported) from board specific gpio information (where in the global
> map the gpio number for this device starts).

I could see the argument if multiple pm8xxx instances pointed to a
single pm8xxx_gpio_core_data structure, but in this case it is simply
encapsulated.  Personally I'd just drop the extra structure.

g.

--
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/