Received-SPF: pass (google.com: domain of linux-kernel+bounces-46302-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 31 Jan 2024 11:03:11 +0000
From: Lee Jones <lee@kernel.org>
To: Karel Balej <karelb@gimli.ms.mff.cuni.cz>
Cc: Karel Balej <balejk@matfyz.cz>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, linux-input@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Duje =?utf-8?Q?Mihanovi=C4=87?= <duje.mihanovic@skole.hr>,
	~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org
Subject: Re: [RFC PATCH 2/5] mfd: add 88pm88x driver
Message-ID: <20240131110311.GI8551@google.com>
References: <20231217131838.7569-1-karelb@gimli.ms.mff.cuni.cz>
 <20231217131838.7569-3-karelb@gimli.ms.mff.cuni.cz>
 <20240125122634.GE74950@google.com>
 <CYQ8MEQRJELE.2GB9316NZA998@gimli.ms.mff.cuni.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CYQ8MEQRJELE.2GB9316NZA998@gimli.ms.mff.cuni.cz>

On Sun, 28 Jan 2024, Karel Balej wrote:
> > > +	/* 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.

This is not a Pinctrl driver.

Isn't there a generic API you can use?

> > > +	/* 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.

If you don't know that they are or what they do and you haven't tested
them, they should not be submitted upstream.

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

One device doesn't warrant an MFD.  Please add more devices.

-- 
Lee Jones [李琼斯]