[linux-sunxi] Re: [PATCH 07/14] regulator: SY8106A regulator driver

2016-06-24 Thread Chen-Yu Tsai 
On Sat, Jun 25, 2016 at 8:11 AM, Ondřej Jirman  wrote:
> 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 the
>>>   WM8994 CODEC.
>>>
>>> -endif
>>> +config REGULATOR_SY8106A
>>> +   tristate "Silergy SY8106A"
>>> +   depends on I2C
>>
>> Maybe you should also depend on OF since the driver is going to crippled
>> without any constraints set, or (OF || COMPILE_TEST) if you want some
>> compile test coverage.
>>
>>> +   select REGMAP_I2C
>>> +   help
>>> + This driver provides support for the voltage regulator SY8106A.
>>>
>>> +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) += wm831x-ldo.o
>>>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>>>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>>>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>>> -
>>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
>>
>> Follow the existing ordering in the Makefile.
>>
>>>
>>>  ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
>>> diff --git a/drivers/regulator/sy8106a-regulator.c 
>>> b/drivers/regulator/sy8106a-regulator.c
>>> new file mode 100644
>>> index 000..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řej 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 
>>
>> Do you need this one?
>>
>>> +#include 
>>> +#include 
>>
>> And this one?
>>
>>> +#include 
>>> +#include 
>>
>> Sort alphabetically please.
>>
>>> +
>>> +#define SY8106A_REG_VOUT1_SEL  0x01
>>> +#define SY8106A_REG_VOUT_COM   0x02
>>> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
>>> +#define SY8106A_DISABLE_REG0x01
>>
>> BIT(0) would be clearer.
>>
>>> +
>>> +struct sy8106a {
>>> +   struct regulator_dev *rdev;
>>> +   struct regmap *regmap;
>>> +};
>>> +
>>> +static const struct regmap_config sy8106a_regmap_config = {
>>> +   .reg_bits = 8,
>>> +   .val_bits = 8,
>>> +};
>>> +
>>> +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned 
>>> sel)
>>> +{
>>> +   return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>>> + 0xff, sel | 0x80);
>>
>> Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?
>
> 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.

OK. It's best to add a comment then, in case someone comes in and refactors it.

>
>>> +}
>>> +
>>> +static const struct regulator_ops sy8106a_ops = {
>>> +   .is_enabled = regulator_is_enabled_regmap,
>>> +   .set_voltage_sel = sy8106a_set_voltage_sel,
>>> +

[linux-sunxi] Re: [PATCH 07/14] regulator: SY8106A regulator driver

2016-06-24 Thread Ondřej Jirman 
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 the
>>   WM8994 CODEC.
>>
>> -endif
>> +config REGULATOR_SY8106A
>> +   tristate "Silergy SY8106A"
>> +   depends on I2C
> 
> Maybe you should also depend on OF since the driver is going to crippled
> without any constraints set, or (OF || COMPILE_TEST) if you want some
> compile test coverage.
> 
>> +   select REGMAP_I2C
>> +   help
>> + This driver provides support for the voltage regulator SY8106A.
>>
>> +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) += wm831x-ldo.o
>>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>> -
>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
> 
> Follow the existing ordering in the Makefile.
> 
>>
>>  ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
>> diff --git a/drivers/regulator/sy8106a-regulator.c 
>> b/drivers/regulator/sy8106a-regulator.c
>> new file mode 100644
>> index 000..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řej 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 
> 
> Do you need this one?
> 
>> +#include 
>> +#include 
> 
> And this one?
> 
>> +#include 
>> +#include 
> 
> Sort alphabetically please.
> 
>> +
>> +#define SY8106A_REG_VOUT1_SEL  0x01
>> +#define SY8106A_REG_VOUT_COM   0x02
>> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
>> +#define SY8106A_DISABLE_REG0x01
> 
> BIT(0) would be clearer.
> 
>> +
>> +struct sy8106a {
>> +   struct regulator_dev *rdev;
>> +   struct regmap *regmap;
>> +};
>> +
>> +static const struct regmap_config sy8106a_regmap_config = {
>> +   .reg_bits = 8,
>> +   .val_bits = 8,
>> +};
>> +
>> +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
>> +{
>> +   return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>> + 0xff, sel | 0x80);
> 
> Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?

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 = {
>> +   .is_enabled = regulator_is_enabled_regmap,
>> +   .set_voltage_sel = sy8106a_set_voltage_sel,
>> +   .set_voltage_time_sel = regulator_set_voltage_time_sel,
>> +   .get_voltage_sel = regulator_get_voltage_sel_regmap,
>> +   .list_voltage = regulator_list_voltage_linear,
>> +};
>> +
>> +/* Default limits measured in millivolts and milliamps */
>> +#define SY8106A_MIN_MV 680
>>

