From mboxrd@z Thu Jan 1 00:00:00 1970 From: Carlo Caione Subject: Re: Re: [PATCH v3 01/10] mfd: AXP20x: Add mfd driver for AXP20x PMIC Date: Sat, 29 Mar 2014 16:53:51 +0100 Message-ID: <20140329155351.GB3952@localhost.fastwebnet.it> References: <1395955764-18103-1-git-send-email-carlo@caione.org> <1395955764-18103-2-git-send-email-carlo@caione.org> <20140328092154.GF17779@lee--X1> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20140328092154.GF17779@lee--X1> List-Post: , List-Help: , List-Archive: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Subscribe: , List-Unsubscribe: , Content-Disposition: inline To: Lee Jones Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org, hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org, wens-jdAy2FN1RRM@public.gmane.org, sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org List-Id: linux-input@vger.kernel.org On Fri, Mar 28, 2014 at 09:21:54AM +0000, Lee Jones wrote: > > This patch introduces the preliminary support for PMICs X-Powers AXP202 > > and AXP209. The AXP209 and AXP202 are the PMUs (Power Management Unit) > > used by A10, A13 and A20 SoCs and developed by X-Powers, a sister company > > of Allwinner. > > > > The core enables support for two subsystems: > > - PEK (Power Enable Key) > > - Regulators > > > > Signed-off-by: Carlo Caione > > --- > > drivers/mfd/Kconfig | 12 +++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/axp20x.c | 240 +++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/axp20x.h | 180 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 433 insertions(+) > > create mode 100644 drivers/mfd/axp20x.c > > create mode 100644 include/linux/mfd/axp20x.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 49bb445..24ba61a 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -59,6 +59,18 @@ config MFD_AAT2870_CORE > > additional drivers must be enabled in order to use the > > functionality of the device. > > > > +config MFD_AXP20X > > + bool "X-Powers AXP20X" > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + depends on I2C=y > > + help > > + If you say Y here you get support for the AXP20X. > > + This driver provides common support for accessing the device, > > + additional drivers must be enabled in order to use the > > + functionality of the device. > > Please tell us what this device is and what sub-devices are available? Ok > [...] > > > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c > > new file mode 100644 > > index 0000000..f77a663 > > --- /dev/null > > +++ b/drivers/mfd/axp20x.c > > @@ -0,0 +1,240 @@ > > +/* > > + * axp20x.c - MFD core driver for the X-Powers AXP202 and AXP209 > > ... which consist of ... Yeah, I should improve my code documentation :) > [...] > > > +static struct resource axp20x_pek_resources[] = { > > + { > > + .name = "PEK_DBR", > > + .start = AXP20X_IRQ_PEK_RIS_EDGE, > > + .end = AXP20X_IRQ_PEK_RIS_EDGE, > > + .flags = IORESOURCE_IRQ, > > + }, { > > + .name = "PEK_DBF", > > + .start = AXP20X_IRQ_PEK_FAL_EDGE, > > + .end = AXP20X_IRQ_PEK_FAL_EDGE, > > + .flags = IORESOURCE_IRQ, > > + }, > > +}; > > Have you considered doing this in the Device Tree? It's a lot less > code/overhead. Ok, I'll change it in v4. > > +static const struct i2c_device_id axp20x_i2c_id[] = { > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id); > > We really should consider changing the I2C subsystem! > > Can you add a comment here describing why we have to add this > seemingly pointless empty struct please? Sure. > > +static struct mfd_cell axp20x_cells[] = { > > + { > > + .name = "axp20x-pek", > > + .num_resources = ARRAY_SIZE(axp20x_pek_resources), > > + .resources = axp20x_pek_resources, > > + }, { > > + .name = "axp20x-regulator", > > + }, > > +}; > > Do these drivers don't look inside the DTB at all? Only the regulators driver. > > + of_id = of_match_device(axp20x_of_match, &i2c->dev); > > + if (!of_id) { > > + dev_err(&i2c->dev, "Unable to setup AXP20X data\n"); > > + return -ENODEV; > > + } > > + axp20x->variant = (int) of_id->data; > > 'variant' needs to be a (unsigned?) long or it will break on 64bit > architectures. Ok, I really need to start thinking to 64bit also. Thank you, -- Carlo Caione