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 LTC7841 #2397

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

Conversation

MCabuena
Copy link

@MCabuena MCabuena commented Dec 12, 2024

Pull Request Description

Please replace this with a detailed description and motivation of the changes.
You can tick the checkboxes below with an 'x' between square brackets or just check them after publishing the PR.
If this PR contains a breaking change, list dependent PRs and try to push all related PRs at the same time.

PR Type

  • Bug fix (change that fixes an issue)
  • New feature (change that adds new functionality)
  • Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

  • I have followed the Coding style guidelines
  • I have performed a self-review of the changes
  • I have commented my code, at least hard-to-understand parts
  • I have build all projects affected by the changes in this PR
  • I have tested in hardware affected projects, at the relevant boards
  • I have signed off all commits from this PR
  • I have updated the documentation (wiki pages, ReadMe etc), if applies

@MCabuena MCabuena marked this pull request as ready for review December 12, 2024 02:33
@kister-jimenez
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@MCabuena MCabuena force-pushed the ltc7841_my_changes_to_run_ci branch from fcc8863 to 5b005cb Compare December 13, 2024 01:40
@MCabuena
Copy link
Author

V2:

  1. Removed from src.mk ltc7841_regs.c and ltc7841_regs.h requirement for building

@kister-jimenez
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@MCabuena MCabuena force-pushed the ltc7841_my_changes_to_run_ci branch from c57b81e to 16af219 Compare December 13, 2024 08:46
@MCabuena
Copy link
Author

V3:

  1. fixed src.mk file inside ltc7841 project folder

@kister-jimenez
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@@ -0,0 +1,199 @@
/***************************************************************************//**
Copy link
Contributor

Choose a reason for hiding this comment

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

This file has the old license. Use the 3 clause BSD instead.

uint8_t read_length;
uint8_t write_length = 1;
uint8_t size = get_register_size(cmd);
uint8_t write_buffer[write_length];
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't need to be an array

ret = no_os_i2c_write(desc->comm_desc, write_buffer, write_length, 0);
if (ret)
return ret;
// read part of write_read, 1 at the end for stop bit sending
Copy link
Contributor

Choose a reason for hiding this comment

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

Use /**/ comment style

if (ret)
return ret;
// read part of write_read, 1 at the end for stop bit sending
ret = no_os_i2c_read(desc->comm_desc, g_i2c0_read_buffer, read_length, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should store the register value in data.

{
int ret;
/* Write register */
ret = ltc7841_reg_write(desc, LTC7841_OPERATION, LTC7841_OPERATION_MARGIN_LOW);
Copy link
Contributor

Choose a reason for hiding this comment

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

Return directly. There are other places where this is the case.

Comment on lines 345 to 417
static struct iio_attribute ltc7841_chan_attrs[] = {
{
.name = "raw",
.show = ltc7841_iio_read_raw
},
END_ATTRIBUTES_ARRAY
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the scale of every channel equal to 1? Otherwise, you also need the scale attribute here.

Copy link
Author

Choose a reason for hiding this comment

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

The scale of every channel is the same at 10mV per bit.

Copy link
Author

Choose a reason for hiding this comment

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

on my next commit, I'll add as you stated below a scale attribute for the channel

Copy link
Contributor

Choose a reason for hiding this comment

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

When implementing review suggestions, you don't need to create a new commit (unless you add new functionality, which should have its own commit). Just modify the current one and push the changes again. After that, you should add a changelog comment in this PR stating what you did since the last push.

struct ltc7841_iio_desc *iio_ltc7841 = dev;
struct ltc7841_desc *ltc7841 = iio_ltc7841->ltc7841_desc;

ret = ltc7841_reg_read(ltc7841, (uint8_t)reg, (uint8_t *)readval);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an intermediary uin8_t variable. Currently, you're passing a different memory location based on endianess.

}
case LTC7841_IIO_IIN_CHAN:
ret = ltc7841_reg_read(ltc7841, (uint8_t)LTC7841_READ_IIN, read_value);
if (0 == ret) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use inverse logic:

if (ret)
    break;

case LTC7841_IIO_IIN_CHAN:
ret = ltc7841_reg_read(ltc7841, (uint8_t)LTC7841_READ_IIN, read_value);
if (0 == ret) {
double current_value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a lower unit (e.g microampere in this case), so you won't lose any digit during the division, and scale the result instead.

Comment on lines 64 to 66
#define PEC_SIZE 0x100
#define BYTE_LENGTH 1
#define WORD_LENGTH 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the LTC7841_ prefix. Otherwise, generic names like these could create conflicts with other macros.

.max_speed_hz = LTC7841_I2C_CLK_SPEED,
.slave_address = LTC7841_I2C_ADDR,
.platform_ops = &max_i2c_ops,
.extra = (void *)&ltc7841_i2c_extra,
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to cast.

@MCabuena MCabuena force-pushed the ltc7841_my_changes_to_run_ci branch from 298e71f to 6423294 Compare December 17, 2024 03:34
@MCabuena
Copy link
Author

V4:

  1. changed commenting style to /* */
  2. removed void in common_data.c for ltc7841_i2c_extra
  3. removed PEC_SIZE since this device doesn't support PEC
  4. added LTC7841_ name to BYTE_LENGTH and WORD_LENGTH in ltc7841.h
  5. when computing for current input and current output, unit usage is now in uA for initial calculations, only when calculating for scale values does it show in A
  6. changed iio_ltc7841.c all reverse logic for checking ret
  7. added scaled value as an attribute
  8. functions in ltc7841.c now return directly
  9. changed write_buffer into a uint8_t type only, and not as an array
  10. changed license file of ltc7841.h

@buha
Copy link
Contributor

buha commented Dec 17, 2024

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

}
return ret;
}
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a style remark but always leave a blank line from the closing bracket of a function to the next content like this

}

/**

reason is, we want the code style to be consistent

} else {
ret = -EINVAL;
}
return ret;
Copy link
Contributor

Choose a reason for hiding this comment

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

another style remark, it's kind of informal but we always put a blank line before the final return statement of the function like:

    }

    return ret;
}

*/
struct ltc7841_init_param {
/* Device register settings */
struct ltc7841_reg_st *regs;
Copy link
Contributor

Choose a reason for hiding this comment

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

i cant find where this struct comes from

Copy link
Contributor

Choose a reason for hiding this comment

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

is the user supposed to provide the register configuration explicitly ? i dont think this approach is best, i think the driver should abstract away as much as possible the registers and not force the user to read the datasheet and provide register values

Copy link
Author

Choose a reason for hiding this comment

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

Hi buha,

It used to be that I have a list of registers I put on a list and populated that. I changed the approach and abstracted it. I wasn't able to remove it on this version of the commit.

I'll remove that part for the next version which I plan to also address your comments.

INCS += $(PROJECT)/src/platform/$(PLATFORM)/parameters.h
SRCS += $(PROJECT)/src/platform/$(PLATFORM)/parameters.c

INCS += $(INCLUDE)/no_os_delay.h \
Copy link
Contributor

Choose a reason for hiding this comment

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

use NO_OS_INC_DIRS instead of INCS, search no-OS github to see examples of its usage

@@ -0,0 +1,16 @@
INCS += $(PLATFORM_DRIVERS)/maxim_gpio.h \
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, NO_OS_INC_DIRS

@MCabuena MCabuena force-pushed the ltc7841_my_changes_to_run_ci branch from 34f8560 to 3674149 Compare December 18, 2024 02:13
@MCabuena
Copy link
Author

V5:

  1. changed src.mk file to use NO_OS_INC_DIRS instead of INC +=
  2. removed ltc7841_reg_st inside ltc7841_init_param
  3. added a blank space before the final return as suggested
  4. added a space in between the {} of functions

@kister-jimenez
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* - Redistributions of source code must retain the above copyright
Copy link
Contributor

Choose a reason for hiding this comment

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

you still have some of these old license headers, please check them all

*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* - Redistributions of source code must retain the above copyright
Copy link
Contributor

Choose a reason for hiding this comment

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

this one

*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* - Redistributions of source code must retain the above copyright
Copy link
Contributor

Choose a reason for hiding this comment

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

this one, and potentially others

*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* - Redistributions of source code must retain the above copyright
Copy link
Contributor

Choose a reason for hiding this comment

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

this one

$(PLATFORM_DRIVERS)/../common/maxim_dma.h \
$(PLATFORM_DRIVERS)/maxim_i2c.h \
$(PLATFORM_DRIVERS)/maxim_uart.h \
$(PLATFORM_DRIVERS)/maxim_uart_stdio.h
Copy link
Contributor

Choose a reason for hiding this comment

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

you still don't use NO_OS_INC_DIRS here

.. code-block:: bash

# Select the example you want to enable by choosing y for enabling and n for disabling
BASIC_EXAMPLE = y
Copy link
Contributor

@buha buha Dec 18, 2024

Choose a reason for hiding this comment

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

i don't think editing Makefiles is good to set the example to build even though we have existing projects done this way... let's strive to make the user interaction easy so that they run one of these commands to choose the example to build:

make EXAMPLE=basic_example
make EXAMPLE=iio_example

Basically you could default to basic like this:

EXAMPLE ?= basic_example

And take action based on it:

ifeq ($(EXAMPLE), iio_example)
...
endif

drivers/power/ltc7841/iio_ltc7841.c Show resolved Hide resolved
Comment on lines 68 to 76
struct ltc7841_init_param ltc7841_ip = {
.comm_param = {
.device_id = LTC7841_I2C_DEVICE_ID,
.max_speed_hz = LTC7841_I2C_CLK_SPEED,
.slave_address = LTC7841_I2C_ADDR,
.platform_ops = &max_i2c_ops,
.extra = (void *)&ltc7841_i2c_extra,
},
.regs = ltc7841_regs
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll probably have to update this to match your current init_param.

Comment on lines 80 to 83
if (ret)
{
goto error;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need brackets for single-statement if/else

Comment on lines 136 to 134
struct ltc7841_iio_desc_init_param ltc7841_iio_ip =
{
.ltc7841_init_param = &ltc7841_ip,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting issues.

Comment on lines 145 to 148
if (ret)
{
goto exit;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting issue.

.debug_attributes = ltc7841_debug_attrs,
.debug_reg_read = ltc7841_iio_reg_read,
.debug_reg_write = ltc7841_iio_reg_write,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

And extra newline at EOF.

$(NO-OS)/util/no_os_mutex.c \
$(NO-OS)/util/no_os_crc8.c

SRCS += $(DRIVERS)/power/ltc7841/ltc7841.c
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add extra newline


struct ltc7841_init_param ltc7841_ip = {
.comm_param = &ltc7841_i2c_ip
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add extra newline

if (ret)
pr_err("Error!\n");
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.

Add extra newline

Comment on lines 90 to 113
enum ltc7841_registers {
LTC7841_OPERATION = 1,
LTC7841_VOUT_MODE = 0x20,
LTC7841_STATUS_WORD = 0x79,
LTC7841_READ_VIN = 0x88,
LTC7841_READ_IIN = 0x89,
LTC7841_READ_VOUT = 0x8B,
LTC7841_READ_IOUT = 0x8C,
LTC7841_READ_TEMPERATURE_1 = 0x8D,
LTC7841_PMBUS_REVISION = 0x98,
LTC7841_MFR_IOUT_PEAK = 0xD7,
LTC7841_MFR_VOUT_PEAK = 0xDD,
LTC7841_MFR_VIN_PEAK = 0xDE,
LTC7841_MFR_TEMEPRATURE1_PEAK = 0xDF,
LTC7841_MFR_IIN_PEAK = 0xE1,
LTC7841_MFR_CLEAR_PEAKS = 0xE3,
LTC7841_MFR_VOUT_MARGIN_HIGH = 0xE5,
LTC7841_MFR_SPECIAL_ID = 0xE7,
LTC7841_MFR_VOUT_COMMAND = 0xE8,
LTC7841_MFR_CONFIG = 0xE9,
LTC7841_MFR_VOUT_MARGIN_LOW = 0xED,
LTC7841_MFR_RAIL_ADDRESS = 0xFA,
LTC7841_MFR_RESET = 0xFD
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may want to follow this: https://github.com/analogdevicesinc/no-OS/blob/a75c351f56ab2699d7b0fe8623ffcaf4e867f6ec/drivers/power/lt7182s/lt7182s.h#L50C1-L170C33

For consistency across PMBUS devices and other no-OS drivers.

@MCabuena MCabuena closed this Dec 19, 2024
@MCabuena MCabuena force-pushed the ltc7841_my_changes_to_run_ci branch from 116c2e8 to cc73a3d Compare December 19, 2024 05:29
@MCabuena
Copy link
Author

I wanted to re-open this pull request

@MCabuena MCabuena reopened this Dec 19, 2024
@MCabuena
Copy link
Author

V6:

  1. changed enum registers for pmbus into # define
  2. changed, checked and re-checked files to use BSD 3 clause for license files
  3. adjusted driver README.rst to reflect the current ltc7841.h file
  4. adjusted project README.rst to instruct using new command for building the examples, adding a variable of EXAMPLE=IIO_EXAMPLE or EXAMPLE=BASIC_EXAMPLE
  5. changed examples_src.mk to use NO_OS_INC_DIRS
  6. changed examples_src.mk to check if EXAMPLE variable is equal to IIO_EXAMPLE or EXAMPLE is equal to BASIC_EXAMPLE
  7. added blank line in files as the last line
  8. added space before final return

@kister-jimenez
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Add initial header and source files for LTC7841 driver.

Signed-off-by: Marvin Neil Cabuenas <[email protected]>
Add initial header and source file for LTC7841 IIO driver.

Signed-off-by: Marvin Neil Cabuenas <[email protected]>
Add README.rst documentation file for LTC7841 alongside other
documentation related files.

Signed-off-by: Marvin Neil Cabuenas <[email protected]>
Add initial project files for both basic and IIO examples for LTC7841.

Signed-off-by: Marvin Neil Cabuenas <[email protected]>
Add README.rst documentation file for project alongside other
documentation related files.

Signed-off-by: Marvin Neil Cabuenas <[email protected]>
@MCabuena MCabuena force-pushed the ltc7841_my_changes_to_run_ci branch from 06fb555 to f3e43f0 Compare December 19, 2024 23:46
@MCabuena
Copy link
Author

V7:

  1. changed builds.json to add new EXAMPLE variable, EXAMPLE=BASIC_EXAMPLE for basic example and EXAMPLE=IIO_EXAMPLE for iio example

@kister-jimenez
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Collaborator

@kister-jimenez kister-jimenez left a comment

Choose a reason for hiding this comment

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

There are still previous comments that have not been addressed in this revision. Please address and resolve them all.

@@ -0,0 +1,24 @@
ifeq ($(EXAMPLE), IIO_EXAMPLE))
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove extra ')'

SRCS += $(PROJECT)/src/examples/iio_example/iio_example.c
endif

ifeq ($(EXAMPLE), BASIC_EXAMPLE))
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove extra ')'

Comment on lines +2 to +3
BASIC_EXAMPLE = n
IIO_EXAMPLE = y
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to remove this and replace with maybe a default like this:
EXAMPLE ?= IIO_EXAMPLE

#endif

#if (BASIC_EXAMPLE + IIO_EXAMPLE == 0)
#error At least one example has to be selected using y value in Makefile.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to update error messages because EXAMPLE is now passed as an argument to make instead of editing the makefile.
e.g.

Select an example by passing the type of EXAMPLE in the make command. For example:
make EXAMPLE=IIO_EXAMPLE

* @return ret - Result of the reading procedure.
* In case of success, the size of the read data is returned.
*/
//static
Copy link
Collaborator

Choose a reason for hiding this comment

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

//static is still present

{
struct ltc7841_desc *dev;
int ret;
/* ltc7841 only needs sda, scl and ground connection to the microcontroller*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this comment is necessary. You can remove this.

/* I2C write target address */
read_length = size;
write_buffer = cmd;
/* write part of write_read, 0 at the end for no stop bit */
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this comment because these can be inferred from line.

ret = no_os_i2c_write(desc->comm_desc, &write_buffer, 1, 0);
if (ret)
return ret;
/* read part of write_read, 1 at the end for stop bit sending */
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this comment as well.

// apply a 7 bit mask since bit 7 needs to be held low to enable communication
addr &= LTC7841_7_BIT_ADDRESS_MASK;

/* Write register */
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can remove this comment.

*/
int ltc7841_change_rail_address(struct ltc7841_desc *desc, uint8_t addr)
{
// apply a 7 bit mask since bit 7 needs to be held low to enable communication
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can remove this comment.

@@ -0,0 +1,9 @@
# Select the example you want to enable by choosing y for enabling and n for disabling
BASIC_EXAMPLE = n
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see this pull request and use it as a template: #2368

The idea is that the user types EXAMPLE=basic or EXAMPLE=iio_example, these being the names of the folders where you put the example code. We will create some infrastructure to do this automatically, but for now you'll have to do it manually.

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.

4 participants