Re: [PATCH v2 06/17] mfd: adp5585: add support for adp5589

[Lee Jones <lee@kernel.org>]

On Tue, 22 Apr 2025, Laurent Pinchart wrote:

> Hi Nuno,
> 
> On Mon, Apr 21, 2025 at 01:21:08PM +0100, Nuno Sá wrote:
> > On Mon, 2025-04-21 at 12:15 +0300, Laurent Pinchart wrote:
> > > On Tue, Apr 15, 2025 at 03:49:22PM +0100, Nuno Sá via B4 Relay wrote:
> > > > From: Nuno Sá <nuno.sa@analog.com>
> > > > 
> > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder,
> > > > programmable logic, reset generator, and PWM generator.
> > > > 
> > > > This patch adds the foundation to add support for the adp5589 gpio and pwm
> > > > drivers. Most importantly, we need to differentiate between some
> > > > registers addresses. It also hints to future keymap support.
> > > 
> > > Please split this in two patches, one that reworks the driver to support
> > > different register addresses, and one that adds adp5589 support.
> > > 
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > ---
> > > >  drivers/mfd/adp5585.c       | 223 +++++++++++++++++++++++++++++++++++++++++---
> > > >  include/linux/mfd/adp5585.h |  57 ++++++++++-
> > > >  2 files changed, 268 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> > > > index fafe3ad93ea196e1eb8e79fecba58f36f12167eb..c3586c0d6aa2e7e7d94667993410610be7fc3672 100644
> > > > --- a/drivers/mfd/adp5585.c
> > > > +++ b/drivers/mfd/adp5585.c
> > > > @@ -25,6 +25,13 @@ static const struct mfd_cell adp5585_devs[] = {
> > > >  
> > > >  };
> > > >  
> > > > +static const struct mfd_cell adp5589_devs[] = {
> > > > +	MFD_CELL_NAME("adp5589-keys"),
> > > > +	MFD_CELL_NAME("adp5589-gpio"),
> > > > +	MFD_CELL_NAME("adp5589-pwm"),
> > > > +
> > > > +};
> > > > +
> > > >  static const struct regmap_range adp5585_volatile_ranges[] = {
> > > >  	regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B),
> > > >  };
> > > > @@ -34,6 +41,15 @@ static const struct regmap_access_table adp5585_volatile_regs = {
> > > >  	.n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges),
> > > >  };
> > > >  
> > > > +static const struct regmap_range adp5589_volatile_ranges[] = {
> > > > +	regmap_reg_range(ADP5585_ID, ADP5589_GPI_STATUS_C),
> > > > +};
> > > > +
> > > > +static const struct regmap_access_table adp5589_volatile_regs = {
> > > > +	.yes_ranges = adp5589_volatile_ranges,
> > > > +	.n_yes_ranges = ARRAY_SIZE(adp5589_volatile_ranges),
> > > > +};
> > > > +
> > > >  /*
> > > >   * Chip variants differ in the default configuration of pull-up and pull-down
> > > >   * resistors, and therefore have different default register values:
> > > > @@ -77,10 +93,52 @@ static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = {
> > > >  	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00,
> > > >  };
> > > >  
> > > > +static const u8 adp5589_regmap_defaults_00[ADP5589_MAX_REG + 1] = {
> > > > +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +};
> > > > +
> > > > +static const u8 adp5589_regmap_defaults_01[ADP5589_MAX_REG + 1] = {
> > > > +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00,
> > > > +	/* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00,
> > > > +};
> > > > +
> > > > +static const u8 adp5589_regmap_defaults_02[ADP5589_MAX_REG + 1] = {
> > > > +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x18 */ 0x00, 0x41, 0x01, 0x00, 0x11, 0x04, 0x00, 0x00,
> > > > +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +	/* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > +};
> > > > +
> > > >  enum adp5585_regmap_type {
> > > >  	ADP5585_REGMAP_00,
> > > >  	ADP5585_REGMAP_02,
> > > >  	ADP5585_REGMAP_04,
> > > > +	ADP5589_REGMAP_00,
> > > > +	ADP5589_REGMAP_01,
> > > > +	ADP5589_REGMAP_02,
> > > >  };
> > > >  
> > > >  static const struct regmap_config adp5585_regmap_configs[] = {
> > > > @@ -111,6 +169,131 @@ static const struct regmap_config adp5585_regmap_configs[] = {
> > > >  		.reg_defaults_raw = adp5585_regmap_defaults_04,
> > > >  		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04),
> > > >  	},
> > > > +	[ADP5589_REGMAP_00] = {
> > > > +		.reg_bits = 8,
> > > > +		.val_bits = 8,
> > > > +		.max_register = ADP5589_MAX_REG,
> > > > +		.volatile_table = &adp5589_volatile_regs,
> > > > +		.cache_type = REGCACHE_MAPLE,
> > > > +		.reg_defaults_raw = adp5589_regmap_defaults_00,
> > > > +		.num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_00),
> > > > +	},
> > > > +	[ADP5589_REGMAP_01] = {
> > > > +		.reg_bits = 8,
> > > > +		.val_bits = 8,
> > > > +		.max_register = ADP5589_MAX_REG,
> > > > +		.volatile_table = &adp5589_volatile_regs,
> > > > +		.cache_type = REGCACHE_MAPLE,
> > > > +		.reg_defaults_raw = adp5589_regmap_defaults_01,
> > > > +		.num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_01),
> > > > +	},
> > > > +	[ADP5589_REGMAP_02] = {
> > > > +		.reg_bits = 8,
> > > > +		.val_bits = 8,
> > > > +		.max_register = ADP5589_MAX_REG,
> > > > +		.volatile_table = &adp5589_volatile_regs,
> > > > +		.cache_type = REGCACHE_MAPLE,
> > > > +		.reg_defaults_raw = adp5589_regmap_defaults_02,
> > > > +		.num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_02),
> > > > +	},
> > > > +};
> > > > +
> > > > +static const struct adp5585_regs adp5585_regs = {
> > > > +	.debounce_dis_a = ADP5585_DEBOUNCE_DIS_A,
> > > > +	.rpull_cfg_a = ADP5585_RPULL_CONFIG_A,
> > > > +	.gpo_data_a = ADP5585_GPO_DATA_OUT_A,
> > > > +	.gpo_out_a = ADP5585_GPO_OUT_MODE_A,
> > > > +	.gpio_dir_a = ADP5585_GPIO_DIRECTION_A,
> > > > +	.gpi_stat_a = ADP5585_GPI_STATUS_A,
> > > > +	.pwm_cfg = ADP5585_PWM_CFG,
> > > > +	.pwm_offt_low = ADP5585_PWM_OFFT_LOW,
> > > > +	.pwm_ont_low = ADP5585_PWM_ONT_LOW,
> > > > +	.gen_cfg = ADP5585_GENERAL_CFG,
> > > > +	.ext_cfg = ADP5585_PIN_CONFIG_C,
> > > > +};
> > > 
> > > Why does this need to be stored in this driver, and not in the drivers
> > > for the gpio and pwm cells ? If the kernel is compiled without e.g. the
> > > adp5585-pwm driver, we shouldn't waste memory here by adding data that
> > > only the adp5585-pwm driver needs.
> > 
> > I don't really think the memory we would save to be that relevant but I can
> > better separate things. I guess i went like this because there's some shared
> > variables that will have to be in the top level structs and I did not wanted to
> > have a "global" and "local" regs thingy...

Memory and power savings are very important.

Please prioritise them over effort.

> I understand, and I think it's at least partly a coding style
> preference. Personally, I find that having child-specific data in child
> drivers makes the code easier to read, as it increases locality.
> Otherwise, I have to look through multiple child drivers to see if and
> where each field is used.

Right.  If resources are only used in specific sub-devices, please move
it there.  Only shared resources should be handed in the parent.

-- 
Lee Jones [李琼斯]