Re: [PATCH v2 2/3] leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

[Linus Walleij]

Hi Rudraksha,

this is a clear improvement! I like the directions this
is going.

Mark Brown needs to look at how you're using regulators
here.

On Wed, Mar 18, 2026 at 7:34 PM Rudraksha Gupta via B4 Relay
<devnull+guptarud.gmail.com@xxxxxxxxxx> wrote:

> From: Rudraksha Gupta <guptarud@xxxxxxxxx>
>
> Extend the RT8515 driver to support flash ICs that use only a single
> GPIO for both flash and torch modes (no separate ENT pin), with an
> optional vin regulator that gates power to the flash IC.
>
> When vin-supply is provided, the driver enables the regulator before
> activating the LED and disables it when turning off.
>
> Make ent-gpios optional and validate at probe time that exactly one of
> ent-gpios or vin-supply is provided. When ent-gpios is absent, the
> driver uses enf-gpios for both flash and torch brightness control.
>
> Signed-off-by: Rudraksha Gupta <guptarud@xxxxxxxxx>

(...)

>         struct regulator *reg;
> +       bool reg_enabled;

I think you can actually ask a regulator if it is enabled already,
regulator_is_enabled().

Also as long as you enable and disable it the same number of
times it also contains an internal reference count, so this is
often enough.

> +       if (rt->reg && rt->reg_enabled) {
> +               regulator_disable(rt->reg);
> +               rt->reg_enabled = false;
> +       }

I don't think you need to NULL-check rt-reg but not sure.

Can you use regulator_is_enabled() instead of the bool local?

> +               /* Enable the vin regulator if needed */
> +               if (rt->reg && !rt->reg_enabled) {

Dito

> +               /* Enable the vin regulator if needed */
> +               if (rt->reg && !rt->reg_enabled) {
> +                       ret = regulator_enable(rt->reg);

Dito

>         if (state) {
> +               /* Enable the vin regulator if needed */
> +               if (rt->reg && !rt->reg_enabled) {

Dito.

> +       /* Optional VIN supply (e.g. GPIO-controlled fixed regulator) */
> +       rt->reg = devm_regulator_get_optional(dev, "vin");
> +       if (IS_ERR(rt->reg)) {
> +               if (PTR_ERR(rt->reg) == -ENODEV)
> +                       rt->reg = NULL;

I think the regulator callbacks are able to deal transparently
with "regulators" that are -ENODEV, can you just let it pass?
Anyway Mark knows what to do here.

> +       /* Exactly one of ENT or VIN must be provided */
> +       if (!rt->enable_torch == !rt->reg)
> +               return dev_err_probe(dev, -EINVAL,
> +                                    "exactly one of ent-gpios or vin-supply is required\n");

Please drop this check.

I think there can be systems using the torch but also define
the regulator.

Yours,
Linus Walleij