lkml.org 
[lkml]   [2011]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v3 01/11] MFD: DA9052 MFD core module
    Date

    > -----Original Message-----
    > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
    > Sent: Saturday, August 06, 2011 7:09 PM
    > To: Ashish Jangam
    > Cc: sameo@openedhand.com; linux-kernel@vger.kernel.org; Dajun; linaro-
    > dev@lists.linaro.org
    > Subject: Re: [PATCH v3 01/11] MFD: DA9052 MFD core module
    >
    > On Fri, Aug 05, 2011 at 07:23:44PM +0530, ashishj3 wrote:
    >
    > > +choice
    > > + prompt "Chip Type"
    > > + depends on MFD_DA9053_SPI || MFD_DA9053_I2C
    > > +config PMIC_DA9053AA
    > > + bool "Support Dialog Semiconductor DA9053 AA PMIC"
    > > + help
    > > + Support for Dialog semiconductor DA9053 AA PMIC.
    > > + This driver provides common support for accessing the device,
    > > + additional drivers must be enabled in order to use the
    > > + functionality of the device.
    > > +config PMIC_DA9053Bx
    >
    > Could do with blank lines between blocks. Though looking at the code
    > here I don't understand why these are compile options at all, or if they
    > need to be compile options for some reason why they're not independantly
    > selectable?
    DA9052 PMIC chip id may get OTP therefore chip id cannot be used as
    a distinguishing factor. Hence these compile time options were introduced.
    DA9053 is a higher version of DA9052 therefore not independently selectable.
    This means that there can be either DA9052 or DA9053 on system. I need to correct
    this Kconfig to take care of this.
    >
    > > +int da9052_reg_read(struct da9052 *da9052, unsigned char reg)
    > > +{
    > > + int val, ret;
    > > +
    > > + if (reg > DA9052_MAX_REG_CNT) {
    > > + dev_err(da9052->dev, "invalid reg %x\n", reg);
    > > + return -EINVAL;
    > > + }
    > > +
    > > + #ifdef CONFIG_MFD_DA9052_SPI
    > > + reg = (reg << 1) | 1;
    > > + #endif
    >
    > There's several problems here:
    >
    > - You shouldn't be indenting preprocessor directives.
    > - You shouldn't be adding extra indentation before.
    > - This will break I2C devices if SPI support is built into the driver.
    >
    > Please, when writing code try to understand the abstractions you're
    > using. For example here think about the purpose of being able to build
    > I2C and SPI separately and simultaneously and the goal of the regmap
    > API.
    >
    > Looks like we need to add per device mangling for the SPI register
    > read/write flag.
    For now we will handle this as below:-
    During SPI and I2C registration we will add bus type(SPI/I2C) info in the
    struct da9052. And before initiating any device I/O this struct member will
    be read and reg address will be manipulated if needed.
    >
    > > + da9052_group_write(da9052, DA9052_EVENT_A_REG, 4, v);
    > > +
    > > + #ifndef CONFIG_PMIC_DA9053Bx
    > > + DA9052_FIXME();
    > > + #endif
    >
    > This should be runtime detected based on the device name, either from
    > the device registration or by reading back chip identification.
    As said above getting chip info will not work in DA9053/53 case. Also DA9052 code
    works for DA9053 except for few minor changes in MFD and regulator module.
    In this case registering different device will also require a preprocessor directive
    Or separate DA9053 file therefore this option was not opt.
    >



    \
     
     \ /
      Last update: 2011-08-09 10:49    [W:4.140 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site