From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751721AbcFYALm (ORCPT ); Fri, 24 Jun 2016 20:11:42 -0400 Received: from megous.com ([83.167.254.221]:41029 "EHLO xff.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751305AbcFYALl (ORCPT ); Fri, 24 Jun 2016 20:11:41 -0400 Subject: Re: [PATCH 07/14] regulator: SY8106A regulator driver To: Chen-Yu Tsai References: <20160623192104.18720-1-megous@megous.com> <20160623192104.18720-8-megous@megous.com> Cc: dev , Mark Brown , Liam Girdwood , linux-arm-kernel , open list From: =?UTF-8?Q?Ond=c5=99ej_Jirman?= Message-ID: Date: Sat, 25 Jun 2016 02:11:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BoUmV5JxMj1a4TqdJAi2Fn19R4utTPakd" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --BoUmV5JxMj1a4TqdJAi2Fn19R4utTPakd Content-Type: multipart/mixed; boundary="ia1itV6tCdA99xWiuFRhKwWjHMsp23od7" From: =?UTF-8?Q?Ond=c5=99ej_Jirman?= To: Chen-Yu Tsai Cc: dev , Mark Brown , Liam Girdwood , linux-arm-kernel , open list Message-ID: Subject: Re: [PATCH 07/14] regulator: SY8106A regulator driver References: <20160623192104.18720-1-megous@megous.com> <20160623192104.18720-8-megous@megous.com> In-Reply-To: --ia1itV6tCdA99xWiuFRhKwWjHMsp23od7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi, thank you for the review. I've resolved most of the issues. Some more comments below. On 24.6.2016 05:41, Chen-Yu Tsai wrote: > On Fri, Jun 24, 2016 at 3:20 AM, wrote: >> From: Ondrej Jirman >> >> SY8106A is I2C attached single output voltage regulator >> made by Silergy. >> >> Signed-off-by: Ondrej Jirman >> --- >> drivers/regulator/Kconfig | 8 +- >> drivers/regulator/Makefile | 2 +- >> drivers/regulator/sy8106a-regulator.c | 153 +++++++++++++++++++++++++= +++++++++ >> 3 files changed, 161 insertions(+), 2 deletions(-) >> create mode 100644 drivers/regulator/sy8106a-regulator.c >> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index 144cbf5..fc3fae2 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -860,5 +860,11 @@ config REGULATOR_WM8994 >> This driver provides support for the voltage regulators on t= he >> WM8994 CODEC. >> >> -endif >> +config REGULATOR_SY8106A >> + tristate "Silergy SY8106A" >> + depends on I2C >=20 > Maybe you should also depend on OF since the driver is going to cripple= d > without any constraints set, or (OF || COMPILE_TEST) if you want some > compile test coverage. >=20 >> + select REGMAP_I2C >> + help >> + This driver provides support for the voltage regulator SY810= 6A. >> >> +endif >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile >> index 85a1d44..f382095 100644 >> --- a/drivers/regulator/Makefile >> +++ b/drivers/regulator/Makefile >> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) +=3D wm831x-ldo.o >> obj-$(CONFIG_REGULATOR_WM8350) +=3D wm8350-regulator.o >> obj-$(CONFIG_REGULATOR_WM8400) +=3D wm8400-regulator.o >> obj-$(CONFIG_REGULATOR_WM8994) +=3D wm8994-regulator.o >> - >> +obj-$(CONFIG_REGULATOR_SY8106A) +=3D sy8106a-regulator.o >=20 > Follow the existing ordering in the Makefile. >=20 >> >> ccflags-$(CONFIG_REGULATOR_DEBUG) +=3D -DDEBUG >> diff --git a/drivers/regulator/sy8106a-regulator.c b/drivers/regulator= /sy8106a-regulator.c >> new file mode 100644 >> index 0000000..34bd69c >> --- /dev/null >> +++ b/drivers/regulator/sy8106a-regulator.c >> @@ -0,0 +1,153 @@ >> +/* >> + * sy8106a-regulator.c - Regulator device driver for SY8106A >> + * >> + * Copyright (C) 2016 Ond=C5=99ej Jirman >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Library General Public >> + * License as published by the Free Software Foundation; either >> + * version 2 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Library General Public License for more details. >> + * >> + * You should have received a copy of the GNU Library General Public >> + * License along with this library; if not, write to the >> + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, >> + * Boston, MA 02110-1301, USA. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >=20 > Do you need this one? >=20 >> +#include >> +#include >=20 > And this one? >=20 >> +#include >> +#include >=20 > Sort alphabetically please. >=20 >> + >> +#define SY8106A_REG_VOUT1_SEL 0x01 >> +#define SY8106A_REG_VOUT_COM 0x02 >> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f >> +#define SY8106A_DISABLE_REG 0x01 >=20 > BIT(0) would be clearer. >=20 >> + >> +struct sy8106a { >> + struct regulator_dev *rdev; >> + struct regmap *regmap; >> +}; >> + >> +static const struct regmap_config sy8106a_regmap_config =3D { >> + .reg_bits =3D 8, >> + .val_bits =3D 8, >> +}; >> + >> +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsign= ed sel) >> +{ >> + return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg, >> + 0xff, sel | 0x80); >=20 > Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regm= ap? I understand the code savings, but I'd rather avoid that, because it would involve 2 needless readouts and rewrites of the voltage setting register. I'd rather change this to regmap_write, so that the regulator only receives writes over I2C, to minimize possibility of bit flip error by minimizing the communication over the I2C bus. >> +} >> + >> +static const struct regulator_ops sy8106a_ops =3D { >> + .is_enabled =3D regulator_is_enabled_regmap, >> + .set_voltage_sel =3D sy8106a_set_voltage_sel, >> + .set_voltage_time_sel =3D regulator_set_voltage_time_sel, >> + .get_voltage_sel =3D regulator_get_voltage_sel_regmap, >> + .list_voltage =3D regulator_list_voltage_linear, >> +}; >> + >> +/* Default limits measured in millivolts and milliamps */ >> +#define SY8106A_MIN_MV 680 >> +#define SY8106A_MAX_MV 1950 >> +#define SY8106A_STEP_MV 10 >> + >> +static const struct regulator_desc sy8106a_reg =3D { >> + .name =3D "SY8106A", >> + .id =3D 0, >> + .ops =3D &sy8106a_ops, >> + .type =3D REGULATOR_VOLTAGE, >> + .n_voltages =3D ((SY8106A_MAX_MV - SY8106A_MIN_MV) / SY8106A_S= TEP_MV) + 1, >> + .min_uV =3D (SY8106A_MIN_MV * 1000), >> + .uV_step =3D (SY8106A_STEP_MV * 1000), >> + .vsel_reg =3D SY8106A_REG_VOUT1_SEL, >> + .vsel_mask =3D SY8106A_REG_VOUT1_SEL_MASK, >> + .enable_reg =3D SY8106A_REG_VOUT_COM, >> + .enable_mask =3D SY8106A_DISABLE_REG, >> + .disable_val =3D SY8106A_DISABLE_REG, >> + .enable_is_inverted =3D 1, >> + .owner =3D THIS_MODULE, >> +}; >> + >> +/* >> + * I2C driver interface functions >> + */ >> +static int sy8106a_i2c_probe(struct i2c_client *i2c, >> + const struct i2c_device_id *id) >> +{ >> + struct sy8106a *chip; >> + struct device *dev =3D &i2c->dev; >> + struct regulator_dev *rdev =3D NULL; >> + struct regulator_config config =3D { }; >> + unsigned int selector; >> + int error; >> + >> + chip =3D devm_kzalloc(&i2c->dev, sizeof(struct sy8106a), GFP_K= ERNEL); >> + if (!chip) >> + return -ENOMEM; >> + >> + chip->regmap =3D devm_regmap_init_i2c(i2c, &sy8106a_regmap_con= fig); >> + if (IS_ERR(chip->regmap)) { >> + error =3D PTR_ERR(chip->regmap); >> + dev_err(&i2c->dev, "Failed to allocate register map: %= d\n", >> + error); >> + return error; >> + } >> + >> + config.dev =3D &i2c->dev; >> + config.init_data =3D of_get_regulator_init_data(dev, dev->of_n= ode, &sy8106a_reg); >=20 > Check for possible failures? >=20 >> + config.driver_data =3D chip; >> + config.regmap =3D chip->regmap; >> + config.of_node =3D dev->of_node; >> + >> + /* Probe regulator */ >> + error =3D regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &se= lector); >> + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106= A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK)); >> + if (error) { >> + dev_err(&i2c->dev, "Failed to read voltage at probe ti= me: %d\n", error); >> + return error; >> + } >> + >> + rdev =3D devm_regulator_register(&i2c->dev, &sy8106a_reg, &con= fig); >> + if (IS_ERR(rdev)) { >> + dev_err(&i2c->dev, "Failed to register SY8106A regulat= or\n"); >> + return PTR_ERR(rdev); >> + } >> + >> + chip->rdev =3D rdev; >> + >> + i2c_set_clientdata(i2c, chip); >> + >> + return 0; >> +} >> + >> +static const struct i2c_device_id sy8106a_i2c_id[] =3D { >> + {"sy8106a", 0}, >> + {}, >> +}; >> + >=20 > Extra line. >=20 >> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id); >=20 > IMHO probing explicitly through DT (of_device_id) would be better. I checked out a few recently added i2c regulator drivers, and this is the way they're written. I'd rather not differ, especially since I'm a beginner with this thing, atm. Also, I'm not sure what chanhge in particular you're suggesting. thanks, Ondrej >=20 > Regards > ChenYu >=20 >> + >> +static struct i2c_driver sy8106a_regulator_driver =3D { >> + .driver =3D { >> + .name =3D "sy8106a", >> + }, >> + .probe =3D sy8106a_i2c_probe, >> + .id_table =3D sy8106a_i2c_id, >> +}; >> + >> +module_i2c_driver(sy8106a_regulator_driver); >> + >> +MODULE_AUTHOR("Ond=C5=99ej Jirman "); >> +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.9.0 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --ia1itV6tCdA99xWiuFRhKwWjHMsp23od7-- --BoUmV5JxMj1a4TqdJAi2Fn19R4utTPakd Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXbcwyAAoJEG5kJsZ3z+/xwYAP/RWHD4CEUZ60U+/U/YWvEyQb eN3Y07Ku8SY9V0bgyrB3MoDo5WF88cHuXHWGDT9wDj9eLMCWo1L3pdoll79gxcwj 5BDOT1HzCxEut6Mr5dNUTDaHqTsbbWnFQYY07+i9umOvX6O3kg/yEjpBXMA5NuEw I6Rfu8Sb1cS5OHM0jEICowGRTBKmS/6A98nZYjVi5i4zgEZygdjIQHfV2oh7Qn+t NPrTpylSPYOQwJlLm1AoyzrgvmyIJJ0/VGkec0BRf1CwUz8oykk+DYrZGBc0TxLr Td51x98H3mjgJXN8D62VfNWoYiw4GIr4dkyRkYblfConnUdxdtnyjw2SfX06F8O/ li0xJECgvhB9j8Y0pH0Ai+7Smjai4DmuuC4HY+oCyqAfHDopviwV32WZ384rVpKM jEjbuaLqEkRA3QSvY68355ny9M/Ol5TBPpx9LgrWkFFihdirJ+W3kbEE9ErixASz SMuJIyfuwUK45ZLX6L/krNRjikDftJqBd4alu0so/XWn0E1A8XvDYw6vDLIAgIDn TsvJk9NbFvtZoNxeDwvPsxu1leIA7cIPjH6DRA3TsAxUBeWE5OLeW7Uc2PG0jn/Z 38Gjs/A8Znsl3TsBGhmDEPvHX2eeG3uBPx1QcCNTjlEWxbSNHM1ep3se7xkEABfX Rp8lvcHHaC0FSiVacxHA =VU++ -----END PGP SIGNATURE----- --BoUmV5JxMj1a4TqdJAi2Fn19R4utTPakd--