| Message ID | 20210615103400.946-1-alistair@alistair23.me |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v6,1/5] dt-bindings: mfd: Initial commit of silergy,sy7636a.yaml | expand |
| Context | Check | Description |
|---|---|---|
| robh/dtbs-check | success | |
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
On Tue, 15 Jun 2021 20:33:56 +1000, Alistair Francis wrote: > Initial support for the Silergy SY7636A Power Management chip > and regulator. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [3/5] regulator: sy7636a: Initial commit commit: 8c485bedfb7852fa4de2a34aac2a6fd911f539f4 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Tue, 15 Jun 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 | 82 +++++++++++++++++++++++++++++++++++++ > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++ > 4 files changed, 139 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 5c7f2b100191..7d6cf32b1549 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1339,6 +1339,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 4f6d2b8a5f76..f95e1e725a95 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.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..e08f29ea63f8 > --- /dev/null > +++ b/drivers/mfd/sy7636a.c > @@ -0,0 +1,82 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// Only the SPDX with C++ style comments please. > +// MFD parent driver for SY7636A chip Drop the MFD part. It's a Linuxisum that doesn't really exist. > +// 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); Hold on. This driver doesn't really do anything. If you create OF nodes for all the sub-devices, you can use simple-mfd-i2c. Any reason you can't do that?
On Thu, 17 Jun 2021, Alistair Francis wrote: > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Tue, 15 Jun 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 | 82 +++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++ > > > 4 files changed, 139 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 5c7f2b100191..7d6cf32b1549 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -1339,6 +1339,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 4f6d2b8a5f76..f95e1e725a95 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.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..e08f29ea63f8 > > > --- /dev/null > > > +++ b/drivers/mfd/sy7636a.c > > > @@ -0,0 +1,82 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +// > > > > Only the SPDX with C++ style comments please. > > > > > +// MFD parent driver for SY7636A chip > > > > Drop the MFD part. It's a Linuxisum that doesn't really exist. > > > > > +// 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); > > > > Hold on. This driver doesn't really do anything. If you create OF > > nodes for all the sub-devices, you can use simple-mfd-i2c. > > > > Any reason you can't do that? > > Just to confirm, you mean something like this? > > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts > b/arch/arm/boot/dts/imx7d-remarkable2.dts > index 9327d1c06c96..3577104b3853 100644 > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts > @@ -382,6 +382,21 @@ epd_pmic: sy7636a@62 { > pinctrl-0 = <&pinctrl_epdpmic>; > #thermal-sensor-cells = <0>; > > + regulator@0 { > + compatible = "sy7636a-regulator"; > + reg = <0>; > + }; > + > + temperature@0 { > + compatible = "sy7636a-temperature"; > + reg = <0>; > + }; > + > + thermal@0 { > + compatible = "sy7636a-thermal"; > + reg = <0>; > + }; > + > regulators { > compatible = "silergy,sy7636a-regulator"; > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c > index 87f684cff9a1..622a05318cff 100644 > --- a/drivers/mfd/simple-mfd-i2c.c > +++ b/drivers/mfd/simple-mfd-i2c.c > @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c) > > static const struct of_device_id simple_mfd_i2c_of_match[] = { > { .compatible = "kontron,sl28cpld" }, > + { .compatible = "silergy,sy7636a" }, > {} > }; > MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match); Essentially. Take a look at how the other users are implementing. The reg entries look bogus to me though. Maybe just leave them out?
On Tue, Jun 15, 2021 at 08:33:56PM +1000, Alistair Francis wrote: > Initial support for the Silergy SY7636A Power Management chip > and regulator. > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > --- > .../bindings/mfd/silergy,sy7636a.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml > new file mode 100644 > index 000000000000..9e50f57d5e8d > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/silergy,sy7636a.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: silergy sy7636a PMIC > + > +maintainers: > + - Alistair Francis <alistair@alistair23.me> > + > +properties: > + compatible: > + const: silergy,sy7636a > + > + reg: > + maxItems: 1 > + > + '#thermal-sensor-cells': > + const: 0 > + > + epd-pwr-good-gpios: > + description: > + Specifying the power good GPIOs. As defined in bindings/gpio.txt. Drop the 2nd sentence. > + maxItems: 1 > + > + regulators: > + type: object > + > + properties: > + compatible: > + const: silergy,sy7636a-regulator > + > + "vcom": Don't need quotes. > + type: object > + $ref: /schemas/regulator/regulator.yaml# > + > + regulator-name: > + const: "vcom" Don't need quotes. Doesn't this belong in the 'vcom' node? You need another 'properties' and this under it. > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - '#thermal-sensor-cells' > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pmic@62 { > + compatible = "silergy,sy7636a"; > + reg = <0x62>; > + status = "okay"; Don't show status in examples. > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_epdpmic>; > + #thermal-sensor-cells = <0>; > + > + regulators { > + compatible = "silergy,sy7636a-regulator"; > + reg_epdpmic: vcom { > + regulator-name = "vcom"; > + regulator-boot-on; > + }; > + }; > + }; > + }; > +... > -- > 2.31.1 > >
On Thu, Jun 17, 2021 at 7:20 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Thu, 17 Jun 2021, Alistair Francis wrote: > > > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > On Tue, 15 Jun 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 | 82 +++++++++++++++++++++++++++++++++++++ > > > > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++ > > > > 4 files changed, 139 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 5c7f2b100191..7d6cf32b1549 100644 > > > > --- a/drivers/mfd/Kconfig > > > > +++ b/drivers/mfd/Kconfig > > > > @@ -1339,6 +1339,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 4f6d2b8a5f76..f95e1e725a95 100644 > > > > --- a/drivers/mfd/Makefile > > > > +++ b/drivers/mfd/Makefile > > > > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > > > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.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..e08f29ea63f8 > > > > --- /dev/null > > > > +++ b/drivers/mfd/sy7636a.c > > > > @@ -0,0 +1,82 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +// > > > > > > Only the SPDX with C++ style comments please. > > > > > > > +// MFD parent driver for SY7636A chip > > > > > > Drop the MFD part. It's a Linuxisum that doesn't really exist. > > > > > > > +// 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); > > > > > > Hold on. This driver doesn't really do anything. If you create OF > > > nodes for all the sub-devices, you can use simple-mfd-i2c. > > > > > > Any reason you can't do that? > > > > Just to confirm, you mean something like this? > > > > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts > > b/arch/arm/boot/dts/imx7d-remarkable2.dts > > index 9327d1c06c96..3577104b3853 100644 > > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts > > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts > > @@ -382,6 +382,21 @@ epd_pmic: sy7636a@62 { > > pinctrl-0 = <&pinctrl_epdpmic>; > > #thermal-sensor-cells = <0>; > > > > + regulator@0 { > > + compatible = "sy7636a-regulator"; > > + reg = <0>; > > + }; > > + > > + temperature@0 { > > + compatible = "sy7636a-temperature"; > > + reg = <0>; > > + }; > > + > > + thermal@0 { > > + compatible = "sy7636a-thermal"; > > + reg = <0>; > > + }; > > + > > regulators { > > compatible = "silergy,sy7636a-regulator"; > > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c > > index 87f684cff9a1..622a05318cff 100644 > > --- a/drivers/mfd/simple-mfd-i2c.c > > +++ b/drivers/mfd/simple-mfd-i2c.c > > @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c) > > > > static const struct of_device_id simple_mfd_i2c_of_match[] = { > > { .compatible = "kontron,sl28cpld" }, > > + { .compatible = "silergy,sy7636a" }, > > {} > > }; > > MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match); > > Essentially. Take a look at how the other users are implementing. > > The reg entries look bogus to me though. Maybe just leave them out? So I tried this and didn't have any luck. After some Kconfig changes to allow it to build, I managed to get it probing, but I never got it to power up. It doesn't seem to be the same. Alistair > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
On Thu, 08 Jul 2021, Alistair Francis wrote: > On Thu, Jun 17, 2021 at 7:20 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Thu, 17 Jun 2021, Alistair Francis wrote: > > > > > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > > On Tue, 15 Jun 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 | 82 +++++++++++++++++++++++++++++++++++++ > > > > > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++ > > > > > 4 files changed, 139 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 5c7f2b100191..7d6cf32b1549 100644 > > > > > --- a/drivers/mfd/Kconfig > > > > > +++ b/drivers/mfd/Kconfig > > > > > @@ -1339,6 +1339,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 4f6d2b8a5f76..f95e1e725a95 100644 > > > > > --- a/drivers/mfd/Makefile > > > > > +++ b/drivers/mfd/Makefile > > > > > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > > > > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.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..e08f29ea63f8 > > > > > --- /dev/null > > > > > +++ b/drivers/mfd/sy7636a.c > > > > > @@ -0,0 +1,82 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > +// > > > > > > > > Only the SPDX with C++ style comments please. > > > > > > > > > +// MFD parent driver for SY7636A chip > > > > > > > > Drop the MFD part. It's a Linuxisum that doesn't really exist. > > > > > > > > > +// 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); > > > > > > > > Hold on. This driver doesn't really do anything. If you create OF > > > > nodes for all the sub-devices, you can use simple-mfd-i2c. > > > > > > > > Any reason you can't do that? > > > > > > Just to confirm, you mean something like this? > > > > > > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts > > > b/arch/arm/boot/dts/imx7d-remarkable2.dts > > > index 9327d1c06c96..3577104b3853 100644 > > > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts > > > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts > > > @@ -382,6 +382,21 @@ epd_pmic: sy7636a@62 { > > > pinctrl-0 = <&pinctrl_epdpmic>; > > > #thermal-sensor-cells = <0>; > > > > > > + regulator@0 { > > > + compatible = "sy7636a-regulator"; > > > + reg = <0>; > > > + }; > > > + > > > + temperature@0 { > > > + compatible = "sy7636a-temperature"; > > > + reg = <0>; > > > + }; > > > + > > > + thermal@0 { > > > + compatible = "sy7636a-thermal"; > > > + reg = <0>; > > > + }; > > > + > > > regulators { > > > compatible = "silergy,sy7636a-regulator"; > > > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c > > > index 87f684cff9a1..622a05318cff 100644 > > > --- a/drivers/mfd/simple-mfd-i2c.c > > > +++ b/drivers/mfd/simple-mfd-i2c.c > > > @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c) > > > > > > static const struct of_device_id simple_mfd_i2c_of_match[] = { > > > { .compatible = "kontron,sl28cpld" }, > > > + { .compatible = "silergy,sy7636a" }, > > > {} > > > }; > > > MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match); > > > > Essentially. Take a look at how the other users are implementing. > > > > The reg entries look bogus to me though. Maybe just leave them out? > > So I tried this and didn't have any luck. > > After some Kconfig changes to allow it to build, I managed to get it > probing, but I never got it to power up. It doesn't seem to be the > same. I need a more technical reason why this is not the correct approach for you. "I can't get it to work" doesn't quite reach the quality line I'm afraid. Did you try enabling the debug prints in of_platform_bus_create() and friends to see if your devices are probing correctly?
On Thu, Jul 8, 2021 at 10:47 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Thu, 08 Jul 2021, Alistair Francis wrote: > > > On Thu, Jun 17, 2021 at 7:20 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > On Thu, 17 Jun 2021, Alistair Francis wrote: > > > > > > > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > > > > On Tue, 15 Jun 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 | 82 +++++++++++++++++++++++++++++++++++++ > > > > > > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++ > > > > > > 4 files changed, 139 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 5c7f2b100191..7d6cf32b1549 100644 > > > > > > --- a/drivers/mfd/Kconfig > > > > > > +++ b/drivers/mfd/Kconfig > > > > > > @@ -1339,6 +1339,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 4f6d2b8a5f76..f95e1e725a95 100644 > > > > > > --- a/drivers/mfd/Makefile > > > > > > +++ b/drivers/mfd/Makefile > > > > > > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > > > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > > > > > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.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..e08f29ea63f8 > > > > > > --- /dev/null > > > > > > +++ b/drivers/mfd/sy7636a.c > > > > > > @@ -0,0 +1,82 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > +// > > > > > > > > > > Only the SPDX with C++ style comments please. > > > > > > > > > > > +// MFD parent driver for SY7636A chip > > > > > > > > > > Drop the MFD part. It's a Linuxisum that doesn't really exist. > > > > > > > > > > > +// 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); > > > > > > > > > > Hold on. This driver doesn't really do anything. If you create OF > > > > > nodes for all the sub-devices, you can use simple-mfd-i2c. > > > > > > > > > > Any reason you can't do that? > > > > > > > > Just to confirm, you mean something like this? > > > > > > > > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > b/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > index 9327d1c06c96..3577104b3853 100644 > > > > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > @@ -382,6 +382,21 @@ epd_pmic: sy7636a@62 { > > > > pinctrl-0 = <&pinctrl_epdpmic>; > > > > #thermal-sensor-cells = <0>; > > > > > > > > + regulator@0 { > > > > + compatible = "sy7636a-regulator"; > > > > + reg = <0>; > > > > + }; > > > > + > > > > + temperature@0 { > > > > + compatible = "sy7636a-temperature"; > > > > + reg = <0>; > > > > + }; > > > > + > > > > + thermal@0 { > > > > + compatible = "sy7636a-thermal"; > > > > + reg = <0>; > > > > + }; > > > > + > > > > regulators { > > > > compatible = "silergy,sy7636a-regulator"; > > > > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c > > > > index 87f684cff9a1..622a05318cff 100644 > > > > --- a/drivers/mfd/simple-mfd-i2c.c > > > > +++ b/drivers/mfd/simple-mfd-i2c.c > > > > @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c) > > > > > > > > static const struct of_device_id simple_mfd_i2c_of_match[] = { > > > > { .compatible = "kontron,sl28cpld" }, > > > > + { .compatible = "silergy,sy7636a" }, > > > > {} > > > > }; > > > > MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match); > > > > > > Essentially. Take a look at how the other users are implementing. > > > > > > The reg entries look bogus to me though. Maybe just leave them out? > > > > So I tried this and didn't have any luck. > > > > After some Kconfig changes to allow it to build, I managed to get it > > probing, but I never got it to power up. It doesn't seem to be the > > same. > > I need a more technical reason why this is not the correct approach > for you. "I can't get it to work" doesn't quite reach the quality > line I'm afraid. Yep, so one thing that we need that the simple-mfd-i2c doesn't do is the epd-pwr-good-gpios GPIO. This is a GPIO line from the mfd that the regulator uses. > > Did you try enabling the debug prints in of_platform_bus_create() and > friends to see if your devices are probing correctly? I think I did, but I can have another look and see if I can get it working. Alistair > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
On Wed, 28 Jul 2021, Alistair Francis wrote: > On Thu, Jul 8, 2021 at 10:47 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Thu, 08 Jul 2021, Alistair Francis wrote: > > > > > On Thu, Jun 17, 2021 at 7:20 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > > On Thu, 17 Jun 2021, Alistair Francis wrote: > > > > > > > > > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > > > > > > On Tue, 15 Jun 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 | 82 +++++++++++++++++++++++++++++++++++++ > > > > > > > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++ > > > > > > > 4 files changed, 139 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 5c7f2b100191..7d6cf32b1549 100644 > > > > > > > --- a/drivers/mfd/Kconfig > > > > > > > +++ b/drivers/mfd/Kconfig > > > > > > > @@ -1339,6 +1339,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 4f6d2b8a5f76..f95e1e725a95 100644 > > > > > > > --- a/drivers/mfd/Makefile > > > > > > > +++ b/drivers/mfd/Makefile > > > > > > > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > > > > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > > > > > > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.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..e08f29ea63f8 > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/mfd/sy7636a.c > > > > > > > @@ -0,0 +1,82 @@ > > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > > +// > > > > > > > > > > > > Only the SPDX with C++ style comments please. > > > > > > > > > > > > > +// MFD parent driver for SY7636A chip > > > > > > > > > > > > Drop the MFD part. It's a Linuxisum that doesn't really exist. > > > > > > > > > > > > > +// 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); > > > > > > > > > > > > Hold on. This driver doesn't really do anything. If you create OF > > > > > > nodes for all the sub-devices, you can use simple-mfd-i2c. > > > > > > > > > > > > Any reason you can't do that? > > > > > > > > > > Just to confirm, you mean something like this? > > > > > > > > > > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > > b/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > > index 9327d1c06c96..3577104b3853 100644 > > > > > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts > > > > > @@ -382,6 +382,21 @@ epd_pmic: sy7636a@62 { > > > > > pinctrl-0 = <&pinctrl_epdpmic>; > > > > > #thermal-sensor-cells = <0>; > > > > > > > > > > + regulator@0 { > > > > > + compatible = "sy7636a-regulator"; > > > > > + reg = <0>; > > > > > + }; > > > > > + > > > > > + temperature@0 { > > > > > + compatible = "sy7636a-temperature"; > > > > > + reg = <0>; > > > > > + }; > > > > > + > > > > > + thermal@0 { > > > > > + compatible = "sy7636a-thermal"; > > > > > + reg = <0>; > > > > > + }; > > > > > + > > > > > regulators { > > > > > compatible = "silergy,sy7636a-regulator"; > > > > > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c > > > > > index 87f684cff9a1..622a05318cff 100644 > > > > > --- a/drivers/mfd/simple-mfd-i2c.c > > > > > +++ b/drivers/mfd/simple-mfd-i2c.c > > > > > @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c) > > > > > > > > > > static const struct of_device_id simple_mfd_i2c_of_match[] = { > > > > > { .compatible = "kontron,sl28cpld" }, > > > > > + { .compatible = "silergy,sy7636a" }, > > > > > {} > > > > > }; > > > > > MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match); > > > > > > > > Essentially. Take a look at how the other users are implementing. > > > > > > > > The reg entries look bogus to me though. Maybe just leave them out? > > > > > > So I tried this and didn't have any luck. > > > > > > After some Kconfig changes to allow it to build, I managed to get it > > > probing, but I never got it to power up. It doesn't seem to be the > > > same. > > > > I need a more technical reason why this is not the correct approach > > for you. "I can't get it to work" doesn't quite reach the quality > > line I'm afraid. > > Yep, so one thing that we need that the simple-mfd-i2c doesn't do is > the epd-pwr-good-gpios GPIO. This is a GPIO line from the mfd that the > regulator uses. Can this be obtained from anywhere else? Like in the regulator driver for instance. > > Did you try enabling the debug prints in of_platform_bus_create() and > > friends to see if your devices are probing correctly? > > I think I did, but I can have another look and see if I can get it working. > > Alistair > > >
diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml new file mode 100644 index 000000000000..9e50f57d5e8d --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml @@ -0,0 +1,73 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/silergy,sy7636a.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: silergy sy7636a PMIC + +maintainers: + - Alistair Francis <alistair@alistair23.me> + +properties: + compatible: + const: silergy,sy7636a + + reg: + maxItems: 1 + + '#thermal-sensor-cells': + const: 0 + + epd-pwr-good-gpios: + description: + Specifying the power good GPIOs. As defined in bindings/gpio.txt. + maxItems: 1 + + regulators: + type: object + + properties: + compatible: + const: silergy,sy7636a-regulator + + "vcom": + type: object + $ref: /schemas/regulator/regulator.yaml# + + regulator-name: + const: "vcom" + + additionalProperties: false + +required: + - compatible + - reg + - '#thermal-sensor-cells' + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pmic@62 { + compatible = "silergy,sy7636a"; + reg = <0x62>; + status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_epdpmic>; + #thermal-sensor-cells = <0>; + + regulators { + compatible = "silergy,sy7636a-regulator"; + reg_epdpmic: vcom { + regulator-name = "vcom"; + regulator-boot-on; + }; + }; + }; + }; +...
Initial support for the Silergy SY7636A Power Management chip and regulator. Signed-off-by: Alistair Francis <alistair@alistair23.me> --- .../bindings/mfd/silergy,sy7636a.yaml | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml