| 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
| ||
|
Date: Tue, 22 Aug 2023 10:18:08 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jiansheng Wu <jiansheng.wu@...soc.com>, Lee Jones <lee@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Orson Zhai <orsonzhai@...il.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
Chunyan Zhang <zhang.lyra@...il.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
yongzhi.chen@...soc.com, xiaoqing.wu@...soc.com,
jinfeng.lin1@...soc.com, jianshengwu16@...il.com
Subject: Re: [PATCH 2/2] mfd: sprd-sc27xx-spi: Add PMICs support for UMS9621
SoC
On 22/08/2023 09:51, Jiansheng Wu wrote:
> There are three PMICs (UMP9620/21/22) on Unisoc's UMS9621 chip.
> UMP9620 is a master PMIC, the others are slave ones. Slave PMICs
> don't have irq functions, which is different from master device,
> such as SC27xx series and UMP9620, etc.
>
> Signed-off-by: Jiansheng Wu <jiansheng.wu@...soc.com>
> ---
> drivers/mfd/sprd-sc27xx-spi.c | 104 +++++++++++++++++++++++++---------
> 1 file changed, 77 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
> index d21f32cc784d..aa91301568a9 100644
> --- a/drivers/mfd/sprd-sc27xx-spi.c
> +++ b/drivers/mfd/sprd-sc27xx-spi.c
> @@ -24,6 +24,10 @@
> #define SPRD_SC2731_IRQ_BASE 0x140
> #define SPRD_SC2731_IRQ_NUMS 16
> #define SPRD_SC2731_CHG_DET 0xedc
> +#define SPRD_UMP9620_IRQ_BASE 0x80
> +#define SPRD_UMP9620_IRQ_NUMS 11
> +#define SPRD_UMP9621_SLAVE_ID 0x8000
> +#define SPRD_UMP9622_SLAVE_ID 0xc000
>
> /* PMIC charger detection definition */
> #define SPRD_PMIC_CHG_DET_DELAY_US 200000
> @@ -45,6 +49,7 @@ struct sprd_pmic {
> };
>
> struct sprd_pmic_data {
> + u32 slave_id;
See coding style about such wording. You know, it is not 2010 anymore...
> u32 irq_base;
> u32 num_irqs;
> u32 charger_det;
> @@ -67,6 +72,19 @@ static const struct sprd_pmic_data sc2731_data = {
> .charger_det = SPRD_SC2731_CHG_DET,
> };
>
> +static const struct sprd_pmic_data ump9620_data = {
> + .irq_base = SPRD_UMP9620_IRQ_BASE,
> + .num_irqs = SPRD_UMP9620_IRQ_NUMS,
> +};
> +
> +static const struct sprd_pmic_data ump9621_data = {
> + .slave_id = SPRD_UMP9621_SLAVE_ID,
> +};
> +
> +static const struct sprd_pmic_data ump9622_data = {
> + .slave_id = SPRD_UMP9622_SLAVE_ID,
> +};
> +
> enum usb_charger_type sprd_pmic_detect_charger_type(struct device *dev)
> {
> struct spi_device *spi = to_spi_device(dev);
> @@ -108,8 +126,27 @@ 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);
> + const struct sprd_pmic_data *pdata;
> + int ret;
> + u32 *pmdata;
> +
> + if (!pdata->slave_id) {
> + ret = spi_write(spi, data, count);
> + } else {
> + pdata = ((struct sprd_pmic *)spi_get_drvdata(spi))->pdata;
> +
> + pmdata = kzalloc(count, GFP_KERNEL);
> + if (!pmdata)
> + return -ENOMEM;
> + memcpy(pmdata, data, count);
> + *pmdata += pdata->slave_id;
> + ret = spi_write(spi, (const void *)pmdata, count);
> + kfree(pmdata);
> + }
> + if (ret)
> + pr_err("pmic mfd write failed!\n");
>
> - return spi_write(spi, data, count);
> + return ret;
> }
>
> static int sprd_pmic_spi_read(void *context,
> @@ -118,6 +155,7 @@ static int sprd_pmic_spi_read(void *context,
> {
> struct device *dev = context;
> struct spi_device *spi = to_spi_device(dev);
> + const struct sprd_pmic_data *pdata;
> u32 rx_buf[2] = { 0 };
> int ret;
>
> @@ -125,11 +163,16 @@ static int sprd_pmic_spi_read(void *context,
> if (reg_size != sizeof(u32) || val_size != sizeof(u32))
> return -EINVAL;
>
> + pdata = ((struct sprd_pmic *)spi_get_drvdata(spi))->pdata;
> /* Copy address to read from into first element of SPI buffer. */
> memcpy(rx_buf, reg, sizeof(u32));
> + if (!pdata->slave_id)
> + rx_buf[0] += pdata->slave_id;
> ret = spi_read(spi, rx_buf, 1);
> - if (ret < 0)
> + if (ret < 0) {
> + pr_err("pmic mfd read failed!\n");
Eh... drivers do not use pr_ but dev_. Which you can easily see within
this driver, so before posting changes please look at the driver and its
style, then learn from it and use similar coding convention.
> return ret;
> + }
>
> memcpy(val, rx_buf, val_size);
> return 0;
...
>
> ret = devm_of_platform_populate(&spi->dev);
> @@ -240,6 +284,9 @@ static DEFINE_SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops,
> static const struct of_device_id sprd_pmic_match[] = {
> { .compatible = "sprd,sc2730", .data = &sc2730_data },
> { .compatible = "sprd,sc2731", .data = &sc2731_data },
> + { .compatible = "sprd,ump9620", .data = &ump9620_data },
> + { .compatible = "sprd,ump9621", .data = &ump9621_data },
> + { .compatible = "sprd,ump9622", .data = &ump9622_data },
> {},
> };
> MODULE_DEVICE_TABLE(of, sprd_pmic_match);
> @@ -247,6 +294,9 @@ MODULE_DEVICE_TABLE(of, sprd_pmic_match);
> static const struct spi_device_id sprd_pmic_spi_ids[] = {
> { .name = "sc2730", .driver_data = (unsigned long)&sc2730_data },
> { .name = "sc2731", .driver_data = (unsigned long)&sc2731_data },
> + { .name = "ump9620", .driver_data = (unsigned long)&ump9620_data },
> + { .name = "ump9621", .driver_data = (unsigned long)&ump9621_data },
> + { .name = "ump9622", .driver_data = (unsigned long)&ump9622_data },
So here you sneaked new compatibles... Sorry, adding new compatibles is
not the same as converting old ones. Entirely separate patch.
Best regards,
Krzysztof
Powered by blists - more mailing lists