[linux-sunxi] Re: [PATCH 07/14] regulator: SY8106A regulator driver

2016-06-23 Thread Chen-Yu Tsai 
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 the
>   WM8994 CODEC.
>
> -endif
> +config REGULATOR_SY8106A
> +   tristate "Silergy SY8106A"
> +   depends on I2C

Maybe you should also depend on OF since the driver is going to crippled
without any constraints set, or (OF || COMPILE_TEST) if you want some
compile test coverage.

> +   select REGMAP_I2C
> +   help
> + This driver provides support for the voltage regulator SY8106A.
>
> +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) += wm831x-ldo.o
>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> -
> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o

Follow the existing ordering in the Makefile.

>
>  ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
> diff --git a/drivers/regulator/sy8106a-regulator.c 
> b/drivers/regulator/sy8106a-regulator.c
> new file mode 100644
> index 000..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řej 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 

Do you need this one?

> +#include 
> +#include 

And this one?

> +#include 
> +#include 

Sort alphabetically please.

> +
> +#define SY8106A_REG_VOUT1_SEL  0x01
> +#define SY8106A_REG_VOUT_COM   0x02
> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
> +#define SY8106A_DISABLE_REG0x01

BIT(0) would be clearer.

> +
> +struct sy8106a {
> +   struct regulator_dev *rdev;
> +   struct regmap *regmap;
> +};
> +
> +static const struct regmap_config sy8106a_regmap_config = {
> +   .reg_bits = 8,
> +   .val_bits = 8,
> +};
> +
> +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
> +{
> +   return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
> + 0xff, sel | 0x80);

Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?

> +}
> +
> +static const struct regulator_ops sy8106a_ops = {
> +   .is_enabled = regulator_is_enabled_regmap,
> +   .set_voltage_sel = sy8106a_set_voltage_sel,
> +   .set_voltage_time_sel = regulator_set_voltage_time_sel,
> +   .get_voltage_sel = regulator_get_voltage_sel_regmap,
> +   .list_voltage = regulator_list_voltage_linear,
> +};
> +
> +/* Default limits measured in millivolts and milliamps */
> +#define SY8106A_MIN_MV 680
> +#define SY8106A_MAX_MV 1950
> +#define SY8106A_STEP_MV10
> +
> +static const struct regulator_desc sy8106a_reg = {
> +   .name = "SY8106A",
> +   .id = 0,
> +   .ops = _ops,
> +   .type = REGULATOR_VOLTAGE,
> +   .n_voltages = ((SY8106A_MAX_MV - SY8106A_MIN_MV) / SY8106A_STEP_MV) + 
> 1,
> +   .min_uV = (SY8106A_MIN_MV * 1000),
> +   .uV_step = (SY8106A_STEP_MV * 1000),
> +   .vsel_reg = SY8106A_REG_VOUT1_SEL,
> +   .vsel_mask = SY8106A_REG_VOUT1_SEL_MASK,
> +   .enable_reg = SY8106A_REG_VOUT_COM,
> +   .enable_mask = SY8106A_DISABLE_REG,
> +