| 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: <8a16c018-8466-4dea-8f1e-e8a65e3ed950@emfend.at>
Date: Fri, 14 Mar 2025 12:27:33 +0100
From: Matthias Fend <matthias.fend@...end.at>
To: Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments
TPS6131X flash LED driver
Hi Lee,
Am 14.03.2025 um 11:52 schrieb Lee Jones:
> On Fri, 14 Mar 2025, Matthias Fend wrote:
>
>> Hi Lee,
>>
>> thanks a lot for your feedback!
>>
>> Am 10.03.2025 um 15:49 schrieb Lee Jones:
>>> On Fri, 28 Feb 2025, Matthias Fend wrote:
>>>
>>>> The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
>>>> stage is capable of supplying a maximum total current of roughly 1500mA.
>>>> The TPS6131x provides three constant-current sinks, capable of sinking up
>>>> to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
>>>> each sink (LED1, LED2, LED3) supports currents up to 175mA.
>>>>
>>>> Signed-off-by: Matthias Fend <matthias.fend@...end.at>
>>>> ---
>>>> MAINTAINERS | 7 +
>>>> drivers/leds/flash/Kconfig | 11 +
>>>> drivers/leds/flash/Makefile | 1 +
>>>> drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 817 insertions(+)
>
> [...]
>
>>>> +static int tps6131x_led_class_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> + struct led_classdev *led_cdev;
>>>> + struct led_flash_setting *setting;
>>>> + struct led_init_data init_data = {};
>>>> + static const struct tps6131x_timer_config *timer_config;
>>>> + int ret;
>>>> +
>>>> + tps6131x->fled_cdev.ops = &flash_ops;
>>>> +
>>>> + setting = &tps6131x->fled_cdev.timeout;
>>>> + timer_config = tps6131x_find_closest_timer_config(0);
>>>> + setting->min = timer_config->time_us;
>>>> + setting->max = tps6131x->max_timeout_us;
>>>> + setting->step = 1; /* Only some specific time periods are supported. No fixed step size. */
>>>> + setting->val = setting->min;
>>>> +
>>>> + setting = &tps6131x->fled_cdev.brightness;
>>>> + setting->min = tps6131x->step_flash_current_ma;
>>>> + setting->max = tps6131x->max_flash_current_ma;
>>>> + setting->step = tps6131x->step_flash_current_ma;
>>>> + setting->val = setting->min;
>>>> +
>>>> + led_cdev = &tps6131x->fled_cdev.led_cdev;
>>>> + led_cdev->brightness_set_blocking = tps6131x_brightness_set;
>>>> + led_cdev->max_brightness = tps6131x->max_torch_current_ma;
>>>> + led_cdev->flags |= LED_DEV_CAP_FLASH;
>>>> +
>>>> + init_data.fwnode = tps6131x->led_node;
>>>> + init_data.devicename = NULL;
>>>> + init_data.default_label = NULL;
>>>> + init_data.devname_mandatory = false;
>>>> +
>>>> + ret = devm_led_classdev_flash_register_ext(&tps6131x->client->dev, &tps6131x->fled_cdev,
>>>> + &init_data);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
>>>
>>> Not keen on #ifery in C files.
>>>
>>> Can you use is_defined() and return early instead?
>>>
>>> I see that there is a precedent for this already. :(
>>
>> Me neither, but since it is done this way in about 9 out of 10 flash
>> controllers, I wanted to continue doing it consistently.
>> But since the required v4l2_flash_* functions are also available as dummies
>> if this option is not activated, I could do it like this:
>>
>> if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS))
>> return 0;
>>
>> Would you prefer this solution?
>
> I would, yes. Thank you.
Sure, then I'll change that.
>
>>>> +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
>>>> +{
>>>> + struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>>>> + struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>>>> +
>>>> + guard(mutex)(&tps6131x->lock);
>>>> +
>>> /> + return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
>>>> + false);
>>>> +}
>>>> +
>>>> +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
>>>> + .external_strobe_set = tps6131x_flash_external_strobe_set,
>>>> +};
>>>> +
>>>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> + struct v4l2_flash_config v4l2_cfg = { 0 };
>>>> + struct led_flash_setting *intensity = &v4l2_cfg.intensity;
>>>> +
>>>> + intensity->min = tps6131x->step_torch_current_ma;
>>>> + intensity->max = tps6131x->max_torch_current_ma;
>>>> + intensity->step = tps6131x->step_torch_current_ma;
>>>> + intensity->val = intensity->min;
>>>> +
>>>> + strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
>>>
>>> tps6131x->client->dev?
>>
>> Do you mean the name should be taken from the I2C device?
>> The current name, for example, is 'white:flash-0', while the I2C device name
>> would be '4-0033'. So I think the current version is appropriate, don't you
>> think?
>
> No, I'm implying that:
>
> tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev
>
> ... and that the former is shorter / neater.
Hmm. That's interesting. I thought these were two different devices,
which seems to be actually the case for me. Hence the different names in
the kobj.
Are the devices really supposed to be identical?
>
>>>> + sizeof(v4l2_cfg.dev_name));
>>>> +
>>>> + v4l2_cfg.has_external_strobe = true;
>>>> + v4l2_cfg.flash_faults = LED_FAULT_TIMEOUT | LED_FAULT_OVER_TEMPERATURE |
>>>> + LED_FAULT_SHORT_CIRCUIT | LED_FAULT_UNDER_VOLTAGE |
>>>> + LED_FAULT_LED_OVER_TEMPERATURE;
>>>> +
>>>> + tps6131x->v4l2_flash = v4l2_flash_init(&tps6131x->client->dev, tps6131x->led_node,
>>>> + &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
>>>> + &v4l2_cfg);
>>>> + if (IS_ERR(tps6131x->v4l2_flash)) {
>>>> + dev_err(&tps6131x->client->dev, "v4l2_flash_init failed\n");
>>>
>>> "Failed to initialise V4L2 flash LED" ?
>>
>> ACK
>>
>>>
>>>> + return PTR_ERR(tps6131x->v4l2_flash);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +#else
>>>> +
>>>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +#endif
>>>> +
>>>> +static int tps6131x_probe(struct i2c_client *client)
>>>> +{
>>>> + struct tps6131x *tps6131x;
>>>> + int ret;
>>>> +
>>>> + tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
>>>> + if (!tps6131x)
>>>> + return -ENOMEM;
>>>> +
>>>> + tps6131x->client = client;
>>>
>>> What are you planning on using client for?
>>>
>>>> + i2c_set_clientdata(client, tps6131x);
>>>
>>> How are you going to _get_ this without client?
>>
>> Maybe I didn't understand the question correctly, but in tps6131x_remove() I
>> get the device data via the client.
>
> Right, which uses 'client' to obtain it, so you don't need to save 'client'.
Okay, now I understand. Since I'm now saving 'dev' instead of 'client,'
as you suggested, that's no longer an issue.
>
>>> Why not save dev and reduce the amount of dereferencing levels required.
>>
>> Absolutely. Good idea.
>>
>>>
>>>> + mutex_init(&tps6131x->lock);
>>>> + INIT_DELAYED_WORK(&tps6131x->torch_refresh_work, tps6131x_torch_refresh_handler);
>>>> +
>>>> + ret = tps6131x_parse_node(tps6131x);
>>>> + if (ret)
>>>> + return -ENODEV;
>>>> +
>>>> + tps6131x->regmap = devm_regmap_init_i2c(client, &tps6131x_regmap);
>>>> + if (IS_ERR(tps6131x->regmap)) {
>>>> + ret = PTR_ERR(tps6131x->regmap);
>>>> + dev_err(&client->dev, "Failed to allocate register map\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>>>> + ret = tps6131x_reset_chip(tps6131x);
>>>> + if (ret)
>>>> + return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
>>>
>>> How do you manage the optional part?
>>
>> If there is a reset line, then tps6131x_reset_chip() uses it to reset the
>> chip. If there is none, the software reset (via an I2C register) is used.
>> Therefore the reset pin can be optional.
>
> Right, but didn't you just fail if one is not provided, or is that
> accounted for in tps6131x_reset_chip()?
Yes, tps6131x_reset_chip() selects the appropriate reset method
depending on the circumstances. Something like this:
if (IS_ERR_OR_NULL(tps6131x->reset_gpio))
sofware reset via i2c
else
hardware reset via gpio
So there's no error just because the reset GPIO isn't present.
Thanks
~Matthias
>
Powered by blists - more mailing lists