Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Jul 2022 15:19:12 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Markuss Broks <markuss.broks@gmail.com>
Cc:     linux-kernel@vger.kernel.org, phone-devel@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht,
        Matti Vaittinen <mazziesaccount@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD
Message-ID: <YtgO4J8BBqRnKPKs@google.com>
References: <20220423085319.483524-1-markuss.broks@gmail.com>
 <20220423085319.483524-5-markuss.broks@gmail.com>
 <Yqj+aUNLC00Tcu49@google.com>
 <5498bf71-66a5-957e-ed3d-13e68b982562@gmail.com>
 <YtUXFTx1+vSrXx70@google.com>
 <295e1809-f6f2-ca31-5c36-be133ffdc93b@gmail.com>
 <Yte7Tgq3w8fowTYA@google.com>
 <4be15cff-b0af-8671-57aa-6567c0206715@gmail.com>
 <YtgDSelfobu/a/be@google.com>
 <c14698f3-8f70-4cd7-1653-ed51630806c3@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c14698f3-8f70-4cd7-1653-ed51630806c3@gmail.com>
Precedence: bulk

On Wed, 20 Jul 2022, Markuss Broks wrote:

> On 7/20/22 16:29, Lee Jones wrote:
> > On Wed, 20 Jul 2022, Markuss Broks wrote:
> > > > > > > > > +static const struct mfd_cell sm5703_devs[] = {
> > > > > > > > > +	{ .name = "sm5703-regulator", },
> > > > > > > > > +};
> > > > > > > > Where are the rest of the child drivers?
> > > > > > > Should those devices still be present even though there's no driver for them
> > > > > > > (yet) ? I have a WIP version of driver for almost every function, but I
> > > > > > > currently lack time to get them done.
> > > > > > Without them the driver-set is useless, no?
> > > > > > 
> > > > > > We try to refrain from applying dead code.
> > > > > > 
> > > > > > A lot of it has a tendency to stay that way.
> > > > > Well, in my opinion, having just the regulator driver is already useful
> > > > > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for
> > > > > powering the touchscreen. Touchscreen is kind of vital functionality for a
> > > > > phone so I decided to upstream parts that are necessary for it first and
> > > > > finish up other stuff later. It's not the only board that uses SM5703's
> > > > > regulators for supplying all different kinds of hardware, either.
> > > > Upstreaming functionality which is useful on its own is fine, but that
> > > > doesn't tick all of the boxes to justify an MFD.  This is a lot of
> > > > code which is hard to justify if it only registers a Regulator driver.
> > > Do you think I should hold on this series until I have other things done?
> > > Alternatively, I could make the regulator driver standalone, dedicated, but
> > > then when I'd add other functionality I'd have to redo it and add the MFD
> > > driver back, that I believe would be quite annoying from maintainers' and
> > > sanity perspective. The other functions left on the main control I2C are
> > > also not really "vital" to device's functionality (Flash LED and charger),
> > > so the regulator function makes the most sense to be available first, which
> > > was my motivation behind upstreaming that first.

Can you configure your mailer to stop removing the space between
messages please?

> > What's the reason for this being an MFD in the first place?

> Well, the "main" I2C interface is shared between three functions:
> regulators, Flash LED and charger. I call this one the "main" one because
> the device is controlled with it: you can select internal clock rate, enable
> or disable the PMIC and do other things that more of apply to the whole
> PMIC, not just the separate functions (as is the case with fuel gauge and
> MUIC I2C) . That's the reason for this being an MFD: those three functions
> share one I2C interface. While they might not be implemented at the moment,
> this places a foundation for adding future support.

Okay, so the functions cannot be controlled separately?  That's fine.

For acceptance into MFD, you need to enable more than one function.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog