diff mbox series

[v7,1/6] mfd: sy7636a: Initial commit

Message ID 20210708115804.212-1-alistair@alistair23.me
State Superseded
Headers show
Series [v7,1/6] mfd: sy7636a: Initial commit | expand

Commit Message

Alistair July 8, 2021, 11:57 a.m. UTC
Initial support for the Silergy SY7636A Power Management chip.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/mfd/Kconfig         |  9 +++++
 drivers/mfd/Makefile        |  1 +
 drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
 4 files changed, 138 insertions(+)
 create mode 100644 drivers/mfd/sy7636a.c
 create mode 100644 include/linux/mfd/sy7636a.h

Comments

Lee Jones July 20, 2021, 2:53 p.m. UTC | #1
On Thu, 08 Jul 2021, Alistair Francis wrote:

> Initial support for the Silergy SY7636A Power Management chip.

> 

> Signed-off-by: Alistair Francis <alistair@alistair23.me>

> ---

>  drivers/mfd/Kconfig         |  9 +++++

>  drivers/mfd/Makefile        |  1 +

>  drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++

>  include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++

>  4 files changed, 138 insertions(+)

>  create mode 100644 drivers/mfd/sy7636a.c

>  create mode 100644 include/linux/mfd/sy7636a.h

> 

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig

> index 6a3fd2d75f96..7b59aa0fd3f2 100644

> --- a/drivers/mfd/Kconfig

> +++ b/drivers/mfd/Kconfig

> @@ -1352,6 +1352,15 @@ config MFD_SYSCON

>  	  Select this option to enable accessing system control registers

>  	  via regmap.

>  

> +config MFD_SY7636A

> +	tristate "Silergy SY7636A Power Management chip"


s/chip/IC/

> +	select MFD_CORE

> +	select REGMAP_I2C

> +	depends on I2C

> +	help

> +	  Select this option to enable support for the Silergy SY7636A

> +	  Power Management chip.


As above.

>  config MFD_DAVINCI_VOICECODEC

>  	tristate

>  	select MFD_CORE

> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile

> index 8116c19d5fd4..cbe581e87fa9 100644

> --- a/drivers/mfd/Makefile

> +++ b/drivers/mfd/Makefile

> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o

>  obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o

>  obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o

>  

> +obj-$(CONFIG_MFD_SY7636A)	+= sy7636a.o

>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o

>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o

>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o

> diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c

> new file mode 100644

> index 000000000000..345892e11221

> --- /dev/null

> +++ b/drivers/mfd/sy7636a.c

> @@ -0,0 +1,81 @@

> +// SPDX-License-Identifier: GPL-2.0+

> +//

> +// MFD parent driver for SY7636A chip

> +//

> +// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/

> +//

> +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>

> +//          Alistair Francis <alistair@alistair23.me>


Only C++ comments for the SPDX please.

> +// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>

> +

> +#include <linux/interrupt.h>

> +#include <linux/mfd/core.h>

> +#include <linux/module.h>

> +#include <linux/of_device.h>

> +

> +#include <linux/mfd/sy7636a.h>

> +

> +static const struct regmap_config sy7636a_regmap_config = {

> +	.reg_bits = 8,

> +	.val_bits = 8,

> +};

> +

> +static const struct mfd_cell sy7636a_cells[] = {

> +	{ .name = "sy7636a-regulator", },

> +	{ .name = "sy7636a-temperature", },

> +	{ .name = "sy7636a-thermal", },

> +};


If you put these in the Device Tree, you can use "simple-mfd-i2c"

> +static const struct of_device_id of_sy7636a_match_table[] = {

> +	{ .compatible = "silergy,sy7636a", },

> +	{}

> +};

> +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);


Place this near the i2c_device_id table please.

> +static int sy7636a_probe(struct i2c_client *client,

> +			 const struct i2c_device_id *ids)

> +{

> +	struct sy7636a *sy7636a;


Please call this ddata.

> +	int ret;

> +

> +	sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);

> +	if (!sy7636a)

> +		return -ENOMEM;

> +

> +	sy7636a->dev = &client->dev;


What are you using 'dev' for?

You can normally just use 'dev->parent' from the child device.

> +	sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);

> +	if (IS_ERR(sy7636a->regmap)) {

> +		ret = PTR_ERR(sy7636a->regmap);

> +		dev_err(sy7636a->dev,

> +			"Failed to initialize register map: %d\n", ret);

> +		return ret;

> +	}

> +

> +	i2c_set_clientdata(client, sy7636a);

> +

> +	return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,

> +					sy7636a_cells, ARRAY_SIZE(sy7636a_cells),

> +					NULL, 0, NULL);

> +}

> +

> +static const struct i2c_device_id sy7636a_id_table[] = {

> +	{ "sy7636a", 0 },

> +	{ },

> +};

> +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);


Use probe_new below, then you can omit this table.

> +static struct i2c_driver sy7636a_driver = {

> +	.driver	= {

> +		.name	= "sy7636a",

> +		.of_match_table = of_sy7636a_match_table,

> +	},

> +	.probe = sy7636a_probe,

> +	.id_table = sy7636a_id_table,

> +};

> +module_i2c_driver(sy7636a_driver);

> +

> +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");

> +MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");

> +MODULE_LICENSE("GPL v2");

> diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h

> new file mode 100644

> index 000000000000..b6845a3572b8

> --- /dev/null

> +++ b/include/linux/mfd/sy7636a.h

> @@ -0,0 +1,47 @@

> +/* SPDX-License-Identifier: GPL-2.0-only */

> +/*

> + * Functions to access SY3686A power management chip.

> + *

> + * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/

> + */

> +

> +#ifndef __MFD_SY7636A_H

> +#define __MFD_SY7636A_H

> +

> +#include <linux/i2c.h>

> +#include <linux/regmap.h>

> +#include <linux/regulator/driver.h>

> +#include <linux/regulator/machine.h>

> +

> +#define SY7636A_REG_OPERATION_MODE_CRL		0x00

> +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL	BIT(6)

> +#define SY7636A_OPERATION_MODE_CRL_ONOFF	BIT(7)

> +#define SY7636A_REG_VCOM_ADJUST_CTRL_L		0x01

> +#define SY7636A_REG_VCOM_ADJUST_CTRL_H		0x02

> +#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK	0x01ff

> +#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL	0x03

> +#define SY7636A_REG_POWER_ON_DELAY_TIME		0x06

> +#define SY7636A_REG_FAULT_FLAG			0x07

> +#define SY7636A_FAULT_FLAG_PG			BIT(0)

> +#define SY7636A_REG_TERMISTOR_READOUT		0x08

> +

> +#define SY7636A_REG_MAX				0x08

> +

> +#define VCOM_MIN		0

> +#define VCOM_MAX		5000

> +

> +#define VCOM_ADJUST_CTRL_MASK	0x1ff

> +// Used to shift the high byte

> +#define VCOM_ADJUST_CTRL_SHIFT	8

> +// Used to scale from VCOM_ADJUST_CTRL to mv

> +#define VCOM_ADJUST_CTRL_SCAL	10000

> +

> +#define FAULT_FLAG_SHIFT	1

> +

> +struct sy7636a {

> +	struct device *dev;

> +	struct regmap *regmap;

> +	struct gpio_desc *pgood_gpio;


This looks unused?

> +};

> +

> +#endif /* __LINUX_MFD_SY7636A_H */


-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Mark Brown July 20, 2021, 3:23 p.m. UTC | #2
On Tue, Jul 20, 2021 at 03:53:25PM +0100, Lee Jones wrote:
> On Thu, 08 Jul 2021, Alistair Francis wrote:


> > +static const struct mfd_cell sy7636a_cells[] = {

> > +	{ .name = "sy7636a-regulator", },

> > +	{ .name = "sy7636a-temperature", },

> > +	{ .name = "sy7636a-thermal", },

> > +};


> If you put these in the Device Tree, you can use "simple-mfd-i2c"


At least the regulator probably shouldn't be - this is just a Linux
specific grouping of devices, it's not really directly a block in the
hardware in a way that's platform independent.
Lee Jones July 20, 2021, 4:09 p.m. UTC | #3
On Tue, 20 Jul 2021, Mark Brown wrote:

> On Tue, Jul 20, 2021 at 03:53:25PM +0100, Lee Jones wrote:

> > On Thu, 08 Jul 2021, Alistair Francis wrote:

> 

> > > +static const struct mfd_cell sy7636a_cells[] = {

> > > +	{ .name = "sy7636a-regulator", },

> > > +	{ .name = "sy7636a-temperature", },

> > > +	{ .name = "sy7636a-thermal", },

> > > +};

> 

> > If you put these in the Device Tree, you can use "simple-mfd-i2c"

> 

> At least the regulator probably shouldn't be - this is just a Linux

> specific grouping of devices, it's not really directly a block in the

> hardware in a way that's platform independent.


I've seen (and authored) regulator support in DT before.

What's changed?  They're controlled by registers, right?

Is the problem that the registers are usually split?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Mark Brown July 20, 2021, 8:26 p.m. UTC | #4
On Tue, Jul 20, 2021 at 05:09:02PM +0100, Lee Jones wrote:
> On Tue, 20 Jul 2021, Mark Brown wrote:


> > At least the regulator probably shouldn't be - this is just a Linux

> > specific grouping of devices, it's not really directly a block in the

> > hardware in a way that's platform independent.


> I've seen (and authored) regulator support in DT before.


> What's changed?  They're controlled by registers, right?


Nothing's changed, I routinely push back on regulator drivers that have
a compatible string for a MFD subfunction like this.  I do miss them
sometimes but try not to.

> Is the problem that the registers are usually split?


It's just not really describing the hardware, it's encoding the way
Linux splits things up into the DT that adds no descriptive information.
We're not getting any information about where the IPs are in the device
or anything from the compatible, and typically it's describing a set of
disjoint IPs with minimal overlap in their configuration.  If it's a
binding for something like an individual LDO or DCDC and we've got
multiple instances of that within a single chip then it starts to get
more useful but that's not what something like this is doing.  We're not
gaining anything by putting a compatible string in there, all it does is
make the DT bigger and add some ABI.

Similar issues exist with CODEC subfunctions - those are usually
describing huge piles of different IPs but we happen to want to pull
them together for Linux, typically including some clocking which if we
were going down to the level of describing components of the MFD in the
DT should be being described using their own bindings.
Alistair Francis July 30, 2021, 6:21 a.m. UTC | #5
On Wed, Jul 21, 2021 at 6:26 AM Mark Brown <broonie@kernel.org> wrote:
>

> On Tue, Jul 20, 2021 at 05:09:02PM +0100, Lee Jones wrote:

> > On Tue, 20 Jul 2021, Mark Brown wrote:

>

> > > At least the regulator probably shouldn't be - this is just a Linux

> > > specific grouping of devices, it's not really directly a block in the

> > > hardware in a way that's platform independent.

>

> > I've seen (and authored) regulator support in DT before.

>

> > What's changed?  They're controlled by registers, right?

>

> Nothing's changed, I routinely push back on regulator drivers that have

> a compatible string for a MFD subfunction like this.  I do miss them

> sometimes but try not to.


Sorry, I just want to clarify what I should do.

Are you saying that I shouldn't add the regulator to the device tree?
Should I leave it as part of `sy7636a_cells` then?

Alistair

>

> > Is the problem that the registers are usually split?

>

> It's just not really describing the hardware, it's encoding the way

> Linux splits things up into the DT that adds no descriptive information.

> We're not getting any information about where the IPs are in the device

> or anything from the compatible, and typically it's describing a set of

> disjoint IPs with minimal overlap in their configuration.  If it's a

> binding for something like an individual LDO or DCDC and we've got

> multiple instances of that within a single chip then it starts to get

> more useful but that's not what something like this is doing.  We're not

> gaining anything by putting a compatible string in there, all it does is

> make the DT bigger and add some ABI.

>

> Similar issues exist with CODEC subfunctions - those are usually

> describing huge piles of different IPs but we happen to want to pull

> them together for Linux, typically including some clocking which if we

> were going down to the level of describing components of the MFD in the

> DT should be being described using their own bindings.
Mark Brown July 30, 2021, 11:13 a.m. UTC | #6
On Fri, Jul 30, 2021 at 04:21:18PM +1000, Alistair Francis wrote:

> Are you saying that I shouldn't add the regulator to the device tree?

> Should I leave it as part of `sy7636a_cells` then?


Yes, that'd be better.
Alistair Francis Aug. 2, 2021, 9:26 a.m. UTC | #7
On Wed, Jul 21, 2021 at 12:53 AM Lee Jones <lee.jones@linaro.org> wrote:
>

> On Thu, 08 Jul 2021, Alistair Francis wrote:

>

> > Initial support for the Silergy SY7636A Power Management chip.

