linux-kernel - Re: [PATCHv4 2/11] GPIO: GPIO module of DA9052 device 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] [day] [month] [year] [list]

Message-ID: <20101222115752.GB4520@sirena.org.uk>
Date:	Wed, 22 Dec 2010 11:57:52 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	dd diasemi <dd.diasemi@...il.com>
Cc:	jic23@....ac.uk, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 2/11] GPIO: GPIO module of DA9052 device driver

On Tue, Dec 21, 2010 at 06:47:19PM +0100, dd diasemi wrote:
> GPIO module for DA9052 PMIC device from Dialog Semiconductor.

> Changes made since last submission:
> . locking for individual GPI pin configuration
> . event registration

> Linux Kernel Version: 2.6.34

As previously mentioned you should be submitting against current kernel
versions.  *Please* read and try to follow Documentation/SubmittingPatches.

Looking through this it seems there's several issues here which have
been pointed out with previous versions of the patch.  Please do address
issues identified in review, either by discussing anything you disagree
with on the list or correcting the code.

> +void da9052_gpio_notifier(struct da9052_eh_nb *eh_data, unsigned int event)
> +{
> +	struct da9052_gpio_chip *gpio =
> +			container_of(eh_data, struct da9052_gpio_chip, eh_data);
> +	kobject_uevent(&gpio->gp.dev->kobj, KOBJ_CHANGE);
> +
> +}

What is this doing?  It looks awfully like this is implementing
interrupt handling - you should be using the genirq framework for this,
it provides standard interrupt handling for the kernel.

> +static s32 write_default_gpio_values(struct da9052 *da9052)
> +{
> +	struct da9052_ssc_msg msg;
> +	u8 created_val = 0;
> +
> +#if (DA9052_GPIO_PIN_0 == DA9052_GPIO_CONFIG)
> +	da9052_lock(da9052);
> +	msg.addr = DA9052_GPIO0001_REG;
> +	msg.data = 0;

This appears to be some sort of system specific configuration - things
like this should be being configured by supplying platform data to the
device.

> +s32 da9052_gpio_multiple_read(struct da9052_gpio_multiple_read *multiple_port,
> +				struct da9052 *da9052)

APIs like this should be added to the gpiolib core rather than done in a
driver custom manner - there were some previous efforts at this which
didn't get merged for whatever reason.  There's nothing particularly
device specific about the idea of setting multiple GPIOs in a single
operation.

> +config DA9052_GPIO_ENABLE
> +	bool "Dialog DA9052 GPIO"
> +	depends on PMIC_DA9052
> +	help
> +	  Say Y  to enable the GPIO driver for the DA9052 chip
> +

The _ENABLE isn't idiomatic here...

> +enum ip_op_type {
> +	ALTERNATE_FUNCTIONALITY = 0,
> +	INPUT,
> +	OUTPUT_OPENDRAIN,
> +	OUTPUT_PUSHPULL
> +};

All these constants and most of the remaining ones in the file need
namespacing.
--
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/