Messages in this thread | | | Date | Wed, 01 Jul 2020 00:32:13 +0200 | From | Michael Walle <> | Subject | Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access |
| |
Hi Lee,
Am 2020-06-30 11:16, schrieb Michael Walle: > I'm just trying to use this for my sl28 driver. Some remarks, see > below. > > Am 2020-06-22 09:51, schrieb Lee Jones: >> The existing SYSCON implementation only supports MMIO (memory mapped) >> accesses, facilitated by Regmap. This extends support for registers >> held behind I2C busses. >> >> Signed-off-by: Lee Jones <lee.jones@linaro.org> >> --- >> Changelog: >> >> v3 => v4 >> - Add ability to provide a non-default Regmap configuration >> >> v2 => v3 >> - Change 'is CONFIG' present check to include loadable modules >> - s/#ifdef CONFIG_MFD_SYSCON_I2C/#if >> IS_ENABLED(CONFIG_MFD_SYSCON_I2C)/ >> >> v1 => v2 >> - Remove legacy references to OF >> - Allow building as a module (fixes h8300 0-day issue) >> >> drivers/mfd/Kconfig | 7 +++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/syscon-i2c.c | 104 >> +++++++++++++++++++++++++++++++++ >> include/linux/mfd/syscon-i2c.h | 36 ++++++++++++ >> 4 files changed, 148 insertions(+) >> create mode 100644 drivers/mfd/syscon-i2c.c >> create mode 100644 include/linux/mfd/syscon-i2c.h >> > > [..] > >> +static struct regmap *syscon_i2c_get_regmap(struct i2c_client >> *client, >> + struct regmap_config *regmap_config) >> +{ >> + struct device *dev = &client->dev; >> + struct syscon *entry, *syscon = NULL; >> + >> + spin_lock(&syscon_i2c_list_slock); >> + >> + list_for_each_entry(entry, &syscon_i2c_list, list) >> + if (entry->dev == dev) { >> + syscon = entry; >> + break; >> + } >> + >> + spin_unlock(&syscon_i2c_list_slock); >> + >> + if (!syscon) >> + syscon = syscon_i2c_register(client, regmap_config); >> + >> + if (IS_ERR(syscon)) >> + return ERR_CAST(syscon); >> + >> + return syscon->regmap; >> +} >> + >> +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client, >> + struct regmap_config *regmap_config) >> +{ >> + return syscon_i2c_get_regmap(client, regmap_config); >> +} >> +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config); >> + >> +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client) >> +{ >> + return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config); >> +} >> +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap); > > What do you think about > > struct regmap *syscon_i2c_to_regmap(struct device *dev) > { > struct i2c_client *client = i2c_verify_client(dev); > > if (!client) > return ERR_PTR(-EINVAL); > > return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config); > } > > Or even move it to syscon_i2c_get_regmap(). > > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" > just > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do > it > all over again in all sub drivers. > > So you could just do a > regmap = syscon_i2c_to_regmap(pdev->dev.parent); > > I've also noticed that the mmio syscon uses device_node as parameter. > What > was the reason to divert from that? Just curious.
How is this supposed to be used?
I had something like the following in mind:
&i2c { cpld@4a { compatible = "simple-mfd"; reg = <0x4a>;
gpio@4 { compatible = "vendor,gpio"; reg = <0x4>; }; }; };
But I think the childen are not enumerated if its an I2C device. And the actual i2c driver is also missing.
-michael
| |