> >

> > Signed-off-by: Alistair Francis <alistair@alistair23.me>

> > ---

> >  drivers/mfd/Kconfig         |  9 +++++

> >  drivers/mfd/Makefile        |  1 +

> >  drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++

> >  include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++

> >  4 files changed, 138 insertions(+)

> >  create mode 100644 drivers/mfd/sy7636a.c

> >  create mode 100644 include/linux/mfd/sy7636a.h

> >

> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig

> > index 6a3fd2d75f96..7b59aa0fd3f2 100644

> > --- a/drivers/mfd/Kconfig

> > +++ b/drivers/mfd/Kconfig

> > @@ -1352,6 +1352,15 @@ config MFD_SYSCON

> >         Select this option to enable accessing system control registers

> >         via regmap.

> >

> > +config MFD_SY7636A

> > +     tristate "Silergy SY7636A Power Management chip"

>

> s/chip/IC/

>

> > +     select MFD_CORE

> > +     select REGMAP_I2C

> > +     depends on I2C

> > +     help

> > +       Select this option to enable support for the Silergy SY7636A

> > +       Power Management chip.

>

> As above.

>

> >  config MFD_DAVINCI_VOICECODEC

> >       tristate

> >       select MFD_CORE

> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile

> > index 8116c19d5fd4..cbe581e87fa9 100644

> > --- a/drivers/mfd/Makefile

> > +++ b/drivers/mfd/Makefile

> > @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_KHADAS_MCU)      += khadas-mcu.o

> >  obj-$(CONFIG_MFD_ACER_A500_EC)       += acer-ec-a500.o

> >  obj-$(CONFIG_MFD_QCOM_PM8008)        += qcom-pm8008.o

> >

> > +obj-$(CONFIG_MFD_SY7636A)    += sy7636a.o

> >  obj-$(CONFIG_SGI_MFD_IOC3)   += ioc3.o

> >  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)     += simple-mfd-i2c.o

> >  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o

> > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c

> > new file mode 100644

> > index 000000000000..345892e11221

> > --- /dev/null

> > +++ b/drivers/mfd/sy7636a.c

> > @@ -0,0 +1,81 @@

> > +// SPDX-License-Identifier: GPL-2.0+

> > +//

> > +// MFD parent driver for SY7636A chip

> > +//

> > +// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/

> > +//

> > +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>

> > +//          Alistair Francis <alistair@alistair23.me>

>

> Only C++ comments for the SPDX please.

>

> > +// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>

> > +

> > +#include <linux/interrupt.h>

> > +#include <linux/mfd/core.h>

> > +#include <linux/module.h>

> > +#include <linux/of_device.h>

> > +

> > +#include <linux/mfd/sy7636a.h>

> > +

> > +static const struct regmap_config sy7636a_regmap_config = {

> > +     .reg_bits = 8,

> > +     .val_bits = 8,

> > +};

> > +

> > +static const struct mfd_cell sy7636a_cells[] = {

> > +     { .name = "sy7636a-regulator", },

> > +     { .name = "sy7636a-temperature", },

> > +     { .name = "sy7636a-thermal", },

> > +};

>

> If you put these in the Device Tree, you can use "simple-mfd-i2c"

>

> > +static const struct of_device_id of_sy7636a_match_table[] = {

> > +     { .compatible = "silergy,sy7636a", },

> > +     {}

> > +};

> > +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);

>

> Place this near the i2c_device_id table please.

>

> > +static int sy7636a_probe(struct i2c_client *client,

> > +                      const struct i2c_device_id *ids)

> > +{

> > +     struct sy7636a *sy7636a;

>

> Please call this ddata.

>

> > +     int ret;

> > +

> > +     sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);

> > +     if (!sy7636a)

> > +             return -ENOMEM;

> > +

> > +     sy7636a->dev = &client->dev;

>

> What are you using 'dev' for?

>

> You can normally just use 'dev->parent' from the child device.


I didn't realise that, I have removed `dev`.

>

> > +     sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);

> > +     if (IS_ERR(sy7636a->regmap)) {

> > +             ret = PTR_ERR(sy7636a->regmap);

> > +             dev_err(sy7636a->dev,

> > +                     "Failed to initialize register map: %d\n", ret);

> > +             return ret;

> > +     }

> > +

> > +     i2c_set_clientdata(client, sy7636a);

> > +

