Lee, thank you for your feedback. On Thu Jan 25, 2024 at 1:26 PM CET, Lee Jones wrote: [...] > > +#define PM88X_REG_INT_STATUS1 0x05 > > + > > +#define PM88X_REG_INT_ENA_1 0x0a > > +#define PM88X_INT_ENA1_ONKEY BIT(0) > > + > > +enum pm88x_irq_number { > > + PM88X_IRQ_ONKEY, > > + > > + PM88X_MAX_IRQ > > +}; > > An enum for a single IRQ? There will be a lot more IRQs but I have only added this one so far as it is the only one used by this series -- is that OK? > > +static struct reg_sequence pm886_presets[] = { > > + /* disable watchdog */ > > + REG_SEQ0(PM88X_REG_WDOG, 0x01), > > Easier to read if you place spaces between them. > > > + /* GPIO1: DVC, GPIO0: input */ > > + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40), > > Shouldn't you set these up using Pintrl? You mean to add a new MFD cell for the pins and write the respective driver? The downstream implementation has no such thing so I'm not sure if I would be able to do that from scratch. > > + /* GPIO2: input */ > > + REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00), > > + /* DVC2, DVC1 */ > > Please unify all of the comments. > > They all use a different structure. > > > + REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44), > > + /* GPIO5V_1:input, GPIO5V_2: input */ > > + REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00), > > + /* output 32 kHz from XO */ > > + REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a), > > + /* OSC_FREERUN = 1, to lock FLL */ > > + REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f), > > + /* XO_LJ = 1, enable low jitter for 32 kHz */ > > + REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20), > > + /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */ > > + REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8), > > + /* set the duty cycle of charger DC/DC to max */ > > + REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0), > > These all looks like they should be handled in their respective drivers? > > "patch"ing these in seems like a hack. To be honest, I don't really know why these are required and what effect they have -- the comments above taken from the downstream version are the only thing I have to go by. I might try removing them to see if there is any noticable change and whether they could be added only later with the respective drivers. > > > +}; > > Why this instead of What are you refering to here please? > > +static struct resource onkey_resources[] = { > > + DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"), > > +}; > > + > > +static struct mfd_cell pm88x_devs[] = { > > + { > > + .name = "88pm88x-onkey", > > + .num_resources = ARRAY_SIZE(onkey_resources), > > + .resources = onkey_resources, > > + .id = -1, > > + }, > > +}; > > It's not an MFD if it only supports a single device. As I have noted above with respect to the IRQ enum and also in the commit message, I have so far only added the parts which there is already use for. I intend to add the other parts along with the respective subdevice drivers, please see my regulator series [1] for an example. I thought this approach would make for shorter and simpler patches and also would allow me to make more informed decisions as I familiarize myself with the downstream subdevice drivers more closely one by one. > > + i2c_set_clientdata(client, chip); > > + > > + device_init_wakeup(&client->dev, 1); > > + > > + chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, &pm88x_i2c_regmap); > > + if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) { > > Just define different regmaps if you really need them. You mean not to use an array of regmaps but add new struct members instead? One for each regmap? > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h > > new file mode 100644 > > index 000000000000..a34c57447827 > > --- /dev/null > > +++ b/include/linux/mfd/88pm88x.h [...] > > +#define PM88X_REG_ID 0x00 > > + > > +#define PM88X_REG_STATUS1 0x01 > > +#define PM88X_ONKEY_STS1 BIT(0) > > + > > +#define PM88X_REG_MISC_CONFIG1 0x14 > > +#define PM88X_SW_PDOWN BIT(5) > > + > > +#define PM88X_REG_MISC_CONFIG2 0x15 > > +#define PM88X_INT_INV BIT(0) > > +#define PM88X_INT_CLEAR BIT(1) > > +#define PM88X_INT_RC 0x00 > > +#define PM88X_INT_WC BIT(1) > > +#define PM88X_INT_MASK_MODE BIT(2) > > + > > +#define PM88X_REG_WDOG 0x1d > > + > > +#define PM88X_REG_LOWPOWER2 0x21 > > +#define PM88X_REG_LOWPOWER4 0x23 > > + > > +#define PM88X_REG_GPIO_CTRL1 0x30 > > These don't really need to be spaced out, do they? I have spaced them out already as I expect to add some related definitions to each of these in the future and thought it would then perhaps be more easily readable like this. [1] https://lore.kernel.org/all/20231228100208.2932-1-karelb@xxxxxxxxxxxxxxxxxxxx/ Kind regards, K. B.