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@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx>
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > ...
> > > 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", ®))
> > > + 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 [李琼斯]
