| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210120083348.GM4903@dell>
Date: Wed, 20 Jan 2021 08:33:48 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Cristian Ciocaltea <cristian.ciocaltea@...il.com>
Cc: Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Sebastian Reichel <sre@...nel.org>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Liam Girdwood <lgirdwood@...il.com>,
Andreas Färber <afaerber@...e.de>,
Linus Walleij <linus.walleij@...aro.org>,
linux-actions@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH v5 3/7] mfd: Add MFD driver for ATC260x PMICs
On Wed, 13 Jan 2021, Cristian Ciocaltea wrote:
> Add initial support for the Actions Semi ATC260x PMICs which integrates
> Audio Codec, Power management, Clock generation and GPIO controller
> blocks.
>
> For the moment this driver only supports Regulator, Poweroff and Onkey
> functionalities for the ATC2603C and ATC2609A chip variants.
>
> Since the PMICs can be accessed using both I2C and SPI buses, the
> following driver structure has been adopted:
>
> -----> atc260x-core.c (Implements core functionalities)
> /
> ATC260x --------> atc260x-i2c.c (Implements I2C interface)
> \
> -----> atc260x-spi.c (Implements SPI interface - TODO)
>
> Co-developed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...il.com>
> ---
> Changes in v5:
> - None
>
> Changes in v4 - according to Lee's review:
> - Replaced 'regmap_add_irq_chip()' with 'devm' counterpart and dropped
> 'atc260x_device_remove()' and 'atc260x_i2c_remove()' functions
> - Moved kerneldoc sections from prototypes to real functions
> - Placed single line entries on one line for mfd_cells[]
> - Several other minor changes
>
> Changes in v3:
> - Fixed the issues reported by Lee's kernel test robot:
> WARNING: modpost: missing MODULE_LICENSE() in drivers/mfd/atc260x-core.o
> >> FATAL: modpost: drivers/mfd/atc260x-i2c: sizeof(struct i2c_device_id)=24 is
> not a modulo of the size of section __mod_i2c__<identifier>_device_table=588.
> >> Fix definition of struct i2c_device_id in mod_devicetable.h
> - Dropped the usage of '.of_compatible' fields in {atc2603c,atc2609a}_mfd_cells[]
> - Added 'Co-developed-by' tag in commit message and dropped [cristian: ...] line
>
> drivers/mfd/Kconfig | 18 ++
> drivers/mfd/Makefile | 3 +
> drivers/mfd/atc260x-core.c | 293 +++++++++++++++++++++++++
> drivers/mfd/atc260x-i2c.c | 64 ++++++
> include/linux/mfd/atc260x/atc2603c.h | 281 ++++++++++++++++++++++++
> include/linux/mfd/atc260x/atc2609a.h | 308 +++++++++++++++++++++++++++
> include/linux/mfd/atc260x/core.h | 58 +++++
> 7 files changed, 1025 insertions(+)
> create mode 100644 drivers/mfd/atc260x-core.c
> create mode 100644 drivers/mfd/atc260x-i2c.c
> create mode 100644 include/linux/mfd/atc260x/atc2603c.h
> create mode 100644 include/linux/mfd/atc260x/atc2609a.h
> create mode 100644 include/linux/mfd/atc260x/core.h
[...]
> +/**
> + * atc260x_device_probe(): Probe a configured ATC260x device
> + *
> + * @atc260x: ATC260x device to probe (must be configured)
> + *
> + * This function lets the ATC260x core register the ATC260x MFD devices
> + * and IRQCHIP. The ATC260x device passed in must be fully configured
> + * with atc260x_match_device, its IRQ set, and regmap created.
> + */
> +int atc260x_device_probe(struct atc260x *atc260x)
> +{
> + struct device *dev = atc260x->dev;
> + unsigned int chip_rev;
> + int ret;
> +
> + if (!atc260x->irq) {
> + dev_err(dev, "No interrupt support\n");
> + return -EINVAL;
> + }
> +
> + /* Initialize the hardware */
> + atc260x->dev_init(atc260x);
> +
> + ret = regmap_read(atc260x->regmap, atc260x->rev_reg, &chip_rev);
> + if (ret) {
> + dev_err(dev, "Failed to get chip revision\n");
> + return ret;
> + }
> +
> + if (chip_rev > 31) {
Nit: If you have to respin this, please define this magic number.
> + dev_err(dev, "Unknown chip revision: %u\n", chip_rev);
> + return -EINVAL;
> + }
> +
> + atc260x->ic_ver = __ffs(chip_rev + 1U);
> +
> + dev_info(dev, "Detected chip type %s rev.%c\n",
> + atc260x->type_name, 'A' + atc260x->ic_ver);
> +
> + ret = devm_regmap_add_irq_chip(dev, atc260x->regmap, atc260x->irq, IRQF_ONESHOT,
> + -1, atc260x->regmap_irq_chip, &atc260x->irq_data);
> + if (ret) {
> + dev_err(dev, "Failed to add IRQ chip: %d\n", ret);
> + return ret;
> + }
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> + atc260x->cells, atc260x->nr_cells, NULL, 0,
> + regmap_irq_get_domain(atc260x->irq_data));
> + if (ret) {
> + dev_err(dev, "Failed to add child devices: %d\n", ret);
> + regmap_del_irq_chip(atc260x->irq, atc260x->irq_data);
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(atc260x_device_probe);
[...]
> +static struct i2c_driver atc260x_i2c_driver = {
> + .driver = {
> + .name = "atc260x",
> + .of_match_table = of_match_ptr(atc260x_i2c_of_match),
> + },
> + .probe = atc260x_i2c_probe,
> +};
Nit: These spacings/line-ups just look odd.
Please stick to one ' ' after the '='.
> +module_i2c_driver(atc260x_i2c_driver);
[...]
> +struct atc260x {
> + struct device *dev;
> +
> + struct regmap *regmap;
> + const struct regmap_irq_chip *regmap_irq_chip;
> + struct regmap_irq_chip_data *irq_data;
> +
> + struct mutex *regmap_mutex; /* mutex for custom regmap locking */
> +
> + const struct mfd_cell *cells;
> + int nr_cells;
> + int irq;
> +
> + enum atc260x_type ic_type;
> + enum atc260x_ver ic_ver;
> + const char *type_name;
> + unsigned int rev_reg;
> +
> + int (*dev_init)(struct atc260x *atc260x);
Ah, I didn't see this before.
Call-backs of this nature are the devil. Please populate a struct
with the differentiating register addresses/values instead and always
call a generic deivce_init().
> +};
> +
> +struct regmap_config;
> +
> +int atc260x_match_device(struct atc260x *atc260x, struct regmap_config *regmap_cfg);
> +int atc260x_device_probe(struct atc260x *atc260x);
> +
> +#endif /* __LINUX_MFD_ATC260X_CORE_H */
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists