From: arnd@arndb•de (Arnd Bergmann)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 1/3] mfd: support 88pm80x in 80x driver
Date: Fri, 29 Jun 2012 13:58:28 +0000 [thread overview]
Message-ID: <201206291358.28788.arnd@arndb.de> (raw)
In-Reply-To: <4FED196D.6090500@marvell.com>
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.
> 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.
> >> 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
next prev parent reply other threads:[~2012-06-29 13:58 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-28 3:13 [PATCH 0/3 V1] add 88pm80x mfd driver Qiao Zhou
2012-06-28 3:13 ` [PATCH 1/3] mfd: support 88pm80x in 80x driver Qiao Zhou
2012-06-28 11:21 ` Arnd Bergmann
2012-06-28 11:46 ` Mark Brown
2012-06-28 14:32 ` Arnd Bergmann
2012-06-29 1:18 ` Haojian Zhuang
2012-06-29 1:29 ` Mark Brown
2012-06-29 14:18 ` Arnd Bergmann
2012-06-29 2:56 ` Qiao Zhou
2012-06-29 13:58 ` Arnd Bergmann [this message]
2012-07-02 7:50 ` Qiao Zhou
2012-07-02 9:22 ` Qiao Zhou
2012-07-02 10:03 ` Mark Brown
2012-07-02 10:09 ` Qiao Zhou
2012-07-02 10:12 ` Mark Brown
2012-07-02 10:15 ` Qiao Zhou
2012-07-02 15:58 ` Arnd Bergmann
2012-07-03 2:28 ` Qiao Zhou
2012-06-29 14:21 ` Arnd Bergmann
2012-06-30 12:02 ` Mark Brown
2012-06-28 3:13 ` [PATCH 2/3] rtc: add rtc support to 88PM80X PMIC Qiao Zhou
2012-06-28 3:13 ` [PATCH 3/3] input: add onkey " Qiao Zhou
-- strict thread matches above, loose matches on Subject: below --
2012-06-13 9:04 [PATCH 0/3 V0] add 88pm80x mfd driver Qiao Zhou
2012-06-13 9:04 ` [PATCH 1/3] mfd: support 88pm80x in 80x driver Qiao Zhou
2012-06-14 12:27 ` Arnd Bergmann
2012-06-14 18:43 ` Mark Brown
2012-06-15 5:23 ` Qiao Zhou
2012-06-15 3:17 ` Qiao Zhou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201206291358.28788.arnd@arndb.de \
--to=arnd@arndb$(echo .)de \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox