[PATCH 1/3] mfd: support 88pm80x in 80x driver

[Qiao Zhou]

On 07/02/2012 03:50 PM, Qiao Zhou wrote:
> On 06/29/2012 09:58 PM, Arnd Bergmann wrote:
>> On Friday 29 June 2012, Qiao Zhou wrote:
>>> On 06/28/2012 07:21 PM, Arnd Bergmann wrote:
>>
>>>>> All I2C operations are accessed by 80x-i2c driver, and register
>>>>> access is via
>>>>> regmap interface.
>>>>>
>>>>> The benefit is that client drivers only need one kind of read/write
>>>>> API. I2C
>>>>> and MFD driver can be shared in both 800 and 805.
>>>>
>>>> I'm not sure what the purpose of this split is. Usually you only
>>>> separate out
>>>> the i2c parts if the same driver has multiple host-side interfaces,
>>>> e.g. i2c
>>>> and spi. If your driver only has one of them, it's easier to just
>>>> have one
>>>> file. Using regmap however seems to be a good idea nonetheless.
>>>>
>>>> I would also suggest you consider splitting the driver into three
>>>> separate
>>>> parts after moving the i2c file into the core file:
>>>>
>>>> a) one "library" that contains the common code and exports symbols
>>>> b) one driver for 805 that contains just the 805 specific parts and
>>>>      registers the i2c driver for 805
>>>> c) one driver for 800 that contains just the 800 specific parts and
>>>>      registers the i2c driver for 800
>>>>
>>>> Does that sound reasonable?
>>>>
>>>> There is very little common that is actually part of the 88pm80x_common
>>>> file, everything else is specific to just one of the variants. The
>>>> only common
>>>> parts I found are device_irq_init_80x and device_irq_exit_80x, and
>>>> even for those I'm not sure if it wouldn't be easier to just
>>>> have separate functions.
>>>>
>>>> The common parts seem to be mostly in your current _i2c.c file, so
>>>> that could
>>>> instead become the common file when you split 805 from 800. You
>>>> basically
>>>> just export the pm_ops and the probe/remove functions and let that
>>>> be called
>>>> from the individual drivers.
>>>
>>> Arnd, the comments is inaccurate for I2C operations. actually all I2C
>>> operations are handled by regmap in the new driver. and no more API
>>> needs to be exported by 88pm80x-i2c driver. driver which needs to access
>>> registers only uses uses regmap handle via dev_get_regmap, only i2c
>>> client needed. actually there is no split in I2C operations. please let
>>> me know if I miss your meaning.
>>
>> The point was something else: The regmap always uses i2c behind the
>> covers,
>> unlike drivers that have a common file using regmap to abstract the
>> interface so that they can be used with either i2c or spi.
> Arnd, understand your meaning. driver just exports the API while callers
> doesn't need to know whether it's via i2c or spi in lower driver. It
> makes sense. thanks!
>
> Here I have a question about the implementation specifically with
> 88pm800. this chip has 3 register pages, and 3 i2c address corresponding
> to each page. so it requires an additional parameter to point out the
> page(i2c) to be accessed. currently I didn't think of a good API to
> export for such purpose. seems to me, the 88pm800 chip is already bound
> to i2c interface, and using regmap directly is a better solution. could
> you give some suggestions?
is it OK to export another two groups of r/w interface for gpadc and 
power page separately in 88pm800? such as pm800_gpadc_read_reg() / 
pm800_power_read_reg() etc?  Appreciate any comments.
>>
>>> in the 88pm80x_core.c, actually it only has five functions,
>>> irq_init_80x, irq_exit_80x, dev_init_800, dev_init_805, dev_exit_80x.
>>> there is indeed few common part. so I assume your meaning is that:
>>> creating 88pm800_core.c and 88pm805_core.c separtely, which implement
>>> irq_init, irq_exit, dev_init, dev_exit for its own chip. is that
>>> correct?
>>
>> Right. However, I would go further and swap the layering of the driver
>> parts: Right now, a common pm08x_probe from the common i2c_driver
>> structure
>> calls the specific device_800_init and device_805_init through
>> a global pm80x_device_init. When you do the split, I would move the
>> i2c_driver structure to each of the two 88pm80?_core.c files and have
>> an individual probe function in them that calls the common pm08x_probe
>> and then its own device_init.
>>
>> This is the layering that most other subsystems use when you have drivers
>> sharing some common code.
> would update according to suggestion. thanks!
>>
>>>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>>>> new file mode 100644
>>>>> index 0000000..9e2e11d
>>>>> --- /dev/null
>>>>
>>>> A lot of the contents of this file should not really be globally
>>>> visible.
>>>> Most of the register definitions are just used by the common mfd
>>>> driver,
>>>> so I would put them in a header file in the mfd directory or (even
>>>> better)
>>>> just into the .c file that uses them.
>>> yes, would category these registers. some registers which is used by
>>> regulator/rtc/onkey/codec/headset det/MISC would be kept in this header
>>> file, while others would be removed from globally visibility.
>>
>> Ok, sounds good.
>>
>>     Arnd
>>
> Arnd, thanks for your suggestions!
>


-- 

Best Regards
Qiao