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. > > 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, > >> > > > > >
