Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20230914114521.1491390-1-naresh.solanki@9elements.com>
 <20230920130528.GG13143@google.com> <CABqG17j_gCr8xw65qjn4Kh7ChdraZbLsyGOsCmFEEWG3txjE4A@mail.gmail.com>
 <20230921103156.GB3449785@google.com>
In-Reply-To: <20230921103156.GB3449785@google.com>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Mon, 30 Oct 2023 14:22:35 +0530
Message-ID: <CABqG17ibzHiYmzCZ6ZpAa8BZhj5N+0dQ0aa1yebtCk0YYVdsFQ@mail.gmail.com>
Subject: Re: [RESEND PATCH v3] leds: max5970: Add support for max5970
To:     Lee Jones <lee@kernel.org>
Cc:     Pavel Machek <pavel@ucw.cz>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Thu, 21 Sept 2023 at 16:02, Lee Jones <lee@kernel.org> wrote:
>
> On Thu, 21 Sep 2023, Naresh Solanki wrote:
>
> > Hi
> >
> >
> > On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
> > >
> > > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> > >
> > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > >
> > > > The MAX5970 is hot swap controller and has 4 indication LED.
> > > >
> > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > ---
> > > > Changes in V3:
> > > > - Drop array for ddata variable.
> > > > Changes in V2:
> > > > - Add of_node_put before return.
> > > > - Code cleanup
> > > > - Refactor code & remove max5970_setup_led function.
> > > > ---
> > > >  drivers/leds/Kconfig        |  11 ++++
> > > >  drivers/leds/Makefile       |   1 +
> > > >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++=
++++
> > > >  3 files changed, 122 insertions(+)
> > > >  create mode 100644 drivers/leds/leds-max5970.c
> > >
> > > Couple of nits and you're good to go.
> > >
> > > Once fixed please resubmit with my:
> > >
> > >   Reviewed-by: Lee Jones <lee@kernel.org>
> > >
> > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > > index b92208eccdea..03ef527cc545 100644
> > > > --- a/drivers/leds/Kconfig
> > > > +++ b/drivers/leds/Kconfig
> > > > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> > > >         To compile this driver as a module, choose M here: the modu=
le will
> > > >         be called leds-adp5520.
> > > >
> > > > +config LEDS_MAX5970
> > > > +     tristate "LED Support for Maxim 5970"
> > > > +     depends on LEDS_CLASS
> > > > +     depends on MFD_MAX5970
> > > > +     help
> > > > +       This option enables support for the Maxim MAX5970 & MAX5978=
 smart
> > > > +       switch indication LEDs via the I2C bus.
> > > > +
> > > > +       To compile this driver as a module, choose M here: the modu=
le will
> > > > +       be called leds-max5970.
> > > > +
> > > >  config LEDS_MC13783
> > > >       tristate "LED Support for MC13XXX PMIC"
> > > >       depends on LEDS_CLASS
> > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > index d7348e8bc019..6eaee0a753c6 100644
> > > > --- a/drivers/leds/Makefile
> > > > +++ b/drivers/leds/Makefile
> > > > @@ -56,6 +56,7 @@ obj-$(CONFIG_LEDS_LP8501)           +=3D leds-lp8=
501.o
> > > >  obj-$(CONFIG_LEDS_LP8788)            +=3D leds-lp8788.o
> > > >  obj-$(CONFIG_LEDS_LP8860)            +=3D leds-lp8860.o
> > > >  obj-$(CONFIG_LEDS_LT3593)            +=3D leds-lt3593.o
> > > > +obj-$(CONFIG_LEDS_MAX5970)           +=3D leds-max5970.o
> > > >  obj-$(CONFIG_LEDS_MAX77650)          +=3D leds-max77650.o
> > > >  obj-$(CONFIG_LEDS_MAX8997)           +=3D leds-max8997.o
> > > >  obj-$(CONFIG_LEDS_MC13783)           +=3D leds-mc13783.o
> > > > diff --git a/drivers/leds/leds-max5970.c b/drivers/leds/leds-max597=
0.c
> > > > new file mode 100644
> > > > index 000000000000..c9685990e26e
> > > > --- /dev/null
> > > > +++ b/drivers/leds/leds-max5970.c
> > > > @@ -0,0 +1,110 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Device driver for leds in MAX5970 and MAX5978 IC
> > > > + *
> > > > + * Copyright (c) 2022 9elements GmbH
> > > > + *
> > > > + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > + */
> > > > +
> > > > +#include <linux/leds.h>
> > > > +#include <linux/mfd/max5970.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led=
, cdev)
> > > > +
> > > > +struct max5970_led {
> > > > +     struct device *dev;
> > > > +     struct regmap *regmap;
> > > > +     struct led_classdev cdev;
> > > > +     unsigned int index;
> > > > +};
> > > > +
> > > > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > > > +                                   enum led_brightness brightness)
> > > > +{
> > > > +     struct max5970_led *ddata =3D ldev_to_maxled(cdev);
> > > > +     int ret, val;
> > > > +
> > > > +     /* Set/clear corresponding bit for given led index */
> > > > +     val =3D !brightness ? BIT(ddata->index) : 0;
> > > > +
> > > > +     ret =3D regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLA=
SH, BIT(ddata->index), val);
> > > > +     if (ret < 0)
> > > > +             dev_err(cdev->dev, "failed to set brightness %d", ret=
);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static int max5970_led_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct device *dev =3D &pdev->dev;
> > > > +     struct device_node *np =3D dev_of_node(dev->parent);
> > > > +     struct regmap *regmap;
> > > > +     struct device_node *led_node;
> > > > +     struct device_node *child;
> > >
> > > Nit: You can place these on the same line.
> > Ack
> > >
> > > > +     struct max5970_led *ddata;
> > > > +     int ret =3D -ENODEV, num_leds =3D 0;
> > > > +
> > > > +     regmap =3D dev_get_regmap(pdev->dev.parent, NULL);
> > > > +     if (!regmap)
> > > > +             return -EPROBE_DEFER;
> > >
> > > Why are you deferring here?
> > This is a Leaf driver. Making sure the parent driver has initialized re=
gmap.
>
> How can this driver initialise before the parent driver?
The parent driver in this case is simple_i2c_mfd.
Based on reference from other similar implementations, the regmap
check was adapted.
As you mentioned, your right that leaf driver will not start before parent
driver is loaded successfully so probably the DEFER might not be needed
here.

Thanks,
Naresh
>
> --
> Lee Jones [=E6=9D=8E=E7=90=BC=E6=96=AF]