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, 12 Apr 2023 10:51:09 +0100
From:   Lee Jones <lee@kernel.org>
To:     Naresh Solanki <naresh.solanki@9elements.com>
Cc:     Pavel Machek <pavel@ucw.cz>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [PATCH v3 2/2] leds: max597x: Add support for max597x
Message-ID: <20230412095109.GY8371@google.com>
References: <20230328094416.3851801-1-Naresh.Solanki@9elements.com>
 <20230328094416.3851801-2-Naresh.Solanki@9elements.com>
 <20230405150706.GM8371@google.com>
 <375c6a74-c664-6ecc-cdcd-20cfa4568cd1@9elements.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <375c6a74-c664-6ecc-cdcd-20cfa4568cd1@9elements.com>
Precedence: bulk

On Wed, 12 Apr 2023, Naresh Solanki wrote:

> Hi Lee,
>
> On 05-04-2023 08:37 pm, Lee Jones wrote:
> > On Tue, 28 Mar 2023, Naresh Solanki wrote:
> >
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > >
> > > max597x is hot swap controller with indicator LED support.
> > > This driver uses DT property to configure led during boot time &
> > > also provide the LED control in sysfs.
> > >
> > > DTS example:
> > >      i2c {
> > >          #address-cells = <1>;
> > >          #size-cells = <0>;
> > >          regulator@3a {
> > >              compatible = "maxim,max5978";
> > >              reg = <0x3a>;
> > >              vss1-supply = <&p3v3>;
> > >
> > >              regulators {
> > >                  sw0_ref_0: sw0 {
> > >                      shunt-resistor-micro-ohms = <12000>;
> > >                  };
> > >              };
> > >
> > >              leds {
> > >                  #address-cells = <1>;
> > >                  #size-cells = <0>;
> > >                  led@0 {
> > >                      reg = <0>;
> > >                      label = "led0";
> > >                      default-state = "on";
> > >                  };
> > >                  led@1 {
> > >                      reg = <1>;
> > >                      label = "led1";
> > >                      default-state = "on";
> > >                  };
> > >              };
> > >          };
> > >      };
> > >
> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > ...
> > > Changes in V3:
> > > - Remove of_node_put as its handled by for loop
> > > - Print error if an LED fails to register.
> > > - Update driver name in Kconfig description
> > > - Remove unneeded variable assignment
> > > - Use devm_led_classdev_register to reget led
> > > Changes in V2:
> > > - Fix regmap update
> > > - Remove devm_kfree
> > > - Remove default-state
> > > - Add example dts in commit message
> > > - Fix whitespace in Kconfig
> > > - Fix comment
> > > ---
> > >   drivers/leds/Kconfig        |  11 ++++
> > >   drivers/leds/Makefile       |   1 +
> > >   drivers/leds/leds-max597x.c | 112 ++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 124 insertions(+)
> > >   create mode 100644 drivers/leds/leds-max597x.c

[...]

> > > +
> > > +static int max597x_led_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device_node *np = dev_of_node(pdev->dev.parent);
> >
> > Why not have your own compatible string?
> This is leaf driver & MFD driver does has compatible string.

I can see that, but why not give this driver it's own one?

> > > +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >
> > These "big" API calls are usually done outside of the allocation block.
> >
> > Please move it to just above the check for !regmap.
> >
> > > +	struct device_node *led_node;
> > > +	struct device_node *child;
> > > +	int ret = 0;
> >
> > Is it okay for an LED driver to not to register any LEDs?
> Yes. Usage of indication LED on the max5970/5978 is optional.
> >
> > Perhaps -ENODEV?
> This driver is loaded only if MFD driver is included. remap is setup by MFD
> driver & hence defer probe till MFD driver is loaded.
> >
> > > +	if (!regmap)
> > > +		return -EPROBE_DEFER;
> > > +
> > > +	led_node = of_get_child_by_name(np, "leds");
> > > +	if (!led_node)
> > > +		return -ENODEV;
> >
> > Ah, that's better.  So set ret to -ENODEV and use it here.
> Yes.
> >
> > > +	for_each_available_child_of_node(led_node, child) {
> > > +		u32 reg;
> > > +
> > > +		if (of_property_read_u32(child, "reg", &reg))
> > > +			continue;
> > > +
> > > +		if (reg >= MAX597X_NUM_LEDS) {
> > > +			dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
> > > +				MAX597X_NUM_LEDS);
> > > +			continue;
> > > +		}
> > > +
> > > +		ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
> > > +		if (ret < 0)
> > > +			dev_err(&pdev->dev, "Failed to initialize LED %u\n", reg);
> >
> > I think you (or I) are missing the point of the previous reviews.  It's
> > not okay to error out and continue executing.  Either this is okay (you
> > can warn and carry on) or it's not (return an error).  Your first
> > submission suggested that this was an error.  In which case you do need
> > to return.  I think Pavel was suggesting that you should unwind
> > (de-register) before retuning, rather than leaving things in an odd
> > half-registered state.  Not that you should blindly carry on as if the
> > issue never occurred.
> I did refer to other such implementations & some have used return on error &
> some just print on error & continue. I felt that continue executing with
> warning(on error) is better approach.

I think it should fail fast and with certainty.


--
Lee Jones [李琼斯]