From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wadim Egorov Subject: Re: [RESEND PATCH v5 1/5] mfd: RK808: Add RK818 support Date: Wed, 6 Jul 2016 10:03:46 +0200 Message-ID: <577CBB62.8080806@phytec.de> References: <1464850228-17244-1-git-send-email-w.egorov@phytec.de> <20160608141735.GE14888@dell> <5759277D.7090101@phytec.de> <577C77DF.9060706@rock-chips.com> Reply-To: rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: Sender: rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org In-Reply-To: <577C77DF.9060706-TNX95d0MmH7DzftRWevZcw@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Andy Yan , Lee Jones Cc: mark.rutland-5wv7dgnIgG8@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org, sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org, broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, zyw-TNX95d0MmH7DzftRWevZcw@public.gmane.org, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-rockchip.vger.kernel.org Hi Andy, On 06.07.2016 05:15, Andy Yan wrote: > Hi Wadim: > > On 2016=E5=B9=B406=E6=9C=8809=E6=97=A5 16:23, Wadim Egorov wrote: >> Hi, >> >> On 08.06.2016 16:17, Lee Jones wrote: >>> On Thu, 02 Jun 2016, Wadim Egorov wrote: >>> >>>> The RK818 chip is a power management IC for multimedia and handheld >>> "Power Management IC (PMIC)" >>> >>>> devices. It contains the following components: >>>> >>>> - Regulators >>>> - RTC >>>> - Clkout >>> Clocking >>> >>>> - battery support >>> Battery support >>> >>>> Both chips RK808 and RK818 are using a similar register map. >>> "Both RK808 ad RK818 chips" >>> >>>> So we can reuse the RTC and Clkout functionality. >>> Swap '.' for ','. >>> >>>> Signed-off-by: Wadim Egorov >>>> --- >>>> drivers/mfd/Kconfig | 4 +- >>>> drivers/mfd/rk808.c | 231 >>>> ++++++++++++++++++++++++++++++++++++++-------- >>>> include/linux/mfd/rk808.h | 162 ++++++++++++++++++++++++++++++-- >>>> 3 files changed, 350 insertions(+), 47 deletions(-) >>>> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>> index 1bcf601..7ba464b 100644 >>>> --- a/drivers/mfd/Kconfig >>>> +++ b/drivers/mfd/Kconfig >>>> @@ -839,13 +839,13 @@ config MFD_RC5T583 >>>> different functionality of the device. >>>> config MFD_RK808 >>>> - tristate "Rockchip RK808 Power Management chip" >>>> + tristate "Rockchip RK808/RK818 Power Management chip" >>> "Chip" >>> >>>> depends on I2C && OF >>>> select MFD_CORE >>>> select REGMAP_I2C >>>> select REGMAP_IRQ >>>> help >>>> - If you say yes here you get support for the RK808 >>>> + If you say yes here you get support for the RK808 and RK818 >>>> Power Management chips. >>>> This driver provides common support for accessing the device >>>> through I2C interface. The device supports multiple >>>> sub-devices >>>> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c >>>> index 49d7f62..3cf9724 100644 >>>> --- a/drivers/mfd/rk808.c >>>> +++ b/drivers/mfd/rk808.c >>>> @@ -1,11 +1,15 @@ >>>> /* >>>> - * MFD core driver for Rockchip RK808 >>>> + * MFD core driver for Rockchip RK808/RK818 >>>> * >>>> * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd >>>> * >>>> * Author: Chris Zhong >>>> * Author: Zhang Qing >>>> * >>>> + * Copyright (C) 2016 PHYTEC Messtechnik GmbH >>>> + * >>>> + * Author: Wadim Egorov >>>> + * >>>> * This program is free software; you can redistribute it and/or >>>> modify it >>>> * under the terms and conditions of the GNU General Public License, >>>> * version 2, as published by the Free Software Foundation. >>>> @@ -22,12 +26,7 @@ >>>> #include >>>> #include >>>> #include >>>> - >>>> -struct rk808_reg_data { >>>> - int addr; >>>> - int mask; >>>> - int value; >>>> -}; >>> Why are you moving this to the header? >> It is now part of the rk808 struct. >> >>>> +#include >>>> static bool rk808_is_volatile_reg(struct device *dev, unsigned >>>> int reg) >>>> { >>>> @@ -57,6 +56,14 @@ static bool rk808_is_volatile_reg(struct device >>>> *dev, unsigned int reg) >>>> return false; >>>> } >>>> +static const struct regmap_config rk818_regmap_config =3D { >>>> + .reg_bits =3D 8, >>>> + .val_bits =3D 8, >>>> + .max_register =3D RK818_USB_CTRL_REG, >>>> + .cache_type =3D REGCACHE_RBTREE, >>>> + .volatile_reg =3D rk808_is_volatile_reg, >>>> +}; >>>> + >>>> static const struct regmap_config rk808_regmap_config =3D { >>>> .reg_bits =3D 8, >>>> .val_bits =3D 8, >>>> @@ -83,7 +90,17 @@ static const struct mfd_cell rk808s[] =3D { >>>> }, >>>> }; >>>> -static const struct rk808_reg_data pre_init_reg[] =3D { >>>> +static const struct mfd_cell rk818s[] =3D { >>>> + { .name =3D "rk808-clkout", }, >>> How does this differ to a normal -clock driver? >> I don't know. It is a normal clock driver. >> >>>> + { .name =3D "rk808-regulator", }, >>>> + { >>>> + .name =3D "rk808-rtc", >>>> + .num_resources =3D ARRAY_SIZE(rtc_resources), >>>> + .resources =3D &rtc_resources[0], >>> .resources =3D rtc_resources, ? >>> >>>> + }, >>>> +}; >>>> + >>>> +static const struct rk8xx_reg_data rk808_pre_init_reg[] =3D { >>>> { RK808_BUCK3_CONFIG_REG, BUCK_ILMIN_MASK, BUCK_ILMIN_150MA }, >>>> { RK808_BUCK4_CONFIG_REG, BUCK_ILMIN_MASK, BUCK_ILMIN_200MA }, >>>> { RK808_BOOST_CONFIG_REG, BOOST_ILMIN_MASK, BOOST_ILMIN_100MA }, >>>> @@ -94,6 +111,22 @@ static const struct rk808_reg_data >>>> pre_init_reg[] =3D { >>>> VB_LO_SEL_3500MV }, >>>> }; >>>> +static const struct rk8xx_reg_data rk818_pre_init_reg[] =3D { >>>> + { RK818_USB_CTRL_REG, RK818_USB_ILIM_SEL_MASK, >>>> + RK818_USB_ILMIN_2000MA }, >>>> + /* close charger when usb lower then 3.4V */ >>>> + { RK818_USB_CTRL_REG, RK818_USB_CHG_SD_VSEL_MASK, (0x7 << >>>> 4) }, >>>> + /* no action when vref */ >>>> + { RK818_H5V_EN_REG, BIT(1), RK818_REF_RDY_CTRL }, >>>> + /* enable HDMI 5V */ >>>> + { RK818_H5V_EN_REG, BIT(0), RK818_H5V_EN }, >>>> + /* improve efficiency */ >>>> + { RK818_BUCK2_CONFIG_REG, BUCK2_RATE_MASK, BUCK_ILMIN_250MA }, >>>> + { RK818_BUCK4_CONFIG_REG, BUCK_ILMIN_MASK, BUCK_ILMIN_250MA }, >>>> + { RK818_BOOST_CONFIG_REG, BOOST_ILMIN_MASK, =20 >>>> BOOST_ILMIN_100MA }, >>>> + { RK808_VB_MON_REG, MASK_ALL, VB_LO_ACT | >>>> VB_LO_SEL_3500MV }, >>>> +}; >>> The alignment here looks odd. >> I will fix it in the next version. >> >>>> static const struct regmap_irq rk808_irqs[] =3D { >>>> /* INT_STS */ >>>> [RK808_IRQ_VOUT_LO] =3D { >>>> @@ -136,6 +169,76 @@ static const struct regmap_irq rk808_irqs[] =3D { >>>> }, >>>> }; >>>> +static const struct regmap_irq rk818_irqs[] =3D { >>>> + /* INT_STS */ >>>> + [RK818_IRQ_VOUT_LO] =3D { >>>> + .mask =3D RK818_IRQ_VOUT_LO_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_VB_LO] =3D { >>>> + .mask =3D RK818_IRQ_VB_LO_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_PWRON] =3D { >>>> + .mask =3D RK818_IRQ_PWRON_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_PWRON_LP] =3D { >>>> + .mask =3D RK818_IRQ_PWRON_LP_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_HOTDIE] =3D { >>>> + .mask =3D RK818_IRQ_HOTDIE_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_RTC_ALARM] =3D { >>>> + .mask =3D RK818_IRQ_RTC_ALARM_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_RTC_PERIOD] =3D { >>>> + .mask =3D RK818_IRQ_RTC_PERIOD_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + [RK818_IRQ_USB_OV] =3D { >>>> + .mask =3D RK818_IRQ_USB_OV_MSK, >>>> + .reg_offset =3D 0, >>>> + }, >>>> + >>>> + /* INT_STS2 */ >>>> + [RK818_IRQ_PLUG_IN] =3D { >>>> + .mask =3D RK818_IRQ_PLUG_IN_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_PLUG_OUT] =3D { >>>> + .mask =3D RK818_IRQ_PLUG_OUT_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_CHG_OK] =3D { >>>> + .mask =3D RK818_IRQ_CHG_OK_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_CHG_TE] =3D { >>>> + .mask =3D RK818_IRQ_CHG_TE_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_CHG_TS1] =3D { >>>> + .mask =3D RK818_IRQ_CHG_TS1_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_TS2] =3D { >>>> + .mask =3D RK818_IRQ_TS2_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_CHG_CVTLIM] =3D { >>>> + .mask =3D RK818_IRQ_CHG_CVTLIM_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> + [RK818_IRQ_DISCHG_ILIM] =3D { >>>> + .mask =3D RK818_IRQ_DISCHG_ILIM_MSK, >>>> + .reg_offset =3D 1, >>>> + }, >>>> +}; >>>> + >>>> static struct regmap_irq_chip rk808_irq_chip =3D { >>>> .name =3D "rk808", >>>> .irqs =3D rk808_irqs, >>>> @@ -148,6 +251,18 @@ static struct regmap_irq_chip rk808_irq_chip =3D = { >>>> .init_ack_masked =3D true, >>>> }; >>>> +static struct regmap_irq_chip rk818_irq_chip =3D { >>>> + .name =3D "rk818", >>>> + .irqs =3D rk818_irqs, >>>> + .num_irqs =3D ARRAY_SIZE(rk818_irqs), >>>> + .num_regs =3D 2, >>>> + .irq_reg_stride =3D 2, >>>> + .status_base =3D RK818_INT_STS_REG1, >>>> + .mask_base =3D RK818_INT_STS_MSK_REG1, >>>> + .ack_base =3D RK818_INT_STS_REG1, >>>> + .init_ack_masked =3D true, >>>> +}; >>>> + >>>> static struct i2c_client *rk808_i2c_client; >>>> static void rk808_device_shutdown(void) >>>> { >>>> @@ -167,6 +282,48 @@ static void rk808_device_shutdown(void) >>>> dev_err(&rk808_i2c_client->dev, "power off error!\n"); >>>> } >>>> +static const struct of_device_id rk808_of_match[] =3D { >>>> + { .compatible =3D "rockchip,rk808", .data =3D (void *) RK808_ID }= , >>>> + { .compatible =3D "rockchip,rk818", .data =3D (void *) RK818_ID }= , >>>> + { }, >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, rk808_of_match); >>>> + >>>> +static int rk8xx_match_device(struct rk808 *rk808, struct device >>>> *dev) >>> If you're going to do it this way, please switch these parameters. >>> >>> It's more common to return the pointer however. >> If it's more common I will return a pointer to rk808. >> >>>> +{ >>>> + const struct of_device_id *of_id; >>>> + >>>> + of_id =3D of_match_device(rk808_of_match, dev); >>>> + if (!of_id) { >>>> + dev_err(dev, "Unable to match OF ID\n"); >>>> + return -ENODEV; >>>> + } >>>> + rk808->variant =3D (long) of_id->data; >>>> + >>>> + switch (rk808->variant) { >>>> + case RK808_ID: >>>> + rk808->nr_cells =3D ARRAY_SIZE(rk808s); >>>> + rk808->cells =3D rk808s; >>>> + rk808->regmap_cfg =3D &rk808_regmap_config; >>>> + rk808->regmap_irq_chip =3D &rk808_irq_chip; >>>> + rk808->pre_init_reg =3D rk808_pre_init_reg; >>>> + rk808->nr_pre_init_regs =3D ARRAY_SIZE(rk808_pre_init_reg); >>>> + break; >>>> + case RK818_ID: >>>> + rk808->nr_cells =3D ARRAY_SIZE(rk818s); >>>> + rk808->cells =3D rk818s; >>>> + rk808->regmap_cfg =3D &rk818_regmap_config; >>>> + rk808->regmap_irq_chip =3D &rk818_irq_chip; >>>> + rk808->pre_init_reg =3D rk818_pre_init_reg; >>>> + rk808->nr_pre_init_regs =3D ARRAY_SIZE(rk818_pre_init_reg); >>>> + break; >>>> + default: >>>> + dev_err(dev, "unsupported RK8XX ID %lu\n", rk808->variant); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> static int rk808_probe(struct i2c_client *client, >>>> const struct i2c_device_id *id) >>>> { >>>> @@ -176,46 +333,52 @@ static int rk808_probe(struct i2c_client >>>> *client, >>>> int ret; >>>> int i; >>>> - if (!client->irq) { >>>> - dev_err(&client->dev, "No interrupt support, no core IRQ\n"); >>>> - return -EINVAL; >>>> - } >>>> - >>>> rk808 =3D devm_kzalloc(&client->dev, sizeof(*rk808), GFP_KERNEL)= ; >>>> if (!rk808) >>>> return -ENOMEM; >>>> - rk808->regmap =3D devm_regmap_init_i2c(client, >>>> &rk808_regmap_config); >>>> + ret =3D rk8xx_match_device(rk808, &client->dev); >>> Is there a way to dynamically probe the device? No device ID you can >>> read directly from the silicon? >> AFAIK there is no device ID register. At least it is not documented in >> the manual. > > register 0x17 and 0x18 is the ID register, 0x17 is the MSB, 0x18 is LSB > > for RK818, register 0x17 is 0x81, 0x18 is 0x81 thank you for sharing this information. I have no RK808 PMIC device here, so I would also need the IDs for RK808. Lee, you have already applied this series. But I can't find the patches in your kernel tree. I would like to read the device ID from the register in the probe function. Do you want me to base my changes on top of this series or send a new version? --=20 You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. ---=20 You received this message because you are subscribed to the Google Groups "= rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout.