| 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: Fri, 16 Dec 2022 18:17:52 +0000
From: Andre Przywara <andre.przywara@....com>
To: Martin Botka <martin.botka@...ainline.org>
Cc: martin.botka1@...il.com,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>,
Marijn Suijten <marijn.suijten@...ainline.org>,
Jami Kettunen <jamipkettunen@...ainline.org>,
Paul Bouchara <paul.bouchara@...ainline.org>,
Jan Trmal <jtrmal@...il.com>, Lee Jones <lee@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Chen-Yu Tsai <wens@...e.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC
On Wed, 14 Dec 2022 20:03:04 +0100
Martin Botka <martin.botka@...ainline.org> wrote:
Hi Martin,
> AXP1530 is a PMIC chip produced by X-Powers and an be connected via
> I2C bus.
> Where AXP313A seems to be closely related so the same driver can be used and
> seen it only paired with H616 SoC.
So as mentioned, I am pretending this is for the AXP313A now, looking at
its datasheet.
Of course the elephant in the room is s/AXP1530/AXP313A/, but other than
that:
>
> Signed-off-by: Martin Botka <martin.botka@...ainline.org>
> ---
> drivers/mfd/axp20x-i2c.c | 2 ++
> drivers/mfd/axp20x.c | 62 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> 3 files changed, 96 insertions(+)
>
> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> index 8fd6727dc30a..6bfb931a580e 100644
> --- a/drivers/mfd/axp20x-i2c.c
> +++ b/drivers/mfd/axp20x-i2c.c
> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct i2c_client *i2c)
> #ifdef CONFIG_OF
> static const struct of_device_id axp20x_i2c_of_match[] = {
> { .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
> + { .compatible = "x-powers,axp1530", .data = (void *)AXP1530_ID},
> { .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
> { .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> { .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>
> static const struct i2c_device_id axp20x_i2c_id[] = {
> { "axp152", 0 },
> + { "axp1530", 0 },
> { "axp202", 0 },
> { "axp209", 0 },
> { "axp221", 0 },
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 880c41fa7021..6caa7e87ad80 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -34,6 +34,7 @@
>
> static const char * const axp20x_model_names[] = {
> "AXP152",
> + "AXP1530",
> "AXP202",
> "AXP209",
> "AXP221",
> @@ -66,6 +67,24 @@ static const struct regmap_access_table axp152_volatile_table = {
> .n_yes_ranges = ARRAY_SIZE(axp152_volatile_ranges),
> };
>
> +static const struct regmap_range axp1530_writeable_ranges[] = {
> + regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
Where does this FREQUENCY register come from? BSP source? Is that the
lost register to set the PWM frequency?
The 313 datasheet doesn't mention it, and since we deny programming the
frequency, I would just leave it out.
If people find it existing (and useful!) later on, we should be able to
add it without breaking anything.
> +};
> +
> +static const struct regmap_range axp1530_volatile_ranges[] = {
> + regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
> +};
> +
> +static const struct regmap_access_table axp1530_writeable_table = {
> + .yes_ranges = axp1530_writeable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
> +};
> +
> +static const struct regmap_access_table axp1530_volatile_table = {
> + .yes_ranges = axp1530_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
> +};
> +
> static const struct regmap_range axp20x_writeable_ranges[] = {
> regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
> regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
> @@ -245,6 +264,15 @@ static const struct regmap_config axp152_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> +static const struct regmap_config axp1530_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .wr_table = &axp1530_writeable_table,
> + .volatile_table = &axp1530_volatile_table,
> + .max_register = AXP1530_FREQUENCY,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> static const struct regmap_config axp20x_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -304,6 +332,16 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
> INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT, 2, 0),
> };
>
> +static const struct regmap_irq axp1530_regmap_irqs[] = {
> + INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
> + INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
> + INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
> + INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),
Are those identifiers from the BSP source? The (translated) manual gives
some explanation, namely:
PWRON key rising edge
PWRON key falling edge
Short press the PWRON button
Long press the PWRON button
So I'd suggest we follow the existing naming:
PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
Or come up with names that people could actually decipher ;-)
> + INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
> + INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
> + INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
> +};
> +
> static const struct regmap_irq axp20x_regmap_irqs[] = {
> INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V, 0, 7),
> INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN, 0, 6),
> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
> .num_regs = 3,
> };
>
> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
> + .name = "axp1530_irq_chip",
> + .status_base = AXP1530_IRQ_STATUS1,
> + .ack_base = AXP1530_IRQ_STATUS1,
> + .mask_base = AXP1530_IRQ_ENABLE1,
> + .mask_invert = true,
> + .init_ack_masked = true,
> + .irqs = axp1530_regmap_irqs,
> + .num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
> + .num_regs = 1,
> +};
> +
> static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> .name = "axp20x_irq_chip",
> .status_base = AXP20X_IRQ1_STATE,
> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] = {
> },
> };
>
> +static struct mfd_cell axp1530_cells[] = {
> + {
> + .name = "axp20x-regulator",
> + },
> +};
> +
> static const struct resource axp288_adc_resources[] = {
> DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> };
> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> axp20x->regmap_cfg = &axp152_regmap_config;
> axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
> break;
> + case AXP1530_ID:
> + axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
> + axp20x->cells = axp1530_cells;
> + axp20x->regmap_cfg = &axp1530_regmap_config;
> + axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
> + break;
> case AXP202_ID:
> case AXP209_ID:
> axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 9ab0e2fca7ea..cad25754500f 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -12,6 +12,7 @@
>
> enum axp20x_variants {
> AXP152_ID = 0,
> + AXP1530_ID,
> AXP202_ID,
> AXP209_ID,
> AXP221_ID,
> @@ -45,6 +46,18 @@ enum axp20x_variants {
> #define AXP152_DCDC_FREQ 0x37
> #define AXP152_DCDC_MODE 0x80
>
> +#define AXP1530_ON_INDICATE 0x00
> +#define AXP1530_OUTPUT_CONTROL 0x10
> +#define AXP1530_DCDC1_CONRTOL 0x13
> +#define AXP1530_DCDC2_CONRTOL 0x14
> +#define AXP1530_DCDC3_CONRTOL 0x15
> +#define AXP1530_ALDO1_CONRTOL 0x16
> +#define AXP1530_DLDO1_CONRTOL 0x17
> +#define AXP1530_OUTOUT_MONITOR 0x1D
Shall this read AXP1530_OUTPUT_MONITOR?
> +#define AXP1530_IRQ_ENABLE1 0x20
> +#define AXP1530_IRQ_STATUS1 0x21
There is only one interrupt register, so can we drop the trailing number?
> +#define AXP1530_FREQUENCY 0x87
As mentioned, the manual does not mention it, and we don't use it anyway.
> +
> #define AXP20X_PWR_INPUT_STATUS 0x00
> #define AXP20X_PWR_OP_MODE 0x01
> #define AXP20X_USB_OTG_STATUS 0x02
> @@ -287,6 +300,15 @@ enum axp20x_variants {
> #define AXP288_FG_TUNE5 0xed
>
> /* Regulators IDs */
> +enum {
> + AXP1530_DCDC1 = 0,
> + AXP1530_DCDC2,
> + AXP1530_DCDC3,
> + AXP1530_LDO1,
> + AXP1530_LDO2,
I guess we should add the RTC LDO as LDO3 here.
The rest of the numbers match with the datasheet.
Cheers,
Andre
> + AXP1530_REG_ID_MAX,
> +};
> +
> enum {
> AXP20X_LDO1 = 0,
> AXP20X_LDO2,
> @@ -440,6 +462,16 @@ enum {
> AXP152_IRQ_GPIO0_INPUT,
> };
>
> +enum axp1530_irqs {
> + AXP1530_IRQ_TEMP_OVER,
> + AXP1530_IRQ_DCDC2_UNDER = 2,
> + AXP1530_IRQ_DCDC3_UNDER,
> + AXP1530_IRQ_POKLIRQ_EN,
> + AXP1530_IRQ_POKSIRQ_EN,
> + AXP1530_IRQ_KEY_L2H_EN,
> + AXP1530_IRQ_KEY_H2L_EN,
> +};
> +
> enum {
> AXP20X_IRQ_ACIN_OVER_V = 1,
> AXP20X_IRQ_ACIN_PLUGIN,
Powered by blists - more mailing lists