Message-ID: <20100521000622.GA3110@sirena.org.uk>
Date:	Fri, 21 May 2010 01:06:22 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	David Dajun Chen <Dajun.Chen@...semi.com>
Cc:	linux-kernel@...r.kernel.org, sameo@...ux.intel.com
Subject: Re: [PATCH] MFD of DA9052 Linux device drivers (1/9)

On Wed, May 19, 2010 at 10:16:10AM +0100, David Dajun Chen wrote:

> The attached is the MFD part of the device drivers newly developed for
> DA9052 Power Management IC from Dialog Semiconductor.

One thing that jumps out at me from this patch is that it seems like
it'd really benefit from being further split up so that the patch is not
so very large.  This will make review much easier, usually the larger
the individual patch the more effort is required to review.  For
example, the code to add ADC and IRQ support both look like very good
candidates for being split out of the core patch.  Similarly, the
headers for things like the RTC would probably be best placed in the
patch that adds that driver.

Addressing some of the issues that have been raised by myself and others
regarding the use of typedefs and the addition of driver-specific ioctl
interfaces would also go a long way to making review easier.  Much of
the code here appears to result from reinventing things that already
have standard implementations in Linux.

I'd also suggest using scripts/checkpatch.pl to check for coding style
issues.

> + * DA9052 has a single IRQ line which can be used to indicate 32 various events.
> + * Event Handler (EH) module has following primary functions.
> + *
> + * (1) To receive and acknowledge interrupts from DA9052.
> + * (2) Extract all events form DA9052 event registers after receiving interrupt.
> + * (3) To provide a mechanism to register call back for specific events of
> + *     their interest, and call these call backs when corresponding event occurs.
> + * (4) To retrive, store and provide TSI data whenever requested by TSI module.

The best way to handle the interrupt controller in devices like this in
Linux is through the generic IRQ framework.  This means a lot of the
event dispatch code is provided for you, gives you diagnostics and
allows good integration with the rest of the kernel (both for the
primary IRQ and also for the IRQs generated by GPIOs on the device).

Drivers such as 88pm860x and wm831x provide examples of doing this in
mainline.

> +static void da9052_eh_setup_irq(void)
> +{
> +	/* Put your platform and board specific code here */
> +#if (ARCH_DEPENDENT_DA9052)	
> +	/* Set up GPI which is connected to nIRQ of DA9052 */
T> +	s3c_gpio_setpull(DA9052_IRQ_PIN, S3C_GPIO_PULL_UP);
> +	
> +	/* Set GPI as LOW LEVEL interrupt source */ 
> +	set_irq_type(DA9052_IRQ, IRQ_TYPE_LEVEL_LOW);

Use platform data for this - you can provide a callback function which
will be run when the device is registered which machines can use to do
any setup that needs to be done when the driver is starting up (or for
things like this they can just do it in the core machine startup).

> +/**
> + * da9052_eh_restore_irq:Function to free IRQ line which is connected to DA9052 
> + * @param void	
> + * @return void
> + */
> +static void da9052_eh_restore_irq(void)
> +{
> +	/* Put your platform and board specific code here */
> +	free_irq(DA9052_IRQ, NULL);

The IRQ used by the device should be passed in through the standard
mechanisms for the bus concerned - eg, the .irq field of the I2C board
info structure.

> +static irqreturn_t da9052_eh_isr(int irq, void *dev_id)
> +{	
> +	/* Schedule work to be done */
> +	schedule_work(&eh_info.eh_isr_work);

Use request_threaded_irq() with IRQF_ONESHOT.

> +/**
> + * da9052_tsi_start_sampling:	Called by TSI driver to indicate start of data
> + * sampling
> + * @param void
> + * @return int status
> + */
> +s32 da9052_tsi_start_sampling(void){

This and all the other touchscreen stuff in here almost certainly want
to be part of your touchscreen driver.

> +/**
> + * da9052_eh_resume: Power Management support function
> + * 
> + * @param *dev   pointer to platform device
> + * @return s32 status of resume operation
> + */
> +static s32 da9052_eh_resume(struct platform_device *dev)
> +{
> +	/* Put your resume related operations here */
> +	printk(KERN_INFO "%s: called\n", __FUNCTION__);
> +	return (SUCCESS);
> +}

As with your other similar suspend and resume callbacks if this is
really needed you should provide platform data based callbacks rather
than expecting users to modify the driver - remember, people might want
to submit their boards to mainline.  It would seem very unusual to use
most if not all of these, though.

> +	depends on MFD_DA9052
> +config MFD_DA9052_SPI
> +	bool "SPI"
> +	select SPI
> +	select GPIOLIB
> +	help
> +	  Say Y  to select SPI serial interface for DA9052 chip
> +
> +config MFD_DA9052_I2C
> +	bool "I2C"
> +	select I2C
> +	help
> +	  Say Y  to select I2C serial interface for DA9052 chip

There should be no reason not to support building both bus types into
the same kernel.  Quite a few drivers in the kernel provide examples of
doing this.

> --- linux-2.6.33.2/include/linux/compiler-gcc4.h	2010-04-02 04:02:33.000000000 +0500
> +++ linux-2.6.33.2_patch/include/linux/compiler-gcc4.h	2010-05-18 15:29:36.000000000 +0500
> @@ -3,9 +3,19 @@
>  #endif
>  
>  /* GCC 4.1.[01] miscompiles __weak */
> +#define GCC_VERSION (__GNUC__ * 10000 \
> +		     + __GNUC_MINOR__ * 100 \
> +		     + __GNUC_PATCHLEVEL__)
> +

The changes in here are totally unrelated to this device and should be
sumbitted separately along with an explanation of the intention.

> +
> +/* Channel Definations */
> +#define ADC_VDDOUT					0
> +#define ADC_ICH						1
> +#define ADC_TBAT					2

These any many of the other definitions in your headers are likely
collide with other users - they should be namespaced to avoid this.
--
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/

Powered by blists - more mailing lists