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

On 07/02/2012 03:50 PM, Qiao Zhou wrote:

On 06/29/2012 09:58 PM, Arnd Bergmann wrote:

quoted

On Friday 29 June 2012, Qiao Zhou wrote:

quoted

On 06/28/2012 07:21 PM, Arnd Bergmann wrote:

quoted

quoted

quoted

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.

quoted

quoted

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!

quoted

quoted

quoted

quoted

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