Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD

[Lee Jones <lee.jones@linaro.org>]

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