| Message ID | 20250318-leds-tps6131x-v2-0-bc09c7a50b2e@emfend.at |
|---|---|
| Headers | show |
| Series | Support for Texas Instruments TPS6131X flash LED driver | expand |
On Tue, Mar 18, 2025 at 08:28:57AM +0100, Matthias Fend wrote: > Document Texas Instruments TPS61310/TPS61311 flash LED driver devicetree > bindings. > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at> > --- > .../devicetree/bindings/leds/ti,tps61310.yaml | 120 +++++++++++++++++++++ > 1 file changed, 120 insertions(+) Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 19/03/2025 17:25, Matthias Fend wrote: >>> + >>> + if (reg3 & TPS6131X_REG_3_HPFL) >>> + *fault |= LED_FAULT_SHORT_CIRCUIT; >>> + >>> + if (reg3 & TPS6131X_REG_3_SELSTIM_TO) >>> + *fault |= LED_FAULT_TIMEOUT; >>> + >>> + if (reg4 & TPS6131X_REG_4_HOTDIE_HI) >>> + *fault |= LED_FAULT_OVER_TEMPERATURE; >>> + >>> + if (reg6 & (TPS6131X_REG_6_LEDHOT | TPS6131X_REG_6_LEDWARN)) >>> + *fault |= LED_FAULT_LED_OVER_TEMPERATURE; >>> + >>> + if (!(reg6 & TPS6131X_REG_6_LEDHDR)) >>> + *fault |= LED_FAULT_UNDER_VOLTAGE; >>> + >>> + if (reg6 & TPS6131X_REG_6_LEDHOT) { >>> + ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_6, >>> + TPS6131X_REG_6_LEDHOT, 0, NULL, false, true); >> >> And this is not locked? > > The read modify write operation is protected by regmap. Since this > operation does not interact with any other functions, no lock is needed > here. Following that logic no lock is needed in the first place. Define what is the purpose of this lock, not just "hardware access". I assumed you want to keep consistent hardware state between multiple updates. If that's correct, how did you prevent returning value from reads happening in the middle of concurrent update? Or how this update_bits_base is prevented from happening while you are in the middle of earlier calls which are protected by your lock? That's confusing lock, considering also too short comment explaining its purpose. > >> >>> + if (ret < 0) >>> + return ret; >>> + } >>> + >> >> ... >> >>> + >>> +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; >>> + >>> + if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS)) >> >> Why builtin? That's a tristate, so I don't get why driver and v4l flash >> cannot be modules. You wanted REACHABLE probably... but then it is >> anyway discouraged practice leading to runtime debugging. So actually >> you want CONFIG_V4L2_FLASH_LED_CLASS || !CONFIG_V4L2_FLASH_LED_CLASS >> dependency. > > Okay, I'll add 'depends on V4L2_FLASH_LED_CLASS || > !V4L2_FLASH_LED_CLASS' to the Kconfig entry and do the check in the > driver like this: Only this > if (!IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)) > return 0; > > Is this solution okay for you? This should should not be needed, because there are v4l2 stubs. Best regards, Krzysztof
Hi Krzysztof, Am 19.03.2025 um 20:28 schrieb Krzysztof Kozlowski: > On 19/03/2025 17:25, Matthias Fend wrote: >>>> + >>>> + if (reg3 & TPS6131X_REG_3_HPFL) >>>> + *fault |= LED_FAULT_SHORT_CIRCUIT; >>>> + >>>> + if (reg3 & TPS6131X_REG_3_SELSTIM_TO) >>>> + *fault |= LED_FAULT_TIMEOUT; >>>> + >>>> + if (reg4 & TPS6131X_REG_4_HOTDIE_HI) >>>> + *fault |= LED_FAULT_OVER_TEMPERATURE; >>>> + >>>> + if (reg6 & (TPS6131X_REG_6_LEDHOT | TPS6131X_REG_6_LEDWARN)) >>>> + *fault |= LED_FAULT_LED_OVER_TEMPERATURE; >>>> + >>>> + if (!(reg6 & TPS6131X_REG_6_LEDHDR)) >>>> + *fault |= LED_FAULT_UNDER_VOLTAGE; >>>> + >>>> + if (reg6 & TPS6131X_REG_6_LEDHOT) { >>>> + ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_6, >>>> + TPS6131X_REG_6_LEDHOT, 0, NULL, false, true); >>> >>> And this is not locked? >> >> The read modify write operation is protected by regmap. Since this >> operation does not interact with any other functions, no lock is needed >> here. > > > Following that logic no lock is needed in the first place. Define what > is the purpose of this lock, not just "hardware access". I assumed you > want to keep consistent hardware state between multiple updates. If > that's correct, how did you prevent returning value from reads happening > in the middle of concurrent update? Or how this update_bits_base is > prevented from happening while you are in the middle of earlier calls > which are protected by your lock? > > That's confusing lock, considering also too short comment explaining its > purpose. Registers 0, 1, 2, and 3 control parts of the controller that are not completely independent of each other. For some operations, it is therefore necessary to write to the registers in a specific order to avoid unwanted side effects. Therefore, I protected write access to these registers with a lock. The RMW sequence in regmap_update_bits_base uses the cached value of the register and does not read from the hardware. Explicit reads to the status registers can be performed at any time. If a flag is set, this can be reported. Since regmap_read_bypassed actually reads from the hardware but doesn't update the cache, this isn't a problem either. Therefore, I don't see any need for a lock here. My suggestion would be to expand the comment as follows: /* Hardware access lock for register 0, 1, 2 and 3 */ and add this additional note before it: /* * Registers 0, 1, 2, and 3 control parts of the controller that are not completely * independent of each other. Since some operations require the registers to be written in * a specific order to avoid unwanted side effects, they are synchronized with a lock. */ Do you think that's acceptable? > >> >>> >>>> + if (ret < 0) >>>> + return ret; >>>> + } >>>> + >>> >>> ... >>> >>>> + >>>> +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; >>>> + >>>> + if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS)) >>> >>> Why builtin? That's a tristate, so I don't get why driver and v4l flash >>> cannot be modules. You wanted REACHABLE probably... but then it is >>> anyway discouraged practice leading to runtime debugging. So actually >>> you want CONFIG_V4L2_FLASH_LED_CLASS || !CONFIG_V4L2_FLASH_LED_CLASS >>> dependency. >> >> Okay, I'll add 'depends on V4L2_FLASH_LED_CLASS || >> !V4L2_FLASH_LED_CLASS' to the Kconfig entry and do the check in the >> driver like this: > > Only this > >> if (!IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)) >> return 0; >> >> Is this solution okay for you? > > This should should not be needed, because there are v4l2 stubs. True, it works that way too, you're right, of course. I was initially tempted by the many 'IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)' in the other drivers, and then by the requested switch to an early return. I will now remove the remaining early return as well. Thanks! ~Matthias > > Best regards, > Krzysztof
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 175m Signed-off-by: Matthias Fend <matthias.fend@emfend.at> --- Changes in v2: - Bindings: Extend device description - Bindings: Drop unused address/size cells - Bindings: Use fallback compatible - Bindings: Corrected minimum current for 50mA steps - Bindings: Drop node label - Fix name of REGISTER4 INDC shift define - Save device instead i2c_client in private data - Add comment for mutex - Use macro to convert from uA to mA - Use defines to describe initial register values - Add safety delay during reset sequence - Use fixed value enum to set the mode - Renamed some local variables - Re-sorted local variables - Replaced ifdefs for V4L2_FLASH_LED_CLASS - Improved some error messages - Link to v1: https://lore.kernel.org/r/20250228-leds-tps6131x-v1-0-d1071d90f9ea@emfend.at --- Matthias Fend (2): dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver .../devicetree/bindings/leds/ti,tps61310.yaml | 120 ++++ MAINTAINERS | 7 + drivers/leds/flash/Kconfig | 11 + drivers/leds/flash/Makefile | 1 + drivers/leds/flash/leds-tps6131x.c | 794 +++++++++++++++++++++ 5 files changed, 933 insertions(+) --- base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04 change-id: 20250227-leds-tps6131x-a7437883e400 Best regards,