On January 13, 2023 1:35:55 AM GMT+01:00, Andre Przywara <andre.przywara@xxxxxxx> wrote: >On Tue, 10 Jan 2023 01:32:58 +0100 >Martin Botka <martin.botka@xxxxxxxxxxxxxx> wrote: > >Hi Martin, > >> On Tue, Jan 10 2023 at 12:00:25 AM +00:00:00, Andre Przywara >> <andre.przywara@xxxxxxx> wrote: >> > On Sat, 17 Dec 2022 01:13:01 +0100 >> > Martin Botka <martin.botka@xxxxxxxxxxxxxx> wrote: >> > >> > Hi Martin, >> Hello Andre, >> > >> > hope you had a good break! Did you have any chance to come back to >> > this >> > again? Now would be a good time to send a new version, otherwise it's >> > getting pretty tight for v6.3 already. >> > >> I did have a good break :) >> I unfortunately did not. Was bit busy with issues to one of my projects. >> Will see if I can sort this out tomorrow but you said you already have >> AXP313A driver >> ready. So it may be better if you send your driver. > >Well, so my patch just compiled, and it wasn't as complete as yours. >Also I have no means of testing it. Also you were first with the patch, >so deserve the credits. > >Since you seem to be busy in the next days, I could offer to address >the comments myself (since you acked them), and send a v6 on >your behalf. I would need to rely on you for testing, maybe Junari >(from #linux-sunxi) could also help out here. That is very kind of you. But please add yourself as co-author or something like that so your contribution isn't without credit. As for testing I can after 21:30UTC+1. I should be on IRC after that hopefully. Cheers, Martin > >> > On Friday, "junari" in the #linux-sunxi IRC channel, made some >> > interesting discovery: he is playing around with an AXP313a on some >> > H616 device and figured that DCDC3 is not behaving like the datasheet: >> > https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-06#31784528; >> > He later confirmed the voltage: >> > https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-08#31788373; >> > >> Ah interesting. What i also found out right after signing off is from a >> friend at BIQU >> that the AXP1530 is the AXP313A. It is the same chip. The only >> difference being that AXP1530 >> is the internal naming AXP uses. I mean we kinda figured that one out >> but its nice to have >> confirmation on this :) > >Indeed, thanks for that. Aside from the BSP driver, there doesn't seem >to be many records of the AXP1530, so I would go with the AXP313a name, >and leave the AXP1530 for the trivia section. > >Cheers, >Andre > > >> > Basically it looks like the DCDC3 parameters you harvested from the >> > BSP >> > code seem to be correct after all. Do you have any chance to measure >> > the voltage? >> I wish i had. Dont think my multimeter would be able to be so precise. >> Will try to source some oscilloscope but that wont be in time >> unfortunately. >> > If not, can we try to deduce what the right settings are? The voltage >> > difference seems to be significant (860mV vs 1200mV), I wonder if any >> > device connected there (DRAM?) would work with the wrong setting? >> I will try to be around in IRC sunxi channel "today" from around >> 13:00UTC+1. >> Would be prob best to discuss this there and then share our findings >> here on LKML :) >> >> Cheers, >> Martin >> > >> > Cheers, >> > Andre >> > >> >> On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara >> >> <andre.przywara@xxxxxxx> wrote: >> >> >On Wed, 14 Dec 2022 20:03:04 +0100 >> >> >Martin Botka <martin.botka@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> >> >> >> --- >> >> >> 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. >> >> >> >> BSP. Ack. >> >> > >> >> >> +}; >> >> >> + >> >> >> +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 ;-) >> >> > >> >> > >> >> Ack. >> >> >> + 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? >> >> > >> >> Yep. >> >> >> +#define AXP1530_FREQUENCY 0x87 >> >> > >> >> >As mentioned, the manual does not mention it, and we don't use it >> >> anyway. >> >> > >> >> Ack. >> >> >> + >> >> >> #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. >> >> > >> >> Ack. >> >> >> >> I will take some time off due to Uni so v6 will be delayed peobably >> >> last holidays. >> >> >> >> Best regards and happy holidays, >> >> Martin >> >> >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, >> >> > >> > >> >> >
