Messages in this thread | | | Date | Fri, 21 May 2010 01:06:22 +0100 | From | Mark Brown <> | 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.
| |