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 LT8491 #2639

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

Conversation

jemfgeronimo
Copy link

PR Description

  • This adds driver support, and device tree bindings for LT8491 Battery Charge Controller
  • Datasheet: LT8491
  • Tested on RPI4 with DC2703A

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)

@jemfgeronimo jemfgeronimo force-pushed the dev/lt8491 branch 3 times, most recently from 54a629b to 4c40e24 Compare October 30, 2024 08:27
Add documentation for devicetree bindings for LT8491

Signed-off-by: John Erasmus Mari Geronimo <[email protected]>
LT8491 High Voltage Buck-Boost Battery Charge Controller with I2C

Signed-off-by: John Erasmus Mari Geronimo <[email protected]>
Add entry for the LT8491 driver.

Signed-off-by: John Erasmus Mari Geronimo <[email protected]>
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.

Here it goes my first round

description: Resistance value of RFBIN2 in ohms.

adi,rfbin1-ohms:
description: Resistance value of RFBIN1 in ohms.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any constrains for the resistor values (or default values)?

- adi,rfbout2-ohms
- adi,rdaci-ohms
- adi,rfbin2-ohms
- adi,rfbin1-ohms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to make sure. Are all these properties really mandatory?

if (!info)
return -ENOMEM;

i2c_set_clientdata(client, info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems you only have this because you pass the client in lt8491_configure_telemetry()? Why not just passing info?

ret = lt8491_configure_resistor(client, "adi,rfbin1-ohms", 100,
LT8491_CFG_RFBIN1_REG);
if (ret < 0)
return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

return lt8491_configure_resistor()

if (ret)
return ret;

return val;
Copy link
Collaborator

Choose a reason for hiding this comment

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

??

return serial_number[i];
}

ret = sprintf(strval, "%04x%04x%04x", serial_number[0], serial_number[1], serial_number[2]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

sysfs_emit()


static s32 lt8491_update_telemetry(struct lt8491_info *info)
{
return i2c_smbus_write_byte_data(info->client, LT8491_CTRL_UPDATE_TELEM_REG, 0xAA);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't think this helper to be much helpful...


return 0;
case POWER_SUPPLY_PROP_MANUFACTURER:
val->strval = "Linear Technology Corporation";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be Analog Devices now?


ret = i2c_smbus_read_word_data(info->client, LT8491_TELE_TBAT_REG);
if (ret < 0)
return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if (at least) the above 4 cases won't need a lock? Another question that might make sense for the lock need is... why do we always need to call lt8491_update_telemetry()?


return 0;
case POWER_SUPPLY_PROP_SERIAL_NUMBER:
ret = lt8491_read_serial_number(info, strval);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I missing something or strval is just some garbage unallocated pointer?

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