Re: [RFC PATCH v2 4/6] mfd: add BD71827 header

2020-12-17 Thread Vaittinen, Matti

On Wed, 2020-12-16 at 08:53 +, Lee Jones wrote:
> On Fri, 04 Dec 2020, Matti Vaittinen wrote:
> 
> > Add BD71827 driver header. For a record - Header is originally
> > based on work authored by Cong Pham although not much of original
> > work is left now.
> > 
> > Signed-off-by: Matti Vaittinen 
> > ---
> > This patch was not in v1. This brings in some charger registers
> > for the BD71827 charger which is in following patches.
> > 
> >  include/linux/mfd/rohm-bd71827.h | 295
> > +++
> >  1 file changed, 295 insertions(+)
> >  create mode 100644 include/linux/mfd/rohm-bd71827.h
> > 
> > diff --git a/include/linux/mfd/rohm-bd71827.h
> > b/include/linux/mfd/rohm-bd71827.h
> > new file mode 100644
> > index ..0f5a343b10ae
> > --- /dev/null
> > +++ b/include/linux/mfd/rohm-bd71827.h
> > @@ -0,0 +1,295 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright 2016
> 
> Out of date.

...

Thanks for the review Lee! It was unexpected as I didn't add you to CC.
Reason for skipping you is that this was very initial RFC patch set -
intended to collect opinions of adding some fuel-gauge logic in power-
supply. I will for sure clean up the findings (and possibly some other
issues) when I proceed with these charger drivers - and at that point I
will ask for a re-review & add you in CC. But I just wanted to point
out that as the fate/format of this driver depends on how the RFC is
seen by Sebastian & others so getting back to this may take some
time...

BTW - you have nice mail-filters as you caught these pathces, you pick
every mail with MFD in subject(?) 


Best Regards
Matti

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)



Re: [RFC PATCH v2 4/6] mfd: add BD71827 header

2020-12-16 Thread Lee Jones
On Fri, 04 Dec 2020, Matti Vaittinen wrote:

> Add BD71827 driver header. For a record - Header is originally
> based on work authored by Cong Pham although not much of original
> work is left now.
> 
> Signed-off-by: Matti Vaittinen 
> ---
> This patch was not in v1. This brings in some charger registers
> for the BD71827 charger which is in following patches.
> 
>  include/linux/mfd/rohm-bd71827.h | 295 +++
>  1 file changed, 295 insertions(+)
>  create mode 100644 include/linux/mfd/rohm-bd71827.h
> 
> diff --git a/include/linux/mfd/rohm-bd71827.h 
> b/include/linux/mfd/rohm-bd71827.h
> new file mode 100644
> index ..0f5a343b10ae
> --- /dev/null
> +++ b/include/linux/mfd/rohm-bd71827.h
> @@ -0,0 +1,295 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright 2016

Out of date.

> + * @author cpham2...@gmail.com

This is not a proper Author: tag.

> + */
> +
> +#ifndef __LINUX_MFD_BD71827_H
> +#define __LINUX_MFD_BD71827_H

You can drop the LINUX part.

> +#include 
> +
> +#define LDO5VSEL_EQ_H0
> +
> +#ifndef LDO5VSEL_EQ_H
> + #error define LDO5VSEL_EQ_H to 1 when connect to High, to 0 when 
> connect to Low
> +#else
> + #if LDO5VSEL_EQ_H == 1
> + #define BD71827_REG_LDO5_VOLT   BD71827_REG_LDO5_VOLT_H
> + #define LDO5_MASK   LDO5_H_MASK
> + #elif LDO5VSEL_EQ_H == 0
> + #define BD71827_REG_LDO5_VOLT   BD71827_REG_LDO5_VOLT_L
> + #define LDO5_MASK   LDO5_L_MASK
> + #else
> + #error Define LDO5VSEL_EQ_H only to 0 or 1
> + #endif
> +#endif

Not a fan of this at all.

What decides which of these to use?

Does this realistically ever change?

> +enum {
> + BD71827_BUCK1   =   0,

Omit the extra ' 's.

> + BD71827_BUCK2,
> + BD71827_BUCK3,
> + BD71827_BUCK4,
> + BD71827_BUCK5,
> + // General Purpose

No C++ comments.

> + BD71827_LDO1,
> + BD71827_LDO2,
> + BD71827_LDO3,
> + BD71827_LDO4,
> + BD71827_LDO5,
> + BD71827_LDO6,
> + // LDO for Secure Non-Volatile Storage
> + BD71827_LDOSNVS,
> + BD71827_REGULATOR_CNT,
> +};
> +
> +#define BD71827_SUPPLY_STATE_ENABLED 0x1
> +
> +#define BD71827_BUCK1_VOLTAGE_NUM0x3F
> +#define BD71827_BUCK2_VOLTAGE_NUM0x3F
> +#define BD71827_BUCK3_VOLTAGE_NUM0x3F
> +#define BD71827_BUCK4_VOLTAGE_NUM0x1F
> +#define BD71827_BUCK5_VOLTAGE_NUM0x1F
> +#define BD71827_LDO1_VOLTAGE_NUM 0x3F
> +#define BD71827_LDO2_VOLTAGE_NUM 0x3F
> +#define BD71827_LDO3_VOLTAGE_NUM 0x3F
> +#define BD71827_LDO4_VOLTAGE_NUM 0x3F
> +#define BD71827_LDO5_VOLTAGE_NUM 0x3F
> +#define BD71827_LDO6_VOLTAGE_NUM 0x1
> +#define BD71827_LDOSNVS_VOLTAGE_NUM  0x1

Please pad out these to there are the same amount of digits after the 0x.

> +#define BD71827_GPIO_NUM 2   /* BD71827 have 2 GPO */

"has 2 GPOs"

> +enum {
> + BD71827_REG_DEVICE  = 0x00,
> + BD71827_REG_PWRCTRL = 0x01,
> + BD71827_REG_BUCK1_MODE  = 0x02,
> + BD71827_REG_BUCK2_MODE  = 0x03,
> + BD71827_REG_BUCK3_MODE  = 0x04,
> + BD71827_REG_BUCK4_MODE  = 0x05,
> + BD71827_REG_BUCK5_MODE  = 0x06,
> + BD71827_REG_BUCK1_VOLT_RUN  = 0x07,
> + BD71827_REG_BUCK1_VOLT_SUSP = 0x08,
> + BD71827_REG_BUCK2_VOLT_RUN  = 0x09,
> + BD71827_REG_BUCK2_VOLT_SUSP = 0x0A,
> + BD71827_REG_BUCK3_VOLT  = 0x0B,
> + BD71827_REG_BUCK4_VOLT  = 0x0C,
> + BD71827_REG_BUCK5_VOLT  = 0x0D,
> + BD71827_REG_LED_CTRL= 0x0E,
> + BD71827_REG_reserved_0F = 0x0F,
> + BD71827_REG_LDO_MODE1   = 0x10,
> + BD71827_REG_LDO_MODE2   = 0x11,
> + BD71827_REG_LDO_MODE3   = 0x12,
> + BD71827_REG_LDO_MODE4   = 0x13,
> + BD71827_REG_LDO1_VOLT   = 0x14,
> + BD71827_REG_LDO2_VOLT   = 0x15,
> + BD71827_REG_LDO3_VOLT   = 0x16,
> + BD71827_REG_LDO4_VOLT   = 0x17,
> + BD71827_REG_LDO5_VOLT_H = 0x18,
> + BD71827_REG_LDO5_VOLT_L = 0x19,
> + BD71827_REG_BUCK_PD_DIS = 0x1A,
> + BD71827_REG_LDO_PD_DIS  = 0x1B,
> + BD71827_REG_GPIO= 0x1C,
> + BD71827_REG_OUT32K  = 0x1D,
> + BD71827_REG_SEC = 0x1E,
> + BD71827_REG_MIN = 0x1F,
> + BD71827_REG_HOUR= 0x20,
> + BD71827_REG_WEEK= 0x21,
> + BD71827_REG_DAY = 0x22,
> + BD71827_REG_MONTH   = 0x23,
> + BD71827_REG_YEAR= 0x24,
> + BD71827_REG_ALM0_SEC= 0x25,
> + BD71827_REG_ALM0_MIN= 0x26,
> + BD71827_REG_ALM0_HOUR   = 0x27,
> + BD71827_REG_ALM0_WEEK   = 0x28,
> + BD71827_REG_ALM0_DAY= 0x29,
> + BD71827_REG_ALM0_MO