Re: [PATCH v5 5/5] power: supply: ltc3350-charger: Add driver

[Sebastian Reichel]

Hi,

On Tue, May 28, 2024 at 11:41:36AM +0200, Mike Looijmans wrote:
> On 27-05-2024 22:40, Sebastian Reichel wrote:
> > > ...
> > > +static int ltc3350_get_property(struct power_supply *psy, enum power_supply_property psp,
> > > +				union power_supply_propval *val)
> > > +{
> > > +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> > > +
> > > +	switch (psp) {
> > > +	case POWER_SUPPLY_PROP_STATUS:
> > > +		return ltc3350_get_status(info, val);
> > > +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> > > +		return ltc3350_get_charge_type(info, val);
> > > +	case POWER_SUPPLY_PROP_ONLINE:
> > > +		return ltc3350_get_online(info, val);
> > > +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > > +		return ltc3350_get_scaled(info, LTC3350_REG_MEAS_VOUT, 22100, &val->intval);
> > > +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> > > +		return ltc3350_get_scaled(info, LTC3350_REG_VOUT_UV_LVL, 22100, &val->intval);
> > > +	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
> > > +		return ltc3350_get_scaled(info, LTC3350_REG_VOUT_OV_LVL, 22100, &val->intval);
> > For USB chargers VOLTAGE_NOW/MIN/MAX refers to VBUS, which is the
> > voltage on the USB lines and thus the input voltage. From my
> > understanding of the LTC3350 this would be VIN and not VOUT. With
> > VOUT being supplied by either VIN or the Capacitors.
> > 
> > So I think your custom vin/vin_uv/vin_ov should be exposed with the
> > above properties.
> 
> Yeah, power supplies report their input voltage. So this should
> report VIN.
> 
> > My understanding for VOUT is, that this is basically the system
> > supply - I would expect more regulators to follow, which convert
> > it to typical voltages like 3.3V or 1.8V. In that case it would
> > make sense to expose VOUT as regulator, so that it can be referenced
> > as vin-supply. You can find a few examples for charger drivers doing
> > that for USB-OTG functionality.
> 
> Looked at other drivers for that. Significant difference is that those have
> something useful to supply to other drivers, e.g. being able to
> enable/disable the output for one thing.
> 
> For the LTC3350, the output (voltage) is just a measurement and there's
> nothing for the driver to configure. Any regulator_ops would be completely
> empty.

Which is also true for fixed regulators, see fixed_voltage_ops in
drivers/regulator/fixed.c. Not having any control options is not a
problem per se.

Note, that regulators have pre-defined events for the functionality
you are trying to provide for VOUT, e.g.
 - REGULATOR_EVENT_OVER_VOLTAGE_WARN
 - REGULATOR_EVENT_UNDER_VOLTAGE
 - REGULATOR_EVENT_UNDER_VOLTAGE_WARN

> Given that, I think it should get the same treatment as GPI, so either a
> custom property or some other device (IIO). Or maybe add a VOLTAGE_OUT
> property? It's not uncommon for power management devices to report their
> output voltage.

power-supply handles batteries and battery chargers. For both cases
the kernel already has properties for the output voltage (which
would be the charging voltage in case of a charger). Other PM voltage
regulation elements are so far handled via the regulator subsystem
and hwmon is used for monitoring.

Greetings,

-- Sebastian

Attachment: signature.asc
Description: PGP signature