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 ltc4162-f/s and ltc4015 battery charger #2594

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

Conversation

kseerp
Copy link
Contributor

@kseerp kseerp commented Sep 9, 2024

PR Description

Add support for:
LTC4162-F 35V/3.2A Multi-Cell LiFePO4 Step-Down Battery Charger
LTC4162-S 35V/3.2A Lead-Acid Step-Down Battery Charger
LTC4015 35V/3.2A Multichemistry Buck Battery Charger Controller

Bindings:

  • Add compatible entries for ltc4162-f/s and ltc4015
  • Include datasheets for new devices
  • Change vendor prefix from lltc to adi

ltc4162:

  • Modify functions for battery voltage/current, input voltage/current,
    charge voltage, die temp, and force telemetry to handle different
    battery chemistries.

PR Type

  • New feature (a change that adds new functionality)
  • Bug fix (a change that fixes an issue)
  • 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)

@kseerp kseerp changed the title add support ltc4162-f/s and ltc4015 batter charger add support for ltc4162-f/s and ltc4015 battery charger Sep 9, 2024
drivers/power/supply/ltc4162-l-charger.c Outdated Show resolved Hide resolved
drivers/power/supply/ltc4162-l-charger.c Outdated Show resolved Hide resolved
drivers/power/supply/ltc4162-l-charger.c Outdated Show resolved Hide resolved
drivers/power/supply/ltc4162-l-charger.c Outdated Show resolved Hide resolved
drivers/power/supply/ltc4162-l-charger.c Outdated Show resolved Hide resolved
@nunojsa
Copy link
Collaborator

nunojsa commented Sep 20, 2024

Also, the things I mentioned as "nice to have" is up to you to add it or not :)

Add support for ltc4162-f/s and ltc4015

- Add compatible entries for ltc4162-f/s and ltc4015
- Include datasheets for new devices

Signed-off-by: Kim Seer Paller <[email protected]>
Add support for
LTC4162-F 35V/3.2A Multi-Cell LiFePO4 Step-Down Battery Charger
LTC4162-S 35V/3.2A Lead-Acid Step-Down Battery Charger
LTC4015 35V/3.2A Multichemistry Buck Battery Charger Controller

Add chip_info struct to hold the chip specific data.
Modify functions for battery voltage/current, input voltage/current,
charge voltage, die temp, and force telemetry to handle different
battery chemistries.

Signed-off-by: Kim Seer Paller <[email protected]>
@kseerp
Copy link
Contributor Author

kseerp commented Oct 11, 2024

v2

  • Added chip_info struct to hold chip specific data.
  • Added functions specific to each chip to get/set vbat, vcharge, die_temp.
  • Duplicated the default descriptor to configure name based on chip_info.
  • Used FIELD_GET to extract the value from register.
  • Changed sprintf to sysfs_emit.

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.

Code looks mostly good to me. Just make sure to split your changes a bit better so it makes it easier to review. At this point, I would say to send this upstream...

@@ -840,6 +1153,12 @@ static int ltc4162l_probe(struct i2c_client *client,
info->client = client;
i2c_set_clientdata(client, info);

chip_info = device_get_match_data(dev);
Copy link
Collaborator

Choose a reason for hiding this comment

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

when upstreaming use i2c_get_match_data()

@@ -820,12 +1131,14 @@ static void ltc4162l_clear_interrupts(struct ltc4162l_info *info)
}

static int ltc4162l_probe(struct i2c_client *client,
const struct i2c_device_id *id)
const struct i2c_device_id *id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a stylistic change... You have more in this patch. Do them in a different patch (like a follow up one)

size_t count)
struct device_attribute *attr,
const char *buf,
size_t count)
Copy link
Collaborator

Choose a reason for hiding this comment

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

style...

return sprintf(buf, "%u\n",
regval == LTC4162L_ARM_SHIP_MODE_MAGIC ? 1 : 0);
return sysfs_emit(buf, "%u\n",
regval == LTC4162L_ARM_SHIP_MODE_MAGIC ? 1 : 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

also not related


return count;
return ret ? ret : count;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@@ -98,7 +138,7 @@ static u8 ltc4162l_get_cell_count(struct ltc4162l_info *info)
return 0;

/* Lower 4 bits is the cell count, or 0 if the chip doesn't know yet */
val &= 0x0f;
val = FIELD_GET(LTC4162L_CELL_COUNT_MASK, val);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also not related with addition of the new devices...

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.

2 participants