| Message ID | 20210117042539.1609-1-alistair@alistair23.me |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/6] devicetree/bindings: Initial commit of silergy,sy7636a.yaml | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote: > --- /dev/null > +++ b/drivers/regulator/sy7636a-regulator.c > @@ -0,0 +1,233 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Functions to access SY3686A power management chip voltages > + * Please make the entire comment a C++ one so things look more intentional. > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > + * > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com> This probably needs an update. > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. This boilerplate is redundant and should be removed. > +static int get_vcom_voltage_op(struct regulator_dev *rdev) > +{ > + int ret = get_vcom_voltage_mv(rdev->regmap); > + Why is this get_vcom_voltage_mv() function not in the regulator driver, and why is it not just inline here? It also needs namespacing. > +static int disable_regulator(struct regulator_dev *rdev) > +{ > + struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent); > + int ret = 0; > + > + mutex_lock(&sy7636a->reglock); > + ret = regulator_disable_regmap(rdev); > + usleep_range(30000, 35000); > + mutex_unlock(&sy7636a->reglock); Why do you need this delay here, and what purpose is this lock intended to serve? I can't understand what it's intended to protect. > + mutex_lock(&sy7636a->reglock); > + ret = regulator_is_enabled_regmap(rdev); > + mutex_unlock(&sy7636a->reglock); This lock usage in particular looks confused. > + ret = regulator_enable_regmap(rdev); > + if (ret) > + goto finish; > + if (!pwr_good) { > + dev_err(&rdev->dev, "Power good signal timeout after %u ms\n", > + jiffies_to_msecs(t1 - t0)); > + ret = -ETIME; > + goto finish; > + } This doesn't undo the underlying enable, leaving the regulator in a partially enabled state. > +static const struct regulator_ops sy7636a_vcom_volt_ops = { > + .get_voltage = get_vcom_voltage_op, > + .enable = enable_regulator_pgood, > + .disable = disable_regulator, > + .is_enabled = sy7636a_regulator_is_enabled, > +}; The namespacing for functions is very random and prone to clashes. Given the power good signal I'd also expect a get_status() operation. > +static int sy7636a_regulator_suspend(struct device *dev) > +{ > + int ret; > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent); > + > + ret = get_vcom_voltage_mv(sy7636a->regmap); > + > + if (ret > 0) > + sy7636a->vcom = (unsigned int)ret; > + > + return 0; > +} What's going on here, and if you are going to store this value over suspend why not store it in a variable of the correct type? In general it's surprising to need a suspend operation for a regulator. > + sy7636a->pgood_gpio = gdp; > + dev_info(sy7636a->dev, > + "Power good GPIO registered (gpio# %d)\n", > + desc_to_gpio(sy7636a->pgood_gpio)); This print is just adding noise to the boot process.
On Sat, 16 Jan 2021 20:25:34 -0800, Alistair Francis wrote: > Initial support for the Silergy SY7636A Power Management chip > driver. > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > --- > .../bindings/mfd/silergy,sy7636a.yaml | 37 +++++++++++++++++++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml: $id: relative path/filename doesn't match actual path or filename expected: http://devicetree.org/schemas/mfd/silergy,sy7636a.yaml# See https://patchwork.ozlabs.org/patch/1427906 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@kernel.org> wrote: > > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote: > > > --- /dev/null > > +++ b/drivers/regulator/sy7636a-regulator.c > > @@ -0,0 +1,233 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Functions to access SY3686A power management chip voltages > > + * > > Please make the entire comment a C++ one so things look more > intentional. Fixed. > > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > > + * > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com> > > This probably needs an update. > > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > This boilerplate is redundant and should be removed. Fixed. > > > +static int get_vcom_voltage_op(struct regulator_dev *rdev) > > +{ > > + int ret = get_vcom_voltage_mv(rdev->regmap); > > + > > Why is this get_vcom_voltage_mv() function not in the regulator driver, > and why is it not just inline here? It also needs namespacing. I'm not sure what you mean, can you please explain? > > > +static int disable_regulator(struct regulator_dev *rdev) > > +{ > > + struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent); > > + int ret = 0; > > + > > + mutex_lock(&sy7636a->reglock); > > + ret = regulator_disable_regmap(rdev); > > + usleep_range(30000, 35000); > > + mutex_unlock(&sy7636a->reglock); > > Why do you need this delay here, and what purpose is this lock intended The delay is to allow a power ramp up, I have added a comment. > to serve? I can't understand what it's intended to protect. Apparently the mutex is to protect enable/disable, I don't think it's required and I will remove it. > > > + mutex_lock(&sy7636a->reglock); > > + ret = regulator_is_enabled_regmap(rdev); > > + mutex_unlock(&sy7636a->reglock); > > This lock usage in particular looks confused. > > > + ret = regulator_enable_regmap(rdev); > > + if (ret) > > + goto finish; > > > + if (!pwr_good) { > > + dev_err(&rdev->dev, "Power good signal timeout after %u ms\n", > > + jiffies_to_msecs(t1 - t0)); > > + ret = -ETIME; > > + goto finish; > > + } > > This doesn't undo the underlying enable, leaving the regulator in a > partially enabled state. Thanks, fixed. > > > +static const struct regulator_ops sy7636a_vcom_volt_ops = { > > + .get_voltage = get_vcom_voltage_op, > > + .enable = enable_regulator_pgood, > > + .disable = disable_regulator, > > + .is_enabled = sy7636a_regulator_is_enabled, > > +}; > > The namespacing for functions is very random and prone to clashes. Fixed. > Given the power good signal I'd also expect a get_status() operation. Added. > > > +static int sy7636a_regulator_suspend(struct device *dev) > > +{ > > + int ret; > > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent); > > + > > + ret = get_vcom_voltage_mv(sy7636a->regmap); > > + > > + if (ret > 0) > > + sy7636a->vcom = (unsigned int)ret; > > + > > + return 0; > > +} > > What's going on here, and if you are going to store this value over > suspend why not store it in a variable of the correct type? In general It is part of the vendor's kernel, they specifically added it to ensure vcom is set on resume. I have fixed the variable type. > it's surprising to need a suspend operation for a regulator. > > > + sy7636a->pgood_gpio = gdp; > > + dev_info(sy7636a->dev, > > + "Power good GPIO registered (gpio# %d)\n", > > + desc_to_gpio(sy7636a->pgood_gpio)); > > This print is just adding noise to the boot process. Removed. Alistair
On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote: > On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@kernel.org> wrote: > > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote: > > > +static int get_vcom_voltage_op(struct regulator_dev *rdev) > > > +{ > > > + int ret = get_vcom_voltage_mv(rdev->regmap); > > > + > > Why is this get_vcom_voltage_mv() function not in the regulator driver, > > and why is it not just inline here? It also needs namespacing. > I'm not sure what you mean, can you please explain? This is a wrapper for a function that has exactly one caller but is not only a separate function but also in the MFD header, part of a separate driver. This seems at best pointless. > > Why do you need this delay here, and what purpose is this lock intended > The delay is to allow a power ramp up, I have added a comment. Use the standard ramp_delay, don't open code it. > > > +static int sy7636a_regulator_suspend(struct device *dev) > > > +{ > > > + int ret; > > > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent); > > > + > > > + ret = get_vcom_voltage_mv(sy7636a->regmap); > > > + > > > + if (ret > 0) > > > + sy7636a->vcom = (unsigned int)ret; > > > + > > > + return 0; > > > +} > > What's going on here, and if you are going to store this value over > > suspend why not store it in a variable of the correct type? In general > It is part of the vendor's kernel, they specifically added it to > ensure vcom is set on resume. "I copied this from the vendor" isn't really a great explanation... If the device is likely to get completely powered off and loosing settings then presumably the entire register map, not just this one value, needs to be saved and restored instead of just this one value. If that is the case it's probably best to use a register cache and just resync it on resume.
On Fri, Jan 22, 2021 at 5:37 AM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote: > > On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@kernel.org> wrote: > > > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote: > > > > > +static int get_vcom_voltage_op(struct regulator_dev *rdev) > > > > +{ > > > > + int ret = get_vcom_voltage_mv(rdev->regmap); > > > > + > > > > Why is this get_vcom_voltage_mv() function not in the regulator driver, > > > and why is it not just inline here? It also needs namespacing. > > > I'm not sure what you mean, can you please explain? > > This is a wrapper for a function that has exactly one caller but is not > only a separate function but also in the MFD header, part of a separate > driver. This seems at best pointless. Ah I see. I think I have fixed this. > > > > Why do you need this delay here, and what purpose is this lock intended > > > The delay is to allow a power ramp up, I have added a comment. > > Use the standard ramp_delay, don't open code it. > > > > > +static int sy7636a_regulator_suspend(struct device *dev) > > > > +{ > > > > + int ret; > > > > + struct sy7636a *sy7636a = dev_get_drvdata(dev->parent); > > > > + > > > > + ret = get_vcom_voltage_mv(sy7636a->regmap); > > > > + > > > > + if (ret > 0) > > > > + sy7636a->vcom = (unsigned int)ret; > > > > + > > > > + return 0; > > > > +} > > > > What's going on here, and if you are going to store this value over > > > suspend why not store it in a variable of the correct type? In general > > > It is part of the vendor's kernel, they specifically added it to > > ensure vcom is set on resume. > > "I copied this from the vendor" isn't really a great explanation... If > the device is likely to get completely powered off and loosing settings > then presumably the entire register map, not just this one value, needs > to be saved and restored instead of just this one value. If that is the > case it's probably best to use a register cache and just resync it on > resume. Good point. I don't have a good answer so I have removed the suspend/resume part. I'll have to investigate in the future if/why this is required. Alistair
On Sat, 16 Jan 2021, Alistair Francis wrote: > Initial support for the Silergy SY7636A Power Management chip > driver. Please remove "driver", as this is not support for the driver, it *is* the driver which supports the chip. > Signed-off-by: Alistair Francis <alistair@alistair23.me> > --- > drivers/mfd/Kconfig | 10 ++ > drivers/mfd/Makefile | 2 + > drivers/mfd/sy7636a.c | 252 ++++++++++++++++++++++++++++++++++++ > include/linux/mfd/sy7636a.h | 50 +++++++ > 4 files changed, 314 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 bdfce7b15621..c8c62d92433c 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1360,6 +1360,16 @@ config MFD_SYSCON > Select this option to enable accessing system control registers > via regmap. > > +config MFD_SY7636A > + tristate "Silergy SY7636A Power Management chip driver" Again, please remove the word "driver" here. > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + depends on I2C=y > + help > + Select this option to enable support for the Silergy SY7636A > + Power Management chip driver. And again. > config MFD_DAVINCI_VOICECODEC > tristate > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 14fdb188af02..1fa1e635f506 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -265,6 +265,8 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > obj-$(CONFIG_MFD_STMFX) += stmfx.o > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > +obj-$(CONFIG_MFD_SY7636A) += sy7636a.o > + Why does this have to be segregated? > 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..39aac965d854 > --- /dev/null > +++ b/drivers/mfd/sy7636a.c > @@ -0,0 +1,252 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * MFD driver for SY7636A chip "Parent driver". > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ This is quite out of date. Please update. > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. This test is replaced by the SPDX header above. > + * 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/regmap.h> > +#include <linux/sysfs.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); Please move this to where it is used i.e. at the bottom of the file. > +static const char * const states[] = { > + "no fault event", > + "UVP at VP rail", > + "UVP at VN rail", > + "UVP at VPOS rail", > + "UVP at VNEG rail", > + "UVP at VDDH rail", > + "UVP at VEE rail", > + "SCP at VP rail", > + "SCP at VN rail", > + "SCP at VPOS rail", > + "SCP at VNEG rail", > + "SCP at VDDH rail", > + "SCP at VEE rail", > + "SCP at V COM rail", > + "UVLO", > + "Thermal shutdown", > +}; > + > +int get_vcom_voltage_mv(struct regmap *regmap) > +{ > + int ret; > + unsigned int val, val_h; > + > + ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, &val); > + if (ret) > + return ret; > + > + ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, &val_h); > + if (ret) > + return ret; > + > + val |= (val_h << 8); Please define the shifts and masks. > + return (val & 0x1FF) * 10; What's 10? > +} > + > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom) > +{ > + int ret; > + unsigned int val; > + > + if (vcom < 0 || vcom > 5000) Please define min/max values. > + return -EINVAL; > + > + val = (unsigned int)(vcom / 10) & 0x1ff; As above. > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val); > + if (ret) > + return ret; > + > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8); > + if (ret) > + return ret; > + > + return 0; > +} Who calls these? > +static ssize_t state_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + unsigned int val; > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > + > + ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val); > + if (ret) { > + dev_err(sy7636a->dev, "Failed to read from device\n"); > + return ret; > + } > + > + val = val >> 1; Why 1? > + if (val >= ARRAY_SIZE(states)) { > + dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val); > + return -EINVAL; > + } > + > + return snprintf(buf, PAGE_SIZE, "%s\n", states[val]); > +} > +static DEVICE_ATTR(state, 0444, state_show, NULL); You need to document new sysfs entries. > +static ssize_t power_good_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + unsigned int val; > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > + > + ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val); > + if (ret) { > + dev_err(sy7636a->dev, "Failed to read from device\n"); > + return ret; > + } > + > + val &= 0x01; > + > + return snprintf(buf, PAGE_SIZE, "%s\n", val ? "ON" : "OFF"); Doesn't 0 just mean "no fault event"? > +} > +static DEVICE_ATTR(power_good, 0444, power_good_show, NULL); > + > +static ssize_t vcom_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > + > + ret = get_vcom_voltage_mv(sy7636a->regmap); > + if (ret < 0) > + return ret; > + > + return snprintf(buf, PAGE_SIZE, "%d\n", -ret); > +} > + Remove this line please. > +static ssize_t vcom_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ > + int ret; > + int vcom; > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > + > + ret = kstrtoint(buf, 0, &vcom); > + if (ret) > + return ret; > + > + if (vcom > 0 || vcom < -5000) > + return -EINVAL; > + > + ret = set_vcom_voltage_mv(sy7636a->regmap, (unsigned int)(-vcom)); > + if (ret) > + return ret; > + > + return count; > +} > +static DEVICE_ATTR(vcom, 0644, vcom_show, vcom_store); > + > +static struct attribute *sy7636a_sysfs_attrs[] = { > + &dev_attr_state.attr, > + &dev_attr_power_good.attr, > + &dev_attr_vcom.attr, > + NULL, > +}; These all look like power options? Do they really belong here? > +static const struct attribute_group sy7636a_sysfs_attr_group = { > + .attrs = sy7636a_sysfs_attrs, > +}; > + > +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(struct sy7636a), GFP_KERNEL); sizeof(*sy7636a) > + if (sy7636a == NULL) 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); > + > + ret = sysfs_create_group(&client->dev.kobj, &sy7636a_sysfs_attr_group); > + if (ret) { > + dev_err(sy7636a->dev, "Failed to create sysfs attributes\n"); > + return ret; > + } > + > + ret = devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO, > + sy7636a_cells, ARRAY_SIZE(sy7636a_cells), > + NULL, 0, NULL); > + if (ret) { > + dev_err(sy7636a->dev, "Failed to add mfd devices\n"); "Failed to add child devices" > + sysfs_remove_group(&client->dev.kobj, &sy7636a_sysfs_attr_group); > + return ret; > + } > + > + return 0; > +} > + > +static const struct i2c_device_id sy7636a_id_table[] = { > + { "sy7636a", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table); If you use .probe2, 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"); s/Multi-Function Device Driver/Power Management Chip/ > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h > new file mode 100644 > index 000000000000..642789c4d0a9 > --- /dev/null > +++ b/include/linux/mfd/sy7636a.h > @@ -0,0 +1,50 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Functions to access SY3686A power management chip. > + * > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ Same issues as mentioned above. > +#ifndef __LINUX_MFD_SY7636A_H > +#define __LINUX_MFD_SY7636A_H Just MFD is fine. > +#include <linux/i2c.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > +#include <linux/regmap.h> Alphabetical. > +#define SY7636A_REG_OPERATION_MODE_CRL 0x00 > +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL (1 << 6) > +#define SY7636A_OPERATION_MODE_CRL_ONOFF (1 << 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 (1 << 0) > +#define SY7636A_REG_TERMISTOR_READOUT 0x08 Tab out the values please. Use BIT() > +#define SY7636A_REG_MAX 0x08 > + > +struct sy7636a { > + struct device *dev; > + struct regmap *regmap; > + unsigned int vcom; Where is this used? > + struct gpio_desc *pgood_gpio; Where is this used? > + struct mutex reglock; Where is this used? > +}; > + > +int get_vcom_voltage_mv(struct regmap *regmap); > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom); What calls these? > +#endif /* __LINUX_MFD_SY7636A_H */
On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Sat, 16 Jan 2021, Alistair Francis wrote: > > > Initial support for the Silergy SY7636A Power Management chip > > driver. > > Please remove "driver", as this is not support for the driver, it *is* > the driver which supports the chip. Sorry for the long delay here. I have addressed your comments. > > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > > --- > > drivers/mfd/Kconfig | 10 ++ > > drivers/mfd/Makefile | 2 + > > drivers/mfd/sy7636a.c | 252 ++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/sy7636a.h | 50 +++++++ > > 4 files changed, 314 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 bdfce7b15621..c8c62d92433c 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1360,6 +1360,16 @@ config MFD_SYSCON > > Select this option to enable accessing system control registers > > via regmap. > > > > +config MFD_SY7636A > > + tristate "Silergy SY7636A Power Management chip driver" > > Again, please remove the word "driver" here. > > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + depends on I2C=y > > + help > > + Select this option to enable support for the Silergy SY7636A > > + Power Management chip driver. > > And again. I removed "driver" from all of these. > > > config MFD_DAVINCI_VOICECODEC > > tristate > > select MFD_CORE > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 14fdb188af02..1fa1e635f506 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -265,6 +265,8 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > > obj-$(CONFIG_MFD_STMFX) += stmfx.o > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > > > +obj-$(CONFIG_MFD_SY7636A) += sy7636a.o > > + > > Why does this have to be segregated? Fixed > > > 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..39aac965d854 > > --- /dev/null > > +++ b/drivers/mfd/sy7636a.c > > @@ -0,0 +1,252 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * MFD driver for SY7636A chip > > "Parent driver". > > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > > This is quite out of date. Please update. I don't own this copyright, so I would rather not change it. > > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com> > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > This test is replaced by the SPDX header above. Removed. > > > + * 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/regmap.h> > > +#include <linux/sysfs.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); > > Please move this to where it is used i.e. at the bottom of the file. Done. > > > +static const char * const states[] = { > > + "no fault event", > > + "UVP at VP rail", > > + "UVP at VN rail", > > + "UVP at VPOS rail", > > + "UVP at VNEG rail", > > + "UVP at VDDH rail", > > + "UVP at VEE rail", > > + "SCP at VP rail", > > + "SCP at VN rail", > > + "SCP at VPOS rail", > > + "SCP at VNEG rail", > > + "SCP at VDDH rail", > > + "SCP at VEE rail", > > + "SCP at V COM rail", > > + "UVLO", > > + "Thermal shutdown", > > +}; > > + > > +int get_vcom_voltage_mv(struct regmap *regmap) > > +{ > > + int ret; > > + unsigned int val, val_h; > > + > > + ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, &val); > > + if (ret) > > + return ret; > > + > > + ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, &val_h); > > + if (ret) > > + return ret; > > + > > + val |= (val_h << 8); > > Please define the shifts and masks. > > > + return (val & 0x1FF) * 10; > > What's 10? > > > +} > > + > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom) > > +{ > > + int ret; > > + unsigned int val; > > + > > + if (vcom < 0 || vcom > 5000) > > Please define min/max values. > > > + return -EINVAL; > > + > > + val = (unsigned int)(vcom / 10) & 0x1ff; > > As above. I have used defines for all of these. > > > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > Who calls these? They sysfs store and show functions. > > > +static ssize_t state_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + int ret; > > + unsigned int val; > > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > > + > > + ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val); > > + if (ret) { > > + dev_err(sy7636a->dev, "Failed to read from device\n"); > > + return ret; > > + } > > + > > + val = val >> 1; > > Why 1? I used a define here to make it clearer > > > + if (val >= ARRAY_SIZE(states)) { > > + dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val); > > + return -EINVAL; > > + } > > + > > + return snprintf(buf, PAGE_SIZE, "%s\n", states[val]); > > +} > > +static DEVICE_ATTR(state, 0444, state_show, NULL); > > You need to document new sysfs entries. I'm not sure how to document this. Do you mind pointing out an example I can use? > > > +static ssize_t power_good_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + int ret; > > + unsigned int val; > > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > > + > > + ret = regmap_read(sy7636a->regmap, SY7636A_REG_FAULT_FLAG, &val); > > + if (ret) { > > + dev_err(sy7636a->dev, "Failed to read from device\n"); > > + return ret; > > + } > > + > > + val &= 0x01; > > + > > + return snprintf(buf, PAGE_SIZE, "%s\n", val ? "ON" : "OFF"); > > Doesn't 0 just mean "no fault event"? The LSB is reserved, so we don't clear that. > > > +} > > +static DEVICE_ATTR(power_good, 0444, power_good_show, NULL); > > + > > +static ssize_t vcom_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + int ret; > > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > > + > > + ret = get_vcom_voltage_mv(sy7636a->regmap); > > + if (ret < 0) > > + return ret; > > + > > + return snprintf(buf, PAGE_SIZE, "%d\n", -ret); > > +} > > + > > Remove this line please. > > > +static ssize_t vcom_store(struct device *dev, > > + struct device_attribute *attr, const char *buf, size_t count) > > +{ > > + int ret; > > + int vcom; > > + struct sy7636a *sy7636a = dev_get_drvdata(dev); > > + > > + ret = kstrtoint(buf, 0, &vcom); > > + if (ret) > > + return ret; > > + > > + if (vcom > 0 || vcom < -5000) > > + return -EINVAL; > > + > > + ret = set_vcom_voltage_mv(sy7636a->regmap, (unsigned int)(-vcom)); > > + if (ret) > > + return ret; > > + > > + return count; > > +} > > +static DEVICE_ATTR(vcom, 0644, vcom_show, vcom_store); > > + > > +static struct attribute *sy7636a_sysfs_attrs[] = { > > + &dev_attr_state.attr, > > + &dev_attr_power_good.attr, > > + &dev_attr_vcom.attr, > > + NULL, > > +}; > > These all look like power options? Do they really belong here? From what I can tell I think they do. Let me know if you don't think so. > > > +static const struct attribute_group sy7636a_sysfs_attr_group = { > > + .attrs = sy7636a_sysfs_attrs, > > +}; > > + > > +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(struct sy7636a), GFP_KERNEL); > > sizeof(*sy7636a) > > > + if (sy7636a == NULL) > > 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); > > + > > + ret = sysfs_create_group(&client->dev.kobj, &sy7636a_sysfs_attr_group); > > + if (ret) { > > + dev_err(sy7636a->dev, "Failed to create sysfs attributes\n"); > > + return ret; > > + } > > + > > + ret = devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO, > > + sy7636a_cells, ARRAY_SIZE(sy7636a_cells), > > + NULL, 0, NULL); > > + if (ret) { > > + dev_err(sy7636a->dev, "Failed to add mfd devices\n"); > > "Failed to add child devices" > > > + sysfs_remove_group(&client->dev.kobj, &sy7636a_sysfs_attr_group); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct i2c_device_id sy7636a_id_table[] = { > > + { "sy7636a", 0 }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table); > > If you use .probe2, 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"); > > s/Multi-Function Device Driver/Power Management Chip/ > > > +MODULE_LICENSE("GPL v2"); > > diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h > > new file mode 100644 > > index 000000000000..642789c4d0a9 > > --- /dev/null > > +++ b/include/linux/mfd/sy7636a.h > > @@ -0,0 +1,50 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Functions to access SY3686A power management chip. > > + * > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > Same issues as mentioned above. > > > +#ifndef __LINUX_MFD_SY7636A_H > > +#define __LINUX_MFD_SY7636A_H > > Just MFD is fine. > > > +#include <linux/i2c.h> > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/machine.h> > > +#include <linux/regmap.h> > > Alphabetical. > > > +#define SY7636A_REG_OPERATION_MODE_CRL 0x00 > > +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL (1 << 6) > > +#define SY7636A_OPERATION_MODE_CRL_ONOFF (1 << 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 (1 << 0) > > +#define SY7636A_REG_TERMISTOR_READOUT 0x08 > > Tab out the values please. > > Use BIT() > > > +#define SY7636A_REG_MAX 0x08 > > + > > +struct sy7636a { > > + struct device *dev; > > + struct regmap *regmap; > > + unsigned int vcom; > > Where is this used? > > > + struct gpio_desc *pgood_gpio; > > Where is this used? > > > + struct mutex reglock; > > Where is this used? I have removed these. > > > +}; > > + > > +int get_vcom_voltage_mv(struct regmap *regmap); > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom); > > What calls these? I have just made these internal to the driver. 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
On Sat, 20 Mar 2021, Alistair Francis wrote: > On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Sat, 16 Jan 2021, Alistair Francis wrote: > > > > > Initial support for the Silergy SY7636A Power Management chip > > > driver. > > > > Please remove "driver", as this is not support for the driver, it *is* > > the driver which supports the chip. > > Sorry for the long delay here. > > I have addressed your comments. [...] > > > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c > > > new file mode 100644 > > > index 000000000000..39aac965d854 > > > --- /dev/null > > > +++ b/drivers/mfd/sy7636a.c > > > @@ -0,0 +1,252 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * MFD driver for SY7636A chip > > > > "Parent driver". > > > > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > > > > This is quite out of date. Please update. > > I don't own this copyright, so I would rather not change it. I'm not comfortable taking a new driver with an old Copyright. Maybe ask reMarkable if it's okay to bump it. > > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com> Or ping this guy. [...] > > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom) > > > +{ > > > + int ret; > > > + unsigned int val; > > > + > > > + if (vcom < 0 || vcom > 5000) > > > > Please define min/max values. > > > > > + return -EINVAL; > > > + > > > + val = (unsigned int)(vcom / 10) & 0x1ff; > > > > As above. > > I have used defines for all of these. > > > > > > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val); > > > + if (ret) > > > + return ret; > > > + > > > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8); > > > + if (ret) > > > + return ret; > > > + > > > + return 0; > > > +} > > > > Who calls these? > > They sysfs store and show functions. They should be in a power/regulator driver really. [...] > > > + if (val >= ARRAY_SIZE(states)) { > > > + dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val); > > > + return -EINVAL; > > > + } > > > + > > > + return snprintf(buf, PAGE_SIZE, "%s\n", states[val]); > > > +} > > > +static DEVICE_ATTR(state, 0444, state_show, NULL); > > > > You need to document new sysfs entries. > > I'm not sure how to document this. Do you mind pointing out an example > I can use? See the final paragraph in: Documentation/filesystems/sysfs.rst [...] > > > +static struct attribute *sy7636a_sysfs_attrs[] = { > > > + &dev_attr_state.attr, > > > + &dev_attr_power_good.attr, > > > + &dev_attr_vcom.attr, > > > + NULL, > > > +}; > > > > These all look like power options? Do they really belong here? > > From what I can tell I think they do. Let me know if you don't think so. As above, I think they should be in power or regulator. [...]
On Tue, Mar 23, 2021 at 5:35 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Sat, 20 Mar 2021, Alistair Francis wrote: > > > On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > On Sat, 16 Jan 2021, Alistair Francis wrote: > > > > > > > Initial support for the Silergy SY7636A Power Management chip > > > > driver. > > > > > > Please remove "driver", as this is not support for the driver, it *is* > > > the driver which supports the chip. > > > > Sorry for the long delay here. > > > > I have addressed your comments. > > [...] > > > > > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c > > > > new file mode 100644 > > > > index 000000000000..39aac965d854 > > > > --- /dev/null > > > > +++ b/drivers/mfd/sy7636a.c > > > > @@ -0,0 +1,252 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * MFD driver for SY7636A chip > > > > > > "Parent driver". > > > > > > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > > > > > > This is quite out of date. Please update. > > > > I don't own this copyright, so I would rather not change it. > > I'm not comfortable taking a new driver with an old Copyright. > > Maybe ask reMarkable if it's okay to bump it. > > > > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com> > > Or ping this guy. I reached out to him and have permission to bump the year. > > [...] > > > > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom) > > > > +{ > > > > + int ret; > > > > + unsigned int val; > > > > + > > > > + if (vcom < 0 || vcom > 5000) > > > > > > Please define min/max values. > > > > > > > + return -EINVAL; > > > > + > > > > + val = (unsigned int)(vcom / 10) & 0x1ff; > > > > > > As above. > > > > I have used defines for all of these. > > > > > > > > > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + return 0; > > > > +} > > > > > > Who calls these? > > > > They sysfs store and show functions. > > They should be in a power/regulator driver really. Ok, I have moved these to the regulator. > > [...] > > > > > + if (val >= ARRAY_SIZE(states)) { > > > > + dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return snprintf(buf, PAGE_SIZE, "%s\n", states[val]); > > > > +} > > > > +static DEVICE_ATTR(state, 0444, state_show, NULL); > > > > > > You need to document new sysfs entries. > > > > I'm not sure how to document this. Do you mind pointing out an example > > I can use? > > See the final paragraph in: > > Documentation/filesystems/sysfs.rst Thanks! > > [...] > > > > > +static struct attribute *sy7636a_sysfs_attrs[] = { > > > > + &dev_attr_state.attr, > > > > + &dev_attr_power_good.attr, > > > > + &dev_attr_vcom.attr, > > > > + NULL, > > > > +}; > > > > > > These all look like power options? Do they really belong here? > > > > From what I can tell I think they do. Let me know if you don't think so. > > As above, I think they should be in power or regulator. Done. Alistair > > [...] > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml new file mode 100644 index 000000000000..37541a7fcc5d --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml @@ -0,0 +1,37 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/regulator/silergy,sy7636a.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: silergy sy7636a PMIC + +maintainers: + - Alistair Francis <alistair@alistair23.me> + +properties: + compatible: + enum: + - silergy,sy7636a + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + regulator@60 { + compatible = "silergy,sy7636a"; + reg = <0x60>; + }; + }; + +...
Initial support for the Silergy SY7636A Power Management chip driver. Signed-off-by: Alistair Francis <alistair@alistair23.me> --- .../bindings/mfd/silergy,sy7636a.yaml | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml