From: Lee Jones <lee.jones@linaro.org>
To: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
Cc: "Mark Brown" <broonie@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
"Sebastian Reichel" <sre@kernel.org>,
"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
"Liam Girdwood" <lgirdwood@gmail.com>,
"Andreas Färber" <afaerber@suse.de>,
"Linus Walleij" <linus.walleij@linaro.org>,
linux-actions@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
linux-pm@vger.kernel.org
Subject: Re: [PATCH v5 3/7] mfd: Add MFD driver for ATC260x PMICs
Date: Wed, 20 Jan 2021 08:33:48 +0000 [thread overview]
Message-ID: <20210120083348.GM4903@dell> (raw)
In-Reply-To: <81546cf3265f51374a1b38b9e801003fd6c3e298.1610534765.git.cristian.ciocaltea@gmail.com>
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@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.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
next prev parent reply other threads:[~2021-01-20 8:37 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-13 11:05 [PATCH v5 0/7] Add initial support for ATC260x PMICs Cristian Ciocaltea
2021-01-13 11:05 ` [PATCH v5 1/7] dt-bindings: input: Add reset-time-sec common property Cristian Ciocaltea
2021-01-13 11:05 ` [PATCH v5 2/7] dt-bindings: mfd: Add Actions Semi ATC260x PMIC binding Cristian Ciocaltea
2021-01-13 11:05 ` [PATCH v5 3/7] mfd: Add MFD driver for ATC260x PMICs Cristian Ciocaltea
2021-01-20 8:33 ` Lee Jones [this message]
2021-01-20 18:39 ` Cristian Ciocaltea
2021-01-13 11:05 ` [PATCH v5 4/7] regulator: Add regulator " Cristian Ciocaltea
2021-01-13 11:05 ` [PATCH v5 5/7] power: reset: Add poweroff " Cristian Ciocaltea
2021-01-13 21:20 ` Sebastian Reichel
2021-01-13 11:05 ` [PATCH v5 6/7] input: atc260x: Add onkey " Cristian Ciocaltea
2021-01-13 11:05 ` [PATCH v5 7/7] MAINTAINERS: Add entry for ATC260x PMIC Cristian Ciocaltea
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210120083348.GM4903@dell \
--to=lee.jones@linaro.org \
--cc=afaerber@suse.de \
--cc=broonie@kernel.org \
--cc=cristian.ciocaltea@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-actions@lists.infradead.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=robh+dt@kernel.org \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).