> > +     return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,

> > +                                     sy7636a_cells, ARRAY_SIZE(sy7636a_cells),

> > +                                     NULL, 0, NULL);

> > +}

> > +

> > +static const struct i2c_device_id sy7636a_id_table[] = {

> > +     { "sy7636a", 0 },

> > +     { },

> > +};

> > +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);

>

> Use probe_new below, then you can omit this table.

>

> > +static struct i2c_driver sy7636a_driver = {

> > +     .driver = {

> > +             .name   = "sy7636a",

> > +             .of_match_table = of_sy7636a_match_table,

> > +     },

> > +     .probe = sy7636a_probe,

> > +     .id_table = sy7636a_id_table,

> > +};

> > +module_i2c_driver(sy7636a_driver);

> > +

> > +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");

> > +MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");

> > +MODULE_LICENSE("GPL v2");

> > diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h

> > new file mode 100644

> > index 000000000000..b6845a3572b8

> > --- /dev/null

> > +++ b/include/linux/mfd/sy7636a.h

> > @@ -0,0 +1,47 @@

> > +/* SPDX-License-Identifier: GPL-2.0-only */

> > +/*

> > + * Functions to access SY3686A power management chip.

> > + *

> > + * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/

> > + */

> > +

> > +#ifndef __MFD_SY7636A_H

> > +#define __MFD_SY7636A_H

> > +

> > +#include <linux/i2c.h>

> > +#include <linux/regmap.h>

> > +#include <linux/regulator/driver.h>

> > +#include <linux/regulator/machine.h>

> > +

> > +#define SY7636A_REG_OPERATION_MODE_CRL               0x00

> > +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL   BIT(6)

> > +#define SY7636A_OPERATION_MODE_CRL_ONOFF     BIT(7)

> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_L               0x01

> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_H               0x02

> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK    0x01ff

> > +#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL        0x03

> > +#define SY7636A_REG_POWER_ON_DELAY_TIME              0x06

> > +#define SY7636A_REG_FAULT_FLAG                       0x07

> > +#define SY7636A_FAULT_FLAG_PG                        BIT(0)

> > +#define SY7636A_REG_TERMISTOR_READOUT                0x08

> > +

> > +#define SY7636A_REG_MAX                              0x08

> > +

> > +#define VCOM_MIN             0

> > +#define VCOM_MAX             5000

> > +

> > +#define VCOM_ADJUST_CTRL_MASK        0x1ff

> > +// Used to shift the high byte

> > +#define VCOM_ADJUST_CTRL_SHIFT       8

> > +// Used to scale from VCOM_ADJUST_CTRL to mv

> > +#define VCOM_ADJUST_CTRL_SCAL        10000

> > +

> > +#define FAULT_FLAG_SHIFT     1

> > +

> > +struct sy7636a {

> > +     struct device *dev;

> > +     struct regmap *regmap;

> > +     struct gpio_desc *pgood_gpio;

>

> This looks unused?


It is used in the syy636a-regulator

Alistair

>

> > +};

> > +

> > +#endif /* __LINUX_MFD_SY7636A_H */

>

> --

> Lee Jones [李琼斯]

> Senior Technical Lead - Developer Services

> Linaro.org │ Open source software for Arm SoCs

> Follow Linaro: Facebook | Twitter | Blog
Lee Jones Aug. 2, 2021, 10:23 a.m. UTC | #8
On Mon, 02 Aug 2021, Alistair Francis wrote:

> On Wed, Jul 21, 2021 at 12:53 AM Lee Jones <lee.jones@linaro.org> wrote:

> >

> > On Thu, 08 Jul 2021, Alistair Francis wrote:

> >

> > > Initial support for the Silergy SY7636A Power Management chip.

> > >

> > > Signed-off-by: Alistair Francis <alistair@alistair23.me>

> > > ---

> > >  drivers/mfd/Kconfig         |  9 +++++

> > >  drivers/mfd/Makefile        |  1 +

> > >  drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++

> > >  include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++

> > >  4 files changed, 138 insertions(+)

> > >  create mode 100644 drivers/mfd/sy7636a.c

> > >  create mode 100644 include/linux/mfd/sy7636a.h


[...]

> > > +struct sy7636a {

> > > +     struct device *dev;

> > > +     struct regmap *regmap;

> > > +     struct gpio_desc *pgood_gpio;

> >

> > This looks unused?

> 

> It is used in the syy636a-regulator


If it's only used in one driver, please declare it there.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
diff mbox series

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6a3fd2d75f96..7b59aa0fd3f2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1352,6 +1352,15 @@  config MFD_SYSCON
 	  Select this option to enable accessing system control registers
 	  via regmap.
 
+config MFD_SY7636A
+	tristate "Silergy SY7636A Power Management chip"
+	select MFD_CORE
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Select this option to enable support for the Silergy SY7636A
+	  Power Management chip.
+
 config MFD_DAVINCI_VOICECODEC
 	tristate
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8116c19d5fd4..cbe581e87fa9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -266,6 +266,7 @@  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o
 obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o
 
+obj-$(CONFIG_MFD_SY7636A)	+= sy7636a.o
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
new file mode 100644
index 000000000000..345892e11221
--- /dev/null
+++ b/drivers/mfd/sy7636a.c
@@ -0,0 +1,81 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+//
+// MFD parent driver for SY7636A chip
+//
+// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
+//
+// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+//          Alistair Francis <alistair@alistair23.me>
+//
+// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
+
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <linux/mfd/sy7636a.h>
+
+static const struct regmap_config sy7636a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct mfd_cell sy7636a_cells[] = {
+	{ .name = "sy7636a-regulator", },
+	{ .name = "sy7636a-temperature", },
+	{ .name = "sy7636a-thermal", },
+};
+
+static const struct of_device_id of_sy7636a_match_table[] = {
+	{ .compatible = "silergy,sy7636a", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);
+
+static int sy7636a_probe(struct i2c_client *client,
+			 const struct i2c_device_id *ids)
+{
+	struct sy7636a *sy7636a;
+	int ret;
+
+	sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);
+	if (!sy7636a)
+		return -ENOMEM;
+
+	sy7636a->dev = &client->dev;
+
+	sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
+	if (IS_ERR(sy7636a->regmap)) {
+		ret = PTR_ERR(sy7636a->regmap);
+		dev_err(sy7636a->dev,
+			"Failed to initialize register map: %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, sy7636a);
+
+	return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
+					sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
+					NULL, 0, NULL);
+}
+
+static const struct i2c_device_id sy7636a_id_table[] = {
+	{ "sy7636a", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);
+
+static struct i2c_driver sy7636a_driver = {
+	.driver	= {
+		.name	= "sy7636a",
+		.of_match_table = of_sy7636a_match_table,
+	},
+	.probe = sy7636a_probe,
+	.id_table = sy7636a_id_table,
+};
+module_i2c_driver(sy7636a_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
+MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
new file mode 100644
index 000000000000..b6845a3572b8
--- /dev/null
+++ b/include/linux/mfd/sy7636a.h
@@ -0,0 +1,47 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Functions to access SY3686A power management chip.
+ *
+ * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
+ */
+
+#ifndef __MFD_SY7636A_H
+#define __MFD_SY7636A_H
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define SY7636A_REG_OPERATION_MODE_CRL		0x00
+#define SY7636A_OPERATION_MODE_CRL_VCOMCTL	BIT(6)
+#define SY7636A_OPERATION_MODE_CRL_ONOFF	BIT(7)
+#define SY7636A_REG_VCOM_ADJUST_CTRL_L		0x01
+#define SY7636A_REG_VCOM_ADJUST_CTRL_H		0x02
+#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK	0x01ff
+#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL	0x03
+#define SY7636A_REG_POWER_ON_DELAY_TIME		0x06
+#define SY7636A_REG_FAULT_FLAG			0x07
+#define SY7636A_FAULT_FLAG_PG			BIT(0)
+#define SY7636A_REG_TERMISTOR_READOUT		0x08
+
+#define SY7636A_REG_MAX				0x08
+
+#define VCOM_MIN		0
+#define VCOM_MAX		5000
+
+#define VCOM_ADJUST_CTRL_MASK	0x1ff
+// Used to shift the high byte
+#define VCOM_ADJUST_CTRL_SHIFT	8
+// Used to scale from VCOM_ADJUST_CTRL to mv
+#define VCOM_ADJUST_CTRL_SCAL	10000
+
+#define FAULT_FLAG_SHIFT	1
+
+struct sy7636a {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *pgood_gpio;
+};
+
+#endif /* __LINUX_MFD_SY7636A_H */