Message ID | 20230323194550.1914725-2-Naresh.Solanki@9elements.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp3122585wrt; Thu, 23 Mar 2023 13:38:28 -0700 (PDT) X-Google-Smtp-Source: AKy350a+Ed8UTv1EC8OOo1rZah7JCg1my0X5jdFjE/dcmQkPYiCWJecrRP40ZKxDIi7cB35jAWr+ X-Received: by 2002:a17:906:6b1a:b0:932:ac10:94b6 with SMTP id q26-20020a1709066b1a00b00932ac1094b6mr313879ejr.32.1679603908655; Thu, 23 Mar 2023 13:38:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679603908; cv=none; d=google.com; s=arc-20160816; b=wW0ckblkBvnYoxPlVN+JUFtZvtJpEdKzB89f5pZnkVwJbihG6iwh5QLFAIp5+h1Dw2 cwIh0Uda4uNo5jUt0W9y5QXqDx2oAts8L5ihoA3f1xG83dYVMMUN8C7WitMroaBk7RWP 2hl02XYoenr6DRoJ6NpxX/h6yQu0CkINbg9jHWqmszXmBhLkDXcT/8CE1cSx46fYMbDt UPwadJr6/duiSHPKzVOh+hPOXwoj2DiJKKe9HICNAzF0urhYXV6EMVf2eVvTd/KEHrWX Ge+amPyQ966iRhWfDX2eJ64X5MQKBjtVZNwRMwlNDPFH8ZUtpQQ3yHVjFf1YDgl8rheI AXzA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=r4aFjfm4grujQtLLswnv/YsMbOxmogRnwE60Yeaq9Rk=; b=CpJJk31wAT++a6zLZayqsr5vWZWt0WAZEE1zAurw1S1yKnQcko8IgIc/jOL4cDR01x GpIeHKOoNy7cRE+0ZljnDsjrhrq81XlBucoh3C8TiiAgVKLZ8/Tyk/4+dtgiXyaPfPm8 qA9xs5xCKHZNZLBSFGUsFn6LlejrDS0eG3pzgUHU40GHWbYe2oYrckweLMDtYKWx4ROd Y3DoGHI7gQcfmO14wqT2zvpHOSijnyRY78jFMgfh0wi7AwC8JgEMogc3j9JT63lH1bNn qp3KTLsKhw6gqveLec8VTMCnwXu13hvQEf3Jsm7qNX/HFJNydnQK1UxmZcWLTmmeSYIV w5Ng== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=O18ork11; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id x10-20020a1709060eea00b0093b794c719csi4296796eji.690.2023.03.23.13.38.02; Thu, 23 Mar 2023 13:38:28 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=O18ork11; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231320AbjCWTqB (ORCPT <rfc822;ezelljr.billy@gmail.com> + 99 others); Thu, 23 Mar 2023 15:46:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50716 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231183AbjCWTp6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 23 Mar 2023 15:45:58 -0400 Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3491F24135 for <linux-kernel@vger.kernel.org>; Thu, 23 Mar 2023 12:45:55 -0700 (PDT) Received: by mail-wr1-x431.google.com with SMTP id r29so21756909wra.13 for <linux-kernel@vger.kernel.org>; Thu, 23 Mar 2023 12:45:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; t=1679600754; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=r4aFjfm4grujQtLLswnv/YsMbOxmogRnwE60Yeaq9Rk=; b=O18ork11VvTKtLgznMRV5gcA1STeP7Px3eVuhqPA8z4McZBONaTAotI51HdKgBtzNY 4Zf6xqLCKv3ED/YjUsmsSyUl5nNwE9hOA3Tg93IZGHAW3rbAl87yNsX7Orj2WPCU5mkT ol2JWiUk2YcYCLQ/ckekG/5CPJs9bx04UttIoYjP4G0zRjr7/CJ+3awADEyaGS2odPhA YX60QlYTMUU+HI5cfRtoZyccTS6AB7DaCMKqr442aKj+amCIu61MN+Ho537DWovFtBtK lxD0ycVQ+iP66KaVurrtszkxrA/ItrHW7LLn1t5071atp55NJZ7xQ+GNa/Vxhf5mnYwK eUgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679600754; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=r4aFjfm4grujQtLLswnv/YsMbOxmogRnwE60Yeaq9Rk=; b=AllG+z5A6dZrX3p+hXr8acxigTfgFt0HLl/uDl+xBEcVxEEOXObkN1Lm/PVtuWL1/U BEOBRwMubWR+rHziejiFgjqaBtsPHV9L7tWEBvusKP+zUxlod46ePkiSNtAhL33daQfP BwXWcYOz5GBtqfVSdRJ/90JN6uAxsabmbUpyuKzR+DL1cjzBVBQv+Vrn5XXExKtWnFte z7W4yadUqqkXXyq83M1HO1Ch+TbsClRCkeZ3V0FufbDLOWubxz6uowS0RaB12Tu4KoxO jOOPQG6v1XeQKDPMibGmrMZ9O41VQW0F6Ur0wMO+o0yRoaWgumN/l6XzfFKP6EczgL3S aJBg== X-Gm-Message-State: AAQBX9flZ7MpIhazYIjc9T9Ch88pMZYzFmNbdSftajfzlNyNGtnqsKA3 H7iojyq/JSw9qK1Aqft6LzNtdA== X-Received: by 2002:adf:dc8c:0:b0:2d0:776:b773 with SMTP id r12-20020adfdc8c000000b002d00776b773mr232591wrj.35.1679600754238; Thu, 23 Mar 2023 12:45:54 -0700 (PDT) Received: from stroh80.sec.9e.network (ip-078-094-000-051.um19.pools.vodafone-ip.de. [78.94.0.51]) by smtp.gmail.com with ESMTPSA id s17-20020a5d4251000000b002d1801018e2sm16829348wrr.63.2023.03.23.12.45.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Mar 2023 12:45:54 -0700 (PDT) From: Naresh Solanki <naresh.solanki@9elements.com> X-Google-Original-From: Naresh Solanki <Naresh.Solanki@9elements.com> To: Lee Jones <lee@kernel.org>, Pavel Machek <pavel@ucw.cz> Cc: Patrick Rudolph <patrick.rudolph@9elements.com>, Naresh Solanki <Naresh.Solanki@9elements.com>, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org Subject: [PATCH v2 2/2] leds: max597x: Add support for max597x Date: Thu, 23 Mar 2023 20:45:49 +0100 Message-Id: <20230323194550.1914725-2-Naresh.Solanki@9elements.com> X-Mailer: git-send-email 2.39.1 In-Reply-To: <20230323194550.1914725-1-Naresh.Solanki@9elements.com> References: <20230323194550.1914725-1-Naresh.Solanki@9elements.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761192348573474626?= X-GMAIL-MSGID: =?utf-8?q?1761192348573474626?= |
Series |
[v2,1/2] iio: max597x: Add support for max597x
|
|
Commit Message
Naresh Solanki
March 23, 2023, 7:45 p.m. UTC
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 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
Comments
Le 23/03/2023 à 20:45, Naresh Solanki a écrit : > 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 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 > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dbce09eabac..ec2b731ae545 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -590,6 +590,17 @@ config LEDS_ADP5520 > To compile this driver as a module, choose M here: the module will > be called leds-adp5520. > > +config LEDS_MAX597X > + tristate "LED Support for Maxim 597x" > + depends on LEDS_CLASS > + depends on MFD_MAX597X > + help > + This option enables support for the Maxim 597x smart switch indication LEDs > + via the I2C bus. > + > + To compile this driver as a module, choose M here: the module will > + be called max597x-led. leds-max597x? > + > config LEDS_MC13783 > tristate "LED Support for MC13XXX PMIC" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index d30395d11fd8..da1192e40268 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -53,6 +53,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_MAX597X) += leds-max597x.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-max597x.c b/drivers/leds/leds-max597x.c > new file mode 100644 > index 000000000000..3e1747c8693e > --- /dev/null > +++ b/drivers/leds/leds-max597x.c > @@ -0,0 +1,112 @@ > +// 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/max597x.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define ldev_to_maxled(c) container_of(c, struct max597x_led, led) > + > +struct max597x_led { > + struct regmap *regmap; > + struct led_classdev led; > + unsigned int index; > +}; > + > +static int max597x_led_set_brightness(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct max597x_led *led = ldev_to_maxled(cdev); > + int ret, val = 0; > + > + if (!led || !led->regmap) > + return -ENODEV; > + > + val = !brightness ? BIT(led->index) : 0; > + ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, BIT(led->index), val); > + if (ret < 0) > + dev_err(cdev->dev, "failed to set brightness %d\n", ret); > + return ret; > +} > + > +static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc, > + u32 reg) > +{ > + struct max597x_led *led; > + int ret = 0; Nit: useless "= 0" > + > + led = devm_kzalloc(dev, sizeof(struct max597x_led), > + GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + if (of_property_read_string(nc, "label", &led->led.name)) > + led->led.name = nc->name; > + > + led->led.max_brightness = 1; > + led->led.brightness_set_blocking = max597x_led_set_brightness; > + led->led.default_trigger = "none"; > + led->index = reg; > + led->regmap = regmap; > + ret = led_classdev_register(dev, &led->led); devm_led_classdev_register? > + if (ret) > + dev_err(dev, "Error in initializing led %s", led->led.name); > + > + return ret; > +} > + > +static int max597x_led_probe(struct platform_device *pdev) > +{ > + struct device_node *np = dev_of_node(pdev->dev.parent); > + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); > + struct device_node *led_node; > + struct device_node *child; > + int ret = 0; > + > + if (!regmap) > + return -EPROBE_DEFER; > + > + led_node = of_get_child_by_name(np, "leds"); > + if (!led_node) > + return -ENODEV; > + > + 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) > + of_node_put(child); This of_node_put() looks odd to me. "return ret;" or "break;" missing ? > + } > + > + return ret; > +} > + > +static struct platform_driver max597x_led_driver = { > + .driver = { > + .name = "max597x-led", > + }, > + .probe = max597x_led_probe, > +}; > + > +module_platform_driver(max597x_led_driver); > + > +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); > +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); > +MODULE_LICENSE("GPL");
Hi, On 24-03-2023 01:48 am, Christophe JAILLET wrote: > Le 23/03/2023 à 20:45, Naresh Solanki a écrit : >> 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 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 >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 9dbce09eabac..ec2b731ae545 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -590,6 +590,17 @@ config LEDS_ADP5520 >> To compile this driver as a module, choose M here: the module >> will >> be called leds-adp5520. >> +config LEDS_MAX597X >> + tristate "LED Support for Maxim 597x" >> + depends on LEDS_CLASS >> + depends on MFD_MAX597X >> + help >> + This option enables support for the Maxim 597x smart switch >> indication LEDs >> + via the I2C bus. >> + >> + To compile this driver as a module, choose M here: the module will >> + be called max597x-led. > > leds-max597x? As per struct max597x_led_driver, driver name is max597x-led > >> + >> config LEDS_MC13783 >> tristate "LED Support for MC13XXX PMIC" >> depends on LEDS_CLASS >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index d30395d11fd8..da1192e40268 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -53,6 +53,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_MAX597X) += leds-max597x.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-max597x.c b/drivers/leds/leds-max597x.c >> new file mode 100644 >> index 000000000000..3e1747c8693e >> --- /dev/null >> +++ b/drivers/leds/leds-max597x.c >> @@ -0,0 +1,112 @@ >> +// 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/max597x.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +#define ldev_to_maxled(c) container_of(c, struct max597x_led, led) >> + >> +struct max597x_led { >> + struct regmap *regmap; >> + struct led_classdev led; >> + unsigned int index; >> +}; >> + >> +static int max597x_led_set_brightness(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct max597x_led *led = ldev_to_maxled(cdev); >> + int ret, val = 0; >> + >> + if (!led || !led->regmap) >> + return -ENODEV; >> + >> + val = !brightness ? BIT(led->index) : 0; >> + ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, >> BIT(led->index), val); >> + if (ret < 0) >> + dev_err(cdev->dev, "failed to set brightness %d\n", ret); >> + return ret; >> +} >> + >> +static int max597x_setup_led(struct device *dev, struct regmap >> *regmap, struct device_node *nc, >> + u32 reg) >> +{ >> + struct max597x_led *led; >> + int ret = 0; > > Nit: useless "= 0" Ack. Will be removing ' = 0' in V3 > >> + >> + led = devm_kzalloc(dev, sizeof(struct max597x_led), >> + GFP_KERNEL); >> + if (!led) >> + return -ENOMEM; >> + >> + if (of_property_read_string(nc, "label", &led->led.name)) >> + led->led.name = nc->name; >> + >> + led->led.max_brightness = 1; >> + led->led.brightness_set_blocking = max597x_led_set_brightness; >> + led->led.default_trigger = "none"; >> + led->index = reg; >> + led->regmap = regmap; >> + ret = led_classdev_register(dev, &led->led); > > devm_led_classdev_register? Ack. Will update in V3 > >> + if (ret) >> + dev_err(dev, "Error in initializing led %s", led->led.name); >> + >> + return ret; >> +} >> + >> +static int max597x_led_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = dev_of_node(pdev->dev.parent); >> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >> + struct device_node *led_node; >> + struct device_node *child; >> + int ret = 0; >> + >> + if (!regmap) >> + return -EPROBE_DEFER; >> + >> + led_node = of_get_child_by_name(np, "leds"); >> + if (!led_node) >> + return -ENODEV; >> + >> + 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) >> + of_node_put(child); > > This of_node_put() looks odd to me. Not sure if I get this right but if led setup fails of_node_put should be called. > "return ret;" or "break;" missing ? > Didn't add a break so that it can continue initializing remaining led if any. >> + } >> + >> + return ret; >> +} >> + >> +static struct platform_driver max597x_led_driver = { >> + .driver = { >> + .name = "max597x-led", >> + }, >> + .probe = max597x_led_probe, >> +}; >> + >> +module_platform_driver(max597x_led_driver); >> + >> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); >> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); >> +MODULE_LICENSE("GPL"); > Will push V3 based on feedback. Regards, Naresh
Hi! > > > +++ b/drivers/leds/Kconfig > > > @@ -590,6 +590,17 @@ config LEDS_ADP5520 > > > To compile this driver as a module, choose M here: the > > > module will > > > be called leds-adp5520. > > > +config LEDS_MAX597X > > > + tristate "LED Support for Maxim 597x" > > > + depends on LEDS_CLASS > > > + depends on MFD_MAX597X > > > + help > > > + This option enables support for the Maxim 597x smart switch > > > indication LEDs > > > + via the I2C bus. > > > + > > > + To compile this driver as a module, choose M here: the module will > > > + be called max597x-led. > > > > leds-max597x? > As per struct max597x_led_driver, driver name is max597x-led Please test the modular build and double check the module name. BR, Pavel
Hi Christophe, Pavel, On 24-03-2023 06:37 pm, Pavel Machek wrote: > Hi! > >>>> +++ b/drivers/leds/Kconfig >>>> @@ -590,6 +590,17 @@ config LEDS_ADP5520 >>>> To compile this driver as a module, choose M here: the >>>> module will >>>> be called leds-adp5520. >>>> +config LEDS_MAX597X >>>> + tristate "LED Support for Maxim 597x" >>>> + depends on LEDS_CLASS >>>> + depends on MFD_MAX597X >>>> + help >>>> + This option enables support for the Maxim 597x smart switch >>>> indication LEDs >>>> + via the I2C bus. >>>> + >>>> + To compile this driver as a module, choose M here: the module will >>>> + be called max597x-led. >>> >>> leds-max597x? >> As per struct max597x_led_driver, driver name is max597x-led > > Please test the modular build and double check the module name. Thank you for your suggestion, it turns out that you were right. The correct module name is leds-max597x(./drivers/leds/leds-max597x.ko). Will update Kconfig message accordingly. Thanks! > > BR, > Pavel > Regards, Naresh
Le 24/03/2023 à 11:54, Naresh Solanki a écrit : > Hi, > > On 24-03-2023 01:48 am, Christophe JAILLET wrote: >> Le 23/03/2023 à 20:45, Naresh Solanki a écrit : >>> 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. >>> [...] >>> +static int max597x_led_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *np = dev_of_node(pdev->dev.parent); >>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >>> + struct device_node *led_node; >>> + struct device_node *child; >>> + int ret = 0; >>> + >>> + if (!regmap) >>> + return -EPROBE_DEFER; >>> + >>> + led_node = of_get_child_by_name(np, "leds"); >>> + if (!led_node) >>> + return -ENODEV; >>> + >>> + 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) >>> + of_node_put(child); >> >> This of_node_put() looks odd to me. > Not sure if I get this right but if led setup fails of_node_put should > be called. My understanding is that this of_node_put() is there in case of error, to release what would otherwise never be released by for_each_available_child_of_node() if we exit early from the loop. If the purpose is to release a reference taken in max597x_setup_led() in case of error: - this should be done IMHO within max597x_setup_led() directly - there should be a of_node_get() somewhere in max597x_setup_led() (after: if (of_property_read_string(nc, "label", &led->led.name)) led->led.name = nc->name; + error handling path, *or* just before the final return ret; when we know that everything is fine, if I understand correctly the code) Is the reference taken elsewhere? Did I miss something obvious? >> "return ret;" or "break;" missing ? >> > Didn't add a break so that it can continue initializing remaining led if > any. Ok. Don't know the code enough to see if correct or not, but based on my comment above, I think that something is missing in max597x_setup_led() and that errors should be silently ignored here. CJ >>> + } >>> + >>> + return ret; >>> +} >>> + [...]
Hi, On 24-03-2023 09:06 pm, Christophe JAILLET wrote: > Le 24/03/2023 à 11:54, Naresh Solanki a écrit : >> Hi, >> >> On 24-03-2023 01:48 am, Christophe JAILLET wrote: >>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit : >>>> 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. >>>> > > [...] > > >>>> +static int max597x_led_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = dev_of_node(pdev->dev.parent); >>>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >>>> + struct device_node *led_node; >>>> + struct device_node *child; >>>> + int ret = 0; >>>> + >>>> + if (!regmap) >>>> + return -EPROBE_DEFER; >>>> + >>>> + led_node = of_get_child_by_name(np, "leds"); >>>> + if (!led_node) >>>> + return -ENODEV; >>>> + >>>> + 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) >>>> + of_node_put(child); >>> >>> This of_node_put() looks odd to me. >> Not sure if I get this right but if led setup fails of_node_put should >> be called. > > My understanding is that this of_node_put() is there in case of error, > to release what would otherwise never be released by > for_each_available_child_of_node() if we exit early from the loop. > > If the purpose is to release a reference taken in max597x_setup_led() in > case of error: > - this should be done IMHO within max597x_setup_led() directly > - there should be a of_node_get() somewhere in max597x_setup_led() > (after: > if (of_property_read_string(nc, "label", &led->led.name)) > led->led.name = nc->name; > + error handling path, *or* > just before the final return ret; when we know that everything is > fine, > if I understand correctly the code) > > Is the reference taken elsewhere? > Did I miss something obvious? > > One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311 Please do let me know if I have to do anything about it. >>> "return ret;" or "break;" missing ? >>> >> Didn't add a break so that it can continue initializing remaining led >> if any. > > Ok. Don't know the code enough to see if correct or not, but based on my > comment above, I think that something is missing in max597x_setup_led() > and that errors should be silently ignored here. In my implementation, I have chosen to continue with the next LED if an error occurs, rather than aborting the 'for loop' with an error. I have seen other implementations also done in a similar way. Do you have any further inputs or suggestions on this approach. > > CJ > >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + > > [...] > Regards, Naresh
Le 27/03/2023 à 17:47, Naresh Solanki a écrit : > Hi, > > On 24-03-2023 09:06 pm, Christophe JAILLET wrote: >> Le 24/03/2023 à 11:54, Naresh Solanki a écrit : >>> Hi, >>> >>> On 24-03-2023 01:48 am, Christophe JAILLET wrote: >>>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit : >>>>> 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. >>>>> >> >> [...] >> >> >>>>> +static int max597x_led_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct device_node *np = dev_of_node(pdev->dev.parent); >>>>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >>>>> + struct device_node *led_node; >>>>> + struct device_node *child; >>>>> + int ret = 0; >>>>> + >>>>> + if (!regmap) >>>>> + return -EPROBE_DEFER; >>>>> + >>>>> + led_node = of_get_child_by_name(np, "leds"); >>>>> + if (!led_node) >>>>> + return -ENODEV; >>>>> + >>>>> + 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) >>>>> + of_node_put(child); >>>> >>>> This of_node_put() looks odd to me. >>> Not sure if I get this right but if led setup fails of_node_put >>> should be called. >> >> My understanding is that this of_node_put() is there in case of error, >> to release what would otherwise never be released by >> for_each_available_child_of_node() if we exit early from the loop. >> >> If the purpose is to release a reference taken in max597x_setup_led() >> in case of error: >> - this should be done IMHO within max597x_setup_led() directly >> - there should be a of_node_get() somewhere in max597x_setup_led() >> (after: >> if (of_property_read_string(nc, "label", &led->led.name)) >> led->led.name = nc->name; >> + error handling path, *or* >> just before the final return ret; when we know that everything >> is fine, >> if I understand correctly the code) >> >> Is the reference taken elsewhere? >> Did I miss something obvious? >> >> > One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311 > Please do let me know if I have to do anything about it. By reference, I was speaking of reference taken by a of_node_get() call and released by a of_node_put() call. Anyway, I do agree with leds-sc27xx-bltc.c. There is a of_node_put() because for_each_available_child_of_node() won't be able to do it by itself *in case of early return* ("return err;") In all other paths (when the loop goes to the end), the reference taken by for_each_available_child_of_node() is also released, on the next iteration, by for_each_available_child_of_node(). In *your* case, if you don't break or return, there is no need to call of_node_put() explicitly. It would lead to a double put. (yours and the one that will be done by for_each_available_child_of_node()). Have a look at for_each_available_child_of_node() and more specifically at of_get_next_available_child(). At the first call 'child' is NULL. A ref is taken [1]. Nothing is released. For following calls, a new ref is taken on a new node [1], and the previous reference is released [2]. On the last call, the 'for' loop will not be executed because there is nothing to scan anymore. No new reference is taken, and the previous (and last) refence is finally released [2]. [1]: https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L808 [2]: https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L811 > >>>> "return ret;" or "break;" missing ? >>>> >>> Didn't add a break so that it can continue initializing remaining led >>> if any. >> >> Ok. Don't know the code enough to see if correct or not, but based on >> my comment above, I think that something is missing in >> max597x_setup_led() and that errors should be silently ignored here. > In my implementation, I have chosen to continue with the next LED if an > error occurs, rather than aborting the 'for loop' with an error. I have > seen other implementations also done in a similar way. > Do you have any further inputs or suggestions on this approach. No, sorry, I won't be of any help on what design is the best. CJ
Hi, On 27-03-2023 10:50 pm, Christophe JAILLET wrote: > Le 27/03/2023 à 17:47, Naresh Solanki a écrit : >> Hi, >> >> On 24-03-2023 09:06 pm, Christophe JAILLET wrote: >>> Le 24/03/2023 à 11:54, Naresh Solanki a écrit : >>>> Hi, >>>> >>>> On 24-03-2023 01:48 am, Christophe JAILLET wrote: >>>>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit : >>>>>> 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. >>>>>> >>> >>> [...] >>> >>> >>>>>> +static int max597x_led_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct device_node *np = dev_of_node(pdev->dev.parent); >>>>>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >>>>>> + struct device_node *led_node; >>>>>> + struct device_node *child; >>>>>> + int ret = 0; >>>>>> + >>>>>> + if (!regmap) >>>>>> + return -EPROBE_DEFER; >>>>>> + >>>>>> + led_node = of_get_child_by_name(np, "leds"); >>>>>> + if (!led_node) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + 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) >>>>>> + of_node_put(child); >>>>> >>>>> This of_node_put() looks odd to me. >>>> Not sure if I get this right but if led setup fails of_node_put >>>> should be called. >>> >>> My understanding is that this of_node_put() is there in case of >>> error, to release what would otherwise never be released by >>> for_each_available_child_of_node() if we exit early from the loop. >>> >>> If the purpose is to release a reference taken in max597x_setup_led() >>> in case of error: >>> - this should be done IMHO within max597x_setup_led() directly >>> - there should be a of_node_get() somewhere in max597x_setup_led() >>> (after: >>> if (of_property_read_string(nc, "label", &led->led.name)) >>> led->led.name = nc->name; >>> + error handling path, *or* >>> just before the final return ret; when we know that everything >>> is fine, >>> if I understand correctly the code) >>> >>> Is the reference taken elsewhere? >>> Did I miss something obvious? >>> >>> >> One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311 >> Please do let me know if I have to do anything about it. > By reference, I was speaking of reference taken by a of_node_get() call > and released by a of_node_put() call. > > Anyway, I do agree with leds-sc27xx-bltc.c. > There is a of_node_put() because for_each_available_child_of_node() > won't be able to do it by itself *in case of early return* ("return err;") > > In all other paths (when the loop goes to the end), the reference taken > by for_each_available_child_of_node() is also released, on the next > iteration, by for_each_available_child_of_node(). > > In *your* case, if you don't break or return, there is no need to call > of_node_put() explicitly. It would lead to a double put. (yours and the > one that will be done by for_each_available_child_of_node()). > > Have a look at for_each_available_child_of_node() and more specifically > at of_get_next_available_child(). > > At the first call 'child' is NULL. A ref is taken [1]. Nothing is released. > For following calls, a new ref is taken on a new node [1], and the > previous reference is released [2]. > On the last call, the 'for' loop will not be executed because there is > nothing to scan anymore. No new reference is taken, and the previous > (and last) refence is finally released [2]. Yes you are right. That of_node_put would be duplicate as it is already taken care by the for loop. Will remove that in next revision. > > > [1]: > https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L808 > [2]: > https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L811 > >> >>>>> "return ret;" or "break;" missing ? >>>>> >>>> Didn't add a break so that it can continue initializing remaining >>>> led if any. >>> >>> Ok. Don't know the code enough to see if correct or not, but based on >>> my comment above, I think that something is missing in >>> max597x_setup_led() and that errors should be silently ignored here. >> In my implementation, I have chosen to continue with the next LED if >> an error occurs, rather than aborting the 'for loop' with an error. I >> have seen other implementations also done in a similar way. >> Do you have any further inputs or suggestions on this approach. > > No, sorry, I won't be of any help on what design is the best. > > CJ > Regards, Naresh
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 9dbce09eabac..ec2b731ae545 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -590,6 +590,17 @@ config LEDS_ADP5520 To compile this driver as a module, choose M here: the module will be called leds-adp5520. +config LEDS_MAX597X + tristate "LED Support for Maxim 597x" + depends on LEDS_CLASS + depends on MFD_MAX597X + help + This option enables support for the Maxim 597x smart switch indication LEDs + via the I2C bus. + + To compile this driver as a module, choose M here: the module will + be called max597x-led. + config LEDS_MC13783 tristate "LED Support for MC13XXX PMIC" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index d30395d11fd8..da1192e40268 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -53,6 +53,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_MAX597X) += leds-max597x.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-max597x.c b/drivers/leds/leds-max597x.c new file mode 100644 index 000000000000..3e1747c8693e --- /dev/null +++ b/drivers/leds/leds-max597x.c @@ -0,0 +1,112 @@ +// 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/max597x.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define ldev_to_maxled(c) container_of(c, struct max597x_led, led) + +struct max597x_led { + struct regmap *regmap; + struct led_classdev led; + unsigned int index; +}; + +static int max597x_led_set_brightness(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct max597x_led *led = ldev_to_maxled(cdev); + int ret, val = 0; + + if (!led || !led->regmap) + return -ENODEV; + + val = !brightness ? BIT(led->index) : 0; + ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, BIT(led->index), val); + if (ret < 0) + dev_err(cdev->dev, "failed to set brightness %d\n", ret); + return ret; +} + +static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc, + u32 reg) +{ + struct max597x_led *led; + int ret = 0; + + led = devm_kzalloc(dev, sizeof(struct max597x_led), + GFP_KERNEL); + if (!led) + return -ENOMEM; + + if (of_property_read_string(nc, "label", &led->led.name)) + led->led.name = nc->name; + + led->led.max_brightness = 1; + led->led.brightness_set_blocking = max597x_led_set_brightness; + led->led.default_trigger = "none"; + led->index = reg; + led->regmap = regmap; + ret = led_classdev_register(dev, &led->led); + if (ret) + dev_err(dev, "Error in initializing led %s", led->led.name); + + return ret; +} + +static int max597x_led_probe(struct platform_device *pdev) +{ + struct device_node *np = dev_of_node(pdev->dev.parent); + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); + struct device_node *led_node; + struct device_node *child; + int ret = 0; + + if (!regmap) + return -EPROBE_DEFER; + + led_node = of_get_child_by_name(np, "leds"); + if (!led_node) + return -ENODEV; + + 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) + of_node_put(child); + } + + return ret; +} + +static struct platform_driver max597x_led_driver = { + .driver = { + .name = "max597x-led", + }, + .probe = max597x_led_probe, +}; + +module_platform_driver(max597x_led_driver); + +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); +MODULE_LICENSE("GPL");