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

[Lee Jones <lee@kernel.org>]

On Thu, 24 Apr 2025, Laurent Pinchart wrote:

> On Thu, Apr 24, 2025 at 05:38:30PM +0100, Lee Jones wrote:
> > On Thu, 24 Apr 2025, Laurent Pinchart wrote:
> > > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote:
> > > > On Tue, 15 Apr 2025, 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.
> > > > > 
> > > > > 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(-)
> > > > 
> > > > [...]
> > > > 
> > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the
> > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to
> > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18.
> > > > > + */
> > > > > +#define ADP5589_BANK(n)			((n) >> 3)
> > > > > +#define ADP5589_BIT(n)			BIT((n) & 0x7)
> > > > > +
> > > > > +struct adp5585_regs {
> > > > > +	unsigned int debounce_dis_a;
> > > > > +	unsigned int rpull_cfg_a;
> > > > > +	unsigned int gpo_data_a;
> > > > > +	unsigned int gpo_out_a;
> > > > > +	unsigned int gpio_dir_a;
> > > > > +	unsigned int gpi_stat_a;
> > > > > +	unsigned int pwm_cfg;
> > > > > +	unsigned int pwm_offt_low;
> > > > > +	unsigned int pwm_ont_low;
> > > > > +	unsigned int gen_cfg;
> > > > > +	unsigned int ext_cfg;
> > > > > +};
> > > > > +
> > > > > +struct adp5585_info {
> > > > > +	const struct mfd_cell *adp5585_devs;
> > > > 
> > > > Okay, we are never doing this.  Either use OF for platform registration
> > > > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF.
> > > 
> > > When I upstreamed the initial driver, I modelled the different functions
> > > through child nodes in DT, with a compatible string for each child. I
> > > was told very strongly to remove that. We have therefore no other choice
> > > than constructing the name of the cells based on the model of the main
> > > device.
> > 
> > It's okay to add this information statically in this driver.  It's not
> > okay to then pass it through the OF API.  You can pass an identifier
> > through the .data attribute to match on, but we are not passing MFD cell
> > data through like this.
> 
> Sorry, I'm not following you. What's the issue with the .data field
> pointing to an instance of a structure that lists properties related to
> the device model ?

There isn't one.  By all means place any type of platform data you want
in there.  Similar to the information you'd find in Device Tree or the
old board-files type pdata.  You can even extract the platform data you
pass through the OF API and place it into MFD platform data.  The line
is being drawn on passing through one type of initialisation API with
another, MFD through OF in this case.  MFD cells containing device
registration data (including platform data!) is not itself platform
data.

> > > > > +	const struct regmap_config *regmap_config;
> > > > > +	const struct adp5585_regs *regs;
> > > > > +	unsigned int n_devs;
> > > > > +	unsigned int id;
> > > > 
> > > > What ID is this?  We already have platform IDs and MFD cell IDs.
> > > 
> > > That's the value of the hardware model ID read-only register, it is used
> > > as a safety check to verify that the connected device corresponds to the
> > > compatible string.
> > 
> > I suggest changing the nomenclature to be more forthcoming.
> > 
> > 'model', 'version', 'hwid', 'chipid', etc.
> > 
> > Why is it being stored?  Is it used to match on at a later date?
> 
> The adp5585_info structure contains static information the describe each
> device model. There's one global static const instance per device model,
> and they are referenced from device id structures (e.g. of_device_id).
> The driver gets an info pointer corresponding to the model reported by
> the platform firmware (e.g. DT). It reads the device ID from the device
> at probe time, and compares it with the value stored in the structure as
> a safety check to ensure there's no mismatch.

I think the current implementation (as a whole, not just the IDs) needs
a rethink.  Very few attributes are changing here, both between the 2
platforms and the several variants you're trying to support, leading to
masses of repetition.

Looking at the static configuration here, this is starting to look like
2 pieces of hardware with the only variation within each being the
default register values.  Is that a correct assumption?  If so, does
mean all of this added complexity is just to configure a few register
values such that the two platforms can be used for different things?  Or
are these really 6 true hardware variants of one another?

Either way, this approach doesn't scale.  Instead of multiplying the
amount of platforms / variants together and creating that number of
static structs, I'd suggest using templating and only adapting what
actually changes.

For instance, the following attributes in 'struct regmap_config' never
change; reg_bits, val_bits, and cache_type.  And max_register only
changes between the 2 hardware platforms.  The reg_defaults_raw values
can be changed in a switch statement.

Same goes for 'struct adp5585_info'.  Only regmap_config changes between
variants.  Everything else is exactly the same.  So, with the use of a
few of templates and a couple of succinct switch cases, you can control
all of the differentiation you need.  And for every variant you wish to
add, it's a couple of extra lines rather than many, leading to a
much more scaleable implementation.

-- 
Lee Jones [李琼斯]