| 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: <CAMz4kuJ1SV0yv-mAbWiGZN15OoYxXap3-CEvVoDCkZwr7MJK9w@mail.gmail.com>
Date: Tue, 24 Oct 2017 17:59:07 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Lee Jones <lee.jones@...aro.org>
Cc: Baolin Wang <baolin.wang@...eadtrum.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v2 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver
Hi Lee,
On 24 October 2017 at 17:26, Lee Jones <lee.jones@...aro.org> wrote:
> On Mon, 16 Oct 2017, Baolin Wang wrote:
>
>> This patch adds support for Spreadtrum SC27xx series PMIC MFD core, and It
>> provides communication through the SPI interfaces. The SC27xx series PMICs
>> contains the following 6 major components:
>> - DCDCs
>> - LDOs
>> - Battery management system
>> - Audio codec
>> - User interface function, such as indicator, flash LED
>> - IC level function, such as power on/off, type-c
>>
>> Signed-off-by: Baolin Wang <baolin.wang@...eadtrum.com>
>> ---
>> Changes since v1:
>> - Re-order the including head files.
>> - Add one condition to check register size and value size when reading.
>> - Fix some coding style issues.
>> ---
>> drivers/mfd/Kconfig | 10 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/sprd-sc27xx-spi.c | 264 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 275 insertions(+)
>> create mode 100644 drivers/mfd/sprd-sc27xx-spi.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index fc5e4fe..4753cd5 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1057,6 +1057,16 @@ config MFD_SMSC
>> To compile this driver as a module, choose M here: the
>> module will be called smsc.
>>
>> +config MFD_SC27XX_PMIC
>> + tristate "Spreadtrum SC27xx PMICs"
>> + depends on ARCH_SPRD || COMPILE_TEST
>> + depends on SPI_MASTER
>> + select MFD_CORE
>> + select REGMAP_SPI
>> + select REGMAP_IRQ
>> + help
>> + This enables support for the Spreadtrum SC27xx PMICs.
>
> More information here please. This looks like a pretty big device.
> It deserves more than a single line of help text.
OK. I will add more information.
>
>> config ABX500_CORE
>> bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions"
>> default y if ARCH_U300 || ARCH_U8500 || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index c3d0a1b..a377e0f 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -226,3 +226,4 @@ obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
>> obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
>> obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
>> obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
>> +obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
>> diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
>> new file mode 100644
>> index 0000000..eb7ff5f
>> --- /dev/null
>> +++ b/drivers/mfd/sprd-sc27xx-spi.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Copyright (C) 2017 Spreadtrum Communications Inc.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#define SPRD_PMIC_INT_MASK_STATUS 0x0
>> +#define SPRD_PMIC_INT_RAW_STATU 0x4
>> +#define SPRD_PMIC_INT_EN 0x8
>> +
>> +struct sprd_pmic {
>> + struct regmap *regmap;
>> + struct device *dev;
>> + struct regmap_irq *irqs;
>> + struct regmap_irq_chip irq_chip;
>> + struct regmap_irq_chip_data *irq_data;
>> + int irq;
>> +};
>> +
>> +struct sprd_pmic_data {
>> + u32 irq_base;
>> + u32 num_irqs;
>> +};
>> +
>> +/*
>> + * Since different PMICs of SC27xx series can have different interrupt
>> + * base address and irq number, we should save irq number and irq base
>> + * in the device data structure.
>> + */
>> +static const struct sprd_pmic_data sc2731_data = {
>> + .irq_base = 0x140,
>
> Does IRQ 0x140 have a name?
>
> Probably best to define it.
OK. I will define 0x140 and 16 with readable macros.
>
>> + .num_irqs = 16,
>> +};
>> +
>> +static const struct mfd_cell sprd_pmic_devs[] = {
>> + {
>> + .name = "sc27xx-wdt",
>> + .of_compatible = "sprd,sc27xx-wdt",
>> + }, {
>> + .name = "sc27xx-rtc",
>> + .of_compatible = "sprd,sc27xx-rtc",
>> + }, {
>> + .name = "sc27xx-charger",
>> + .of_compatible = "sprd,sc27xx-charger",
>> + }, {
>> + .name = "sc27xx-chg-timer",
>> + .of_compatible = "sprd,sc27xx-chg-timer",
>> + }, {
>> + .name = "sc27xx-fast-chg",
>> + .of_compatible = "sprd,sc27xx-fast-chg",
>> + }, {
>> + .name = "sc27xx-chg-wdt",
>> + .of_compatible = "sprd,sc27xx-chg-wdt",
>> + }, {
>> + .name = "sc27xx-typec",
>> + .of_compatible = "sprd,sc27xx-typec",
>> + }, {
>> + .name = "sc27xx-flash",
>> + .of_compatible = "sprd,sc27xx-flash",
>> + }, {
>> + .name = "sc27xx-eic",
>> + .of_compatible = "sprd,sc27xx-eic",
>> + }, {
>> + .name = "sc27xx-efuse",
>> + .of_compatible = "sprd,sc27xx-efuse",
>> + }, {
>> + .name = "sc27xx-thermal",
>> + .of_compatible = "sprd,sc27xx-thermal",
>> + }, {
>> + .name = "sc27xx-adc",
>> + .of_compatible = "sprd,sc27xx-adc",
>> + }, {
>> + .name = "sc27xx-audio-codec",
>> + .of_compatible = "sprd,sc27xx-audio-codec",
>> + }, {
>> + .name = "sc27xx-regulator",
>> + .of_compatible = "sprd,sc27xx-regulator",
>> + }, {
>> + .name = "sc27xx-vibrator",
>> + .of_compatible = "sprd,sc27xx-vibrator",
>> + }, {
>> + .name = "sc27xx-keypad-led",
>> + .of_compatible = "sprd,sc27xx-keypad-led",
>> + }, {
>> + .name = "sc27xx-bltc",
>> + .of_compatible = "sprd,sc27xx-bltc",
>> + }, {
>> + .name = "sc27xx-fgu",
>> + .of_compatible = "sprd,sc27xx-fgu",
>> + }, {
>> + .name = "sc27xx-7sreset",
>> + .of_compatible = "sprd,sc27xx-7sreset",
>> + }, {
>> + .name = "sc27xx-poweroff",
>> + .of_compatible = "sprd,sc27xx-poweroff",
>> + },
>> +};
>> +
>> +static int sprd_pmic_spi_write(void *context, const void *data, size_t count)
>> +{
>> + struct device *dev = context;
>> + struct spi_device *spi = to_spi_device(dev);
>> +
>> + return spi_write(spi, data, count);
>> +}
>> +
>> +static int sprd_pmic_spi_read(void *context,
>> + const void *reg, size_t reg_size,
>> + void *val, size_t val_size)
>> +{
>> + struct device *dev = context;
>> + struct spi_device *spi = to_spi_device(dev);
>> + u32 rx_buf[2] = { 0 };
>> + int ret;
>> +
>> + /* Now we only support one PMIC register to read every time. */
>> + if (reg_size != sizeof(u32) || val_size != sizeof(u32))
>> + return -EINVAL;
>> +
>> + rx_buf[0] = *(u32 *)reg;
>
> This looks dodgy. You should not be doing raw memory manipulation.
>
> You need to be passing __iomem and using one of the predefined APIs
> (i.e. readl) on it.
Sorry, I did not get you here. The 'reg' parameter is just one PMIC
register offset which want to be read. We do not need to pass __iomem
to SPI master driver, we just pass one register offset address to our
SPI master driver then the SPI master driver will handle to read
values from our PMIC.
>
>> + ret = spi_read(spi, rx_buf, 1);
>> + if (ret < 0)
>> + return ret;
>> +
>> + memcpy(val, rx_buf, val_size);
>> + return 0;
>> +}
>> +
>> +static struct regmap_bus sprd_pmic_regmap = {
>> + .write = sprd_pmic_spi_write,
>> + .read = sprd_pmic_spi_read,
>> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
>> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
>> +};
>> +
>> +static const struct regmap_config sprd_pmic_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .max_register = 0xffff,
>> +};
>> +
>> +static int sprd_pmic_probe(struct spi_device *spi)
>> +{
>> + struct sprd_pmic *sprd_pmic;
>
> Would prefer that you called this ddata.
>
>> + const struct sprd_pmic_data *data;
>
> Would prefer that you called this pdata.
Sure.
>
>> + int ret, i;
>> +
>> + data = of_device_get_match_data(&spi->dev);
>> + if (!data) {
>> + dev_err(&spi->dev, "No matching driver data found\n");
>> + return -EINVAL;
>> + }
>> +
>> + sprd_pmic = devm_kzalloc(&spi->dev, sizeof(*sprd_pmic), GFP_KERNEL);
>> + if (!sprd_pmic)
>> + return -ENOMEM;
>> +
>> + sprd_pmic->regmap = devm_regmap_init(&spi->dev, &sprd_pmic_regmap,
>> + &spi->dev, &sprd_pmic_config);
>> + if (IS_ERR(sprd_pmic->regmap)) {
>> + ret = PTR_ERR(sprd_pmic->regmap);
>> + dev_err(&spi->dev, "Failed to allocate register map %d\n", ret);
>> + return ret;
>> + }
>> +
>> + spi_set_drvdata(spi, sprd_pmic);
>> + sprd_pmic->dev = &spi->dev;
>> + sprd_pmic->irq = spi->irq;
>> +
>> + sprd_pmic->irq_chip.name = dev_name(&spi->dev);
>> + sprd_pmic->irq_chip.status_base =
>> + data->irq_base + SPRD_PMIC_INT_MASK_STATUS;
>> + sprd_pmic->irq_chip.mask_base = data->irq_base + SPRD_PMIC_INT_EN;
>> + sprd_pmic->irq_chip.ack_base = 0;
>> + sprd_pmic->irq_chip.num_regs = 1;
>> + sprd_pmic->irq_chip.num_irqs = data->num_irqs;
>> + sprd_pmic->irq_chip.mask_invert = true;
>> +
>> + sprd_pmic->irqs = devm_kzalloc(&spi->dev, sizeof(struct regmap_irq) *
>> + data->num_irqs, GFP_KERNEL);
>> + if (!sprd_pmic->irqs)
>> + return -ENOMEM;
>> +
>> + sprd_pmic->irq_chip.irqs = sprd_pmic->irqs;
>> + for (i = 0; i < data->num_irqs; i++) {
>> + sprd_pmic->irqs[i].reg_offset = i / data->num_irqs;
>> + sprd_pmic->irqs[i].mask = BIT(i % data->num_irqs);
>> + }
>> +
>> + ret = devm_regmap_add_irq_chip(&spi->dev, sprd_pmic->regmap,
>> + sprd_pmic->irq,
>> + IRQF_ONESHOT | IRQF_NO_SUSPEND, 0,
>> + &sprd_pmic->irq_chip,
>> + &sprd_pmic->irq_data);
>> + if (ret) {
>> + dev_err(&spi->dev, "Failed to add PMIC irq chip %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = mfd_add_devices(&spi->dev, PLATFORM_DEVID_AUTO, sprd_pmic_devs,
>> + ARRAY_SIZE(sprd_pmic_devs), NULL, 0,
>> + regmap_irq_get_domain(sprd_pmic->irq_data));
>> + if (ret) {
>> + dev_err(&spi->dev, "Failed to register device %d\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int sprd_pmic_remove(struct spi_device *spi)
>> +{
>> + mfd_remove_devices(&spi->dev);
>
> Anything stopping you from using devm_*?
Ah, I missed devm_* and I will change to use devm_* in next version.
>
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sprd_pmic_match[] = {
>> + { .compatible = "sprd,sc2731", .data = &sc2731_data },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sprd_pmic_match);
>> +
>> +static struct spi_driver sprd_pmic_driver = {
>> + .driver = {
>> + .name = "sc27xx-pmic",
>> + .bus = &spi_bus_type,
>> + .of_match_table = sprd_pmic_match,
>
> of_match_ptr()?
The of_match_ptr() did nothing, so I think it is not necessary.
>
>> + },
>> + .probe = sprd_pmic_probe,
>> + .remove = sprd_pmic_remove,
>> +};
>> +
>> +static int __init sprd_pmic_init(void)
>> +{
>> + return spi_register_driver(&sprd_pmic_driver);
>> +}
>> +subsys_initcall(sprd_pmic_init);
>> +
>> +static void __exit sprd_pmic_exit(void)
>> +{
>> + spi_unregister_driver(&sprd_pmic_driver);
>> +}
>> +module_exit(sprd_pmic_exit);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("SPI support for SC27xx PMICs");
>
> "SPI support"? This is not an SPI driver.
OK, I will modify the driver description. Thanks for your comments.
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists