linux-kernel - Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABqG17h8hxgmMA=G5hitzgjNSX_BZ2utFsMZni9evn0Nogu0GA@mail.gmail.com>
Date:   Thu, 9 Nov 2023 15:20:29 +0530
From:   Naresh Solanki <naresh.solanki@...ements.com>
To:     Lee Jones <lee@...nel.org>
Cc:     Pavel Machek <pavel@....cz>,
        Patrick Rudolph <patrick.rudolph@...ements.com>,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

Hey Lee,

Is there anything specific you'd suggest changing in the current
patchset, or are we good to proceed?

Thanks,
Naresh Solanki

On Mon, 30 Oct 2023 at 14:22, Naresh Solanki
<naresh.solanki@...ements.com> wrote:
>
> Hi,
>
> On Thu, 21 Sept 2023 at 16:02, Lee Jones <lee@...nel.org> wrote:
> >
> > On Thu, 21 Sep 2023, Naresh Solanki wrote:
> >
> > > Hi
> > >
> > >
> > > On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@...nel.org> wrote:
> > > >
> > > > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> > > >
> > > > > From: Patrick Rudolph <patrick.rudolph@...ements.com>
> > > > >
> > > > > The MAX5970 is hot swap controller and has 4 indication LED.
> > > > >
> > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
> > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.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@...nel.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 module 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 module 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)           += leds-lp8501.o
> > > > >  obj-$(CONFIG_LEDS_LP8788)            += leds-lp8788.o
> > > > >  obj-$(CONFIG_LEDS_LP8860)            += leds-lp8860.o
> > > > >  obj-$(CONFIG_LEDS_LT3593)            += leds-lt3593.o
> > > > > +obj-$(CONFIG_LEDS_MAX5970)           += leds-max5970.o
> > > > >  obj-$(CONFIG_LEDS_MAX77650)          += leds-max77650.o
> > > > >  obj-$(CONFIG_LEDS_MAX8997)           += leds-max8997.o
> > > > >  obj-$(CONFIG_LEDS_MC13783)           += leds-mc13783.o
> > > > > diff --git a/drivers/leds/leds-max5970.c b/drivers/leds/leds-max5970.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@...ements.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 = ldev_to_maxled(cdev);
> > > > > +     int ret, val;
> > > > > +
> > > > > +     /* Set/clear corresponding bit for given led index */
> > > > > +     val = !brightness ? BIT(ddata->index) : 0;
> > > > > +
> > > > > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, 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 = &pdev->dev;
> > > > > +     struct device_node *np = 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 = -ENODEV, num_leds = 0;
> > > > > +
> > > > > +     regmap = 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 regmap.
> >
> > 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 [李琼斯]