| 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
| ||
|
Message-Id: <DG75VP6IIUXD.1VA6YSMNTPJQ6@disroot.org>
Date: Thu, 05 Feb 2026 21:46:34 +0530
From: "Kaustabh Chakraborty" <kauschluss@...root.org>
To: André Draszik <andre.draszik@...aro.org>, "Kaustabh
Chakraborty" <kauschluss@...root.org>, "Lee Jones" <lee@...nel.org>, "Pavel
Machek" <pavel@...nel.org>, "Rob Herring" <robh@...nel.org>, "Krzysztof
Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
"MyungJoo Ham" <myungjoo.ham@...sung.com>, "Chanwoo Choi"
<cw00.choi@...sung.com>, "Sebastian Reichel" <sre@...nel.org>, "Krzysztof
Kozlowski" <krzk@...nel.org>, "Alexandre Belloni"
<alexandre.belloni@...tlin.com>, "Jonathan Corbet" <corbet@....net>, "Shuah
Khan" <skhan@...uxfoundation.org>
Cc: <linux-leds@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
<linux-samsung-soc@...r.kernel.org>, <linux-rtc@...r.kernel.org>,
<linux-doc@...r.kernel.org>
Subject: Re: [PATCH v2 08/12] leds: flash: add support for Samsung S2M
series PMIC flash LED device
On 2026-02-04 16:55 +00:00, André Draszik wrote:
> Hi,
>
> On Mon, 2026-01-26 at 00:37 +0530, Kaustabh Chakraborty wrote:
>> Add support for flash LEDs found in certain Samsung S2M series PMICs.
>> The device has two channels for LEDs, typically for the back and front
>> cameras in mobile devices. Both channels can be independently
>> controlled, and can be operated in torch or flash modes.
>>
>> The driver includes initial support for the S2MU005 PMIC flash LEDs.
>>
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@...root.org>
>> ---
>> drivers/leds/flash/Kconfig | 12 ++
>> drivers/leds/flash/Makefile | 1 +
>> drivers/leds/flash/leds-s2m-flash.c | 410 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 423 insertions(+)
>>
>> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
>> index 5e08102a67841..be62e05277429 100644
>> --- a/drivers/leds/flash/Kconfig
>> +++ b/drivers/leds/flash/Kconfig
>> @@ -114,6 +114,18 @@ config LEDS_RT8515
>> To compile this driver as a module, choose M here: the module
>> will be called leds-rt8515.
>>
>> +config LEDS_S2M_FLASH
>> + tristate "Samsung S2M series PMICs flash/torch LED support"
>> + depends on LEDS_CLASS
>> + depends on MFD_SEC_CORE
>> + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>> + select REGMAP_IRQ
>> + help
>> + This option enables support for the flash/torch LEDs found in
>> + certain Samsung S2M series PMICs, such as the S2MU005. It has
>> + a LED channel dedicated for every physical LED. The LEDs can
>> + be controlled in flash and torch modes.
>> +
>> config LEDS_SGM3140
>> tristate "LED support for the SGM3140"
>> depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
>> index 712fb737a428e..44e6c1b4beb37 100644
>> --- a/drivers/leds/flash/Makefile
>> +++ b/drivers/leds/flash/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o
>> obj-$(CONFIG_LEDS_QCOM_FLASH) += leds-qcom-flash.o
>> obj-$(CONFIG_LEDS_RT4505) += leds-rt4505.o
>> obj-$(CONFIG_LEDS_RT8515) += leds-rt8515.o
>> +obj-$(CONFIG_LEDS_S2M_FLASH) += leds-s2m-flash.o
>> obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
>> obj-$(CONFIG_LEDS_SY7802) += leds-sy7802.o
>> obj-$(CONFIG_LEDS_TPS6131X) += leds-tps6131x.o
>> diff --git a/drivers/leds/flash/leds-s2m-flash.c b/drivers/leds/flash/leds-s2m-flash.c
>> new file mode 100644
>> index 0000000000000..1be2745c475bf
>> --- /dev/null
>> +++ b/drivers/leds/flash/leds-s2m-flash.c
>> @@ -0,0 +1,410 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Flash and Torch LED Driver for Samsung S2M series PMICs.
>> + *
>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd
>> + * Copyright (c) 2025 Kaustabh Chakraborty <kauschluss@...root.org>
>> + */
>> +
>> +#include <linux/container_of.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/mfd/samsung/core.h>
>> +#include <linux/mfd/samsung/s2mu005.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <media/v4l2-flash-led-class.h>
>> +
>> +#define MAX_CHANNELS 2
>> +
>> +struct s2m_fled {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct led_classdev_flash cdev;
>> + struct v4l2_flash *v4l2_flash;
>> + struct mutex lock;
>
> Please add a (brief) comment describing what the mutex protects.
The mutex object prevents the concurrent access of flash control
registers by the LED and V4L2 subsystems. -- will add this.
>> +
>> + /*
>> + * Get the LED enable register address. Revision EVT0 has the
>> + * register at CTRL4, while EVT1 and higher have it at CTRL6.
>> + */
>> + if (priv->pmic_revision == 0)
>> + reg_enable = S2MU005_REG_FLED_CTRL4;
>> + else
>> + reg_enable = S2MU005_REG_FLED_CTRL6;
>
> You could REG_FIELD() and friends for this and everywhere else with
> similar if / else.
>
REG_FIELD(), from what I understood, is for selecting a bit field inside
a single register. However this code chooses between two separate
registers. I believe your interpretation was incorrect? Please clarify.
>> +static int s2m_fled_init_channel(struct device *dev, struct fwnode_handle *fwnp,
>> + struct s2m_fled *priv)
>> +{
>> + struct led_classdev *led = &priv->cdev.led_cdev;
>> + struct led_init_data init_data = {};
>> + struct v4l2_flash_config v4l2_cfg = {};
>> + int ret;
>> +
>> + led->max_brightness = priv->spec->torch_max_brightness;
>> + led->brightness_set_blocking = priv->spec->torch_brightness_set_blocking;
>> + led->flags |= LED_DEV_CAP_FLASH;
>> +
>> + priv->cdev.timeout.min = priv->spec->flash_min_timeout_us;
>> + priv->cdev.timeout.step = priv->spec->flash_min_timeout_us;
>> + priv->cdev.timeout.max = priv->spec->flash_max_timeout_us;
>> + priv->cdev.timeout.val = priv->spec->flash_max_timeout_us;
>> +
>> + priv->cdev.brightness.min = priv->spec->flash_min_current_ua;
>> + priv->cdev.brightness.step = priv->spec->flash_min_current_ua;
>> + priv->cdev.brightness.max = priv->spec->flash_max_current_ua;
>> + priv->cdev.brightness.val = priv->spec->flash_max_current_ua;
>> +
>> + s2m_fled_flash_timeout_set(&priv->cdev, priv->cdev.timeout.val);
>> + s2m_fled_flash_brightness_set(&priv->cdev, priv->cdev.brightness.val);
>> +
>> + priv->cdev.ops = priv->spec->flash_ops;
>> +
>> + init_data.fwnode = fwnp;
>> + ret = devm_led_classdev_flash_register_ext(dev, &priv->cdev, &init_data);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to create LED flash device\n");
>> + return ret;
>
> dev_err_probe()?
>
>> + }
>> +
>> + v4l2_cfg.intensity.min = priv->spec->flash_min_current_ua;
>> + v4l2_cfg.intensity.step = priv->spec->flash_min_current_ua;
>> + v4l2_cfg.intensity.max = priv->spec->flash_max_current_ua;
>> + v4l2_cfg.intensity.val = priv->spec->flash_max_current_ua;
>> +
>> + v4l2_cfg.has_external_strobe = true;
>> +
>> + priv->v4l2_flash = v4l2_flash_init(dev, fwnp, &priv->cdev,
>> + &s2m_fled_v4l2_flash_ops, &v4l2_cfg);
>> + if (IS_ERR(priv->v4l2_flash)) {
>> + dev_err(dev, "failed to create V4L2 flash device\n");
>> + v4l2_flash_release(priv->v4l2_flash);
>> + return PTR_ERR(priv->v4l2_flash);
>
> dev_err_probe()?
>
>> + }
>> +
>> + return devm_add_action_or_reset(dev, (void *)v4l2_flash_release,
>> + priv->v4l2_flash);
>
> maybe add dev_err_probe() here, and drop the extra message in s2m_fled_probe().
>
>> +}
>> +
>> +static int s2m_fled_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct sec_pmic_dev *pmic_drvdata = dev_get_drvdata(dev->parent);
>> + struct s2m_fled *priv;
>> + struct fwnode_handle *child;
>> + struct regmap *regmap;
>> + const struct s2m_fled_spec *spec;
>> + int ret;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv) * MAX_CHANNELS, GFP_KERNEL);
>> + if (!priv)
>> + return dev_err_probe(dev, -ENOMEM, "failed to allocate driver private\n");
>> +
>> + platform_set_drvdata(pdev, priv);
>> + regmap = pmic_drvdata->regmap_pmic;
>> +
>> + switch (platform_get_device_id(pdev)->driver_data) {
>> + case S2MU005:
>> + spec = &s2mu005_fled_spec;
>> + /* Enable the LED channels. */
>> + ret = regmap_set_bits(regmap, S2MU005_REG_FLED_CTRL1,
>> + S2MU005_FLED_CH_EN);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "failed to enable LED channels\n");
>> + break;
>> + default:
>> + return dev_err_probe(dev, -ENODEV,
>> + "device type %d is not supported by driver\n",
>> + pmic_drvdata->device_type);
>> + }
>> +
>> + device_for_each_child_node(dev, child) {
>> + u32 reg;
>> +
>> + if (fwnode_property_read_u32(child, "reg", ®))
>> + goto next_child;
>> +
>> + if (reg >= spec->num_channels) {
>> + dev_warn(dev, "channel %d is non-existent\n", reg);
>> + goto next_child;
>> + }
>> +
>> + if (priv[reg].dev) {
>> + dev_warn(dev, "duplicate node for channel %d\n", reg);
>> + goto next_child;
>> + }
>> +
>> + priv[reg].dev = dev;
>> + priv[reg].regmap = regmap;
>> + priv[reg].channel = (u8)reg;
>> + priv[reg].spec = spec;
>> + priv[reg].pmic_revision = pmic_drvdata->revision;
>> +
>> + ret = devm_mutex_init(dev, &priv[reg].lock);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "failed to create mutex lock\n");
>> +
>> + ret = s2m_fled_init_channel(dev, child, &priv[reg]);
>> + if (ret < 0)
>> + dev_warn(dev, "channel init failed (%d)\n", ret);
>
> s2m_fled_init_channel() already prints a message on (most) errors, and then
> there's another one here. Also, is it really OK to continue ignoring the
> error?
>
The LED channels are supposed to be independent, so if one fails, the
others may still work.
But errors in s2m_fled_init_channel() is not-hardware related, so this
reasoning doesn't make sense. So I shall implement your changes.
Powered by blists - more mailing lists