Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging

[Mark Brown]

On Tue, Nov 17, 2020 at 09:38:30AM +0100, Mauro Carvalho Chehab wrote:
> Mark Brown <broonie@xxxxxxxxxx> escreveu:

> > This also appears to be missing a DT binding document, binding
> > documentation is required for anything with DT support.

> The DT binding is documented on patch 3/8, together with MFD.

> As there's just one compatible for MFD and regulators altogether,
> I opted to have just one DT binding document for both.

I should've been copied on that patch then so the bindings can be
reviewed.

> > > +static DEFINE_MUTEX(enable_mutex);  

> > Drivers shouldn't be declaring global variables, if this is required it
> > should be allocated in driver data.

> I'll try to see a better place for this, but in this case, the
> mutex should be global anyway, as the access to the SPMI bus
> should be serialized.

Surely the bus should be dealing with basic I/O serialisation?

> > It looks like it would be less code overall to just implement a regmap
> > for the MFD, even if it were only used in this driver - almost
> > everything in here is just a carbon copy of standard helpers that the
> > core provides for regmap devices.  Doing it in the MFD would be more
> > idiomatic for everything though.

> I tried to use regmap for this driver while porting it from Linaro's
> OOT to upstream, bkut it turns that adding support for it is not trivial.

Could you expand on "not trivial" please?  What did you try and what
difficulties did you encounter?

> I suspect that, just like I2C has their own set of regmap functions
> (regmap_init_i2c, devm_regmap_init_i2c), if we want to properly support
> regmap, we need first to add a SPMI variant to it, as the locking
> should be bus-aware.

drivers/base/regmap/regmap-spmi.c has been there since 2013, or if for
some reason this device is doing something non-standard for the bus then
the reg_read() and reg_write() operations can be used.

> > > +	ret = of_property_read_u32(np, "vsel-reg", &rdesc->vsel_reg);  

> > There is no way this stuff should be being parsed out of DT, we should
> > know the register map for the device and what regulators it has based
> > purely off knowing what device we have.  Baking the register map into
> > the ABI is bad practice, it prevents us from improving our support for
> > the hardware without going and updating people's DTs.

> Well, I can move the existing registers out of DT, but, as I don't
> have any datasheet for this device, it means that I can implement
> there only a subset of the available LDOs. So, from the 36 LDOs that
> this chip support, we only know the registers for 8 of them:

> 	ldo3, ldo4, ldo9, ldo15, ldo16, ldo17, ldo33 and ldo34.

People will be free to submit patches adding additional functionality to
the driver if they need it.

> > > +	/*
> > > +	 * Not all regulators work on idle mode
> > > +	 */
> > > +	ret = of_property_read_u32(np, "idle-mode-mask", &sreg->eco_mode_mask);

> > There's no conditional code to not register the mode operations if the
> > mode information is not available (and again this should be done based 
> > on knowing the properties of the device, this is unlikely to be system
> > dependent).

> This is the same case as before: as we don't have any datasheets,
> I only know what's there at the DT for just one device (Hikey 970).

> Again, we could hardcode those assuming Hikey 970 settings, but,
> if some other device using the same chip will ever be added, and
> they use some different configuration then we may face some
> backward-compatibility issues.

I can't follow the logic here, sorry.  If we have no idea how to
configure something how would offering operations to configure that
thing be useful?

> > > +	constraint->valid_modes_mask = REGULATOR_MODE_NORMAL;

> > This is absolutely not OK, a regulator driver should *not* be modifying
> > the constraints that the machine has set.  If it is safe to change modes
> > on a platform and the system integrator wants to do that then they will
> > set the constraints appropriately, there is no way the regulator driver
> > can tell what is appropriate on a given system.  The fact that the
> > driver is including machine.h at all ought to have been an indicator
> > that there's an abstraction problem here.

> Ok. Where such constraints should be instead? at the Hikey 970 DT
> file?

They should be configured whereever all the other constraints are
configured by the platform, if that is DT then they should also be
configured in DT.

Attachment: signature.asc
Description: PGP signature