Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for LTC7871 #2690

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

adiceline
Copy link
Collaborator

PR Description

  • This adds driver support and device tree bindings for LTC7871 Six-Phase
    Buck-Boost Converter.
  • Datasheet: LTC7871
  • Tested on RPI4 using the following demo board: DC2886A

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly (if there is the case)

drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
@adiceline adiceline force-pushed the dev/ltc7871 branch 2 times, most recently from a87a3cb to bfcb9d3 Compare January 30, 2025 09:02
@adiceline
Copy link
Collaborator Author

V2

DT Bindings

  • Updated commit message
  • Updated enable-chip-ctrl-wp description
  • Renamed freq-spread-range to freq-spread-percentage, changed type to string, and updated description
  • Renamed modulation-signal-freq to switching-freq-divider and updated description

Kconfig

  • Updated Kconfig to reflect that LTC7871depends on both SPI and OF

Driver

  • Added comment for default values
  • Added of_device_id
  • Added devm_regulator_register
  • Added contraints for ra_ext, rb_ext, rc_ext, rd_ext, and idac_setcur_uA
  • Changed ltc7871_freq_spread_percentage from unsigned int to char *
  • Used struct ltc7871 *ltc7871 as argument for _ltc7871_uV_to_dac and _ltc7871_dac_to_uV instead of r1 and r2
  • Fixed formatting issues

drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
drivers/regulator/ltc7871-regulator.c Outdated Show resolved Hide resolved
divided to get the modulation frequency.
$ref: /schemas/types.yaml#/definitions/uint32
enum: [512, 1024, 2048, 4096, 256, 128, 64]
default: 512
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we somehow express this in frequency and do the math in the driver to get the actual divider?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switching frequency is set via another external resistor which is arbitrary which means that the modulation frequency cannot be computed without prior knowledge of this resistor. Thus, we cannot directly use the modulation frequency as a property because it doesn't have fixed values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to make the user input an integer for the modulation frequency, do the math in the driver, then round to the nearest valid value for the divisor. However, if we do this, the user will not be able to readback the actual value of the modulation frequency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, if we do this, the user will not be able to readback the actual value of the modulation frequency.

Why not? But for the above to work you would need to know the controller switching frequency so that you would need another resistor property right? I mean, if an external clock is used on the SYNC pin, it would be trivial to get the frequency but if you have the resistor on the FREQ pin, you would need to know it in order to do the math. Maybe let's go with the divider and see if upstream accepts it. Divider properties are not that uncommon after all.

But one thing though... you're missing an optional clock property for the SYNC pin when an external clock is used (should also be enabled in the driver). You could also add some regulator properties for the chip supplies.

The LTC7871 is a bidirectional buck or boost
switching regulator controller that operates in
either buck or boost mode on demand. Add
corresponding DT bindings.

Signed-off-by: Celine Joy A. Capua <celinejoy.capua@analog.com>
Add ADI LTC7871 buck-boost controller driver support.

Signed-off-by: Celine Joy A. Capua <celinejoy.capua@analog.com>
imply REGULATOR_LTC7871.

Signed-off-by: Celine Joy A. Capua <celinejoy.capua@analog.com>
Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fairly good to send upstream. Take my last notes and please send the patches...

divided to get the modulation frequency.
$ref: /schemas/types.yaml#/definitions/uint32
enum: [512, 1024, 2048, 4096, 256, 128, 64]
default: 512
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, if we do this, the user will not be able to readback the actual value of the modulation frequency.

Why not? But for the above to work you would need to know the controller switching frequency so that you would need another resistor property right? I mean, if an external clock is used on the SYNC pin, it would be trivial to get the frequency but if you have the resistor on the FREQ pin, you would need to know it in order to do the math. Maybe let's go with the divider and see if upstream accepts it. Divider properties are not that uncommon after all.

But one thing though... you're missing an optional clock property for the SYNC pin when an external clock is used (should also be enabled in the driver). You could also add some regulator properties for the chip supplies.

return ret;
} else {
return -EINVAL;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typical pattern is to check for errors first... Flip the logic:

if (chip->idac_setcur_uA < LTC7871_IDAC_MIN ||
    chip->idac_setcur_uA > LTC7871_IDAC_MAX) {
    return dev_err_probe();

ret = ...
if (ret)
    return ret;

Makes the code neater. A log message with dev_err_probe() would also be nice so we know why we failed to probe (ditto for the other properties)

ret = device_property_read_u32(&chip->spi->dev, "adi,ra-external-ohms",
&chip->ra_ext);
if (!ret) {
if (chip->ra_ext <= 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just if (!chip->ra_ext). It's an unsigned value so checking for negative values is meaningless...

static s64 _ltc7871_dac_to_uV(struct ltc7871 *ltc7871, u32 dac_val)
{
return 1200 * (1000 + (div_s64(ltc7871->r2 * 1000, ltc7871->r1))) -
dac_val * ltc7871->r2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add a tmp variable as in _ltc7871_dac_to_uV() to make the above a bit more readable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants