Messages in this thread | | | Date | Tue, 30 Jun 2020 11:16:33 +0200 | From | Michael Walle <> | Subject | Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access |
| |
Hi Lee,
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.
-michael
| |