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.

Sign up for GitHub

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

Jump to bottom

drivers: iio: adc : Add ad4695 support #2433

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

drivers: iio: adc : Add ad4695 support #2433

wants to merge 7 commits into from

Conversation

rbolboac
Copy link
Contributor

No description provided.

@rbolboac rbolboac self-assigned this Feb 20, 2024
@nunojsa
Copy link
Collaborator

nunojsa commented Feb 20, 2024

I'll try to do a proper review in the next few days... One thing that I'll already ask is to rename your driver name.

Typically we don't name it with "wildcards" like 'x'. Just pick one of the parts the drivers supports (maybe the one you have) and name the driver against it.

@rbolboac rbolboac force-pushed the dev/ad469x branch 2 times, most recently from 2f5312b to 10555f2 Compare February 20, 2024 12:03
@nunojsa nunojsa requested a review from a team March 13, 2024 08:01
nunojsa
nunojsa reviewed Apr 30, 2024
View reviewed changes
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.

Finally remembered about this one 😅

Documentation/devicetree/bindings/iio/adc/adi,ad4696.yaml Outdated
description: The regulator supply for ADC reference voltage.

"#io-channel-cells":
const: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the above?

Documentation/devicetree/bindings/iio/adc/adi,ad4696.yaml Outdated

clock-names:
items:
- const: ref_clk
Copy link
Collaborator

Choose a reason for hiding this comment

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

just one clock... drop this one

Documentation/devicetree/bindings/iio/adc/adi,ad4696.yaml Outdated

pwm-names:
items:
- const: cnv
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that for dma we need the -names property. Is that also true for pwm? Being just one, I would expect not to need it.

Documentation/devicetree/bindings/iio/adc/adi,ad4696.yaml Outdated
Select the pin-oairing option on the specific channel. Valid values
are:
0: Channel paired with REFGND.
1: Channel paired with COM
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference between refgnd and com from a software point of view? Could both be seen as single-ended channels?

Documentation/devicetree/bindings/iio/adc/adi,ad4696.yaml Outdated
are:
0: Channel paired with REFGND.
1: Channel paired with COM
2: Even and odd channels pairing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The above needs more explanations? What's the meaning of odd and even in this case? (yes, not reading the datasheet at this point :))

drivers/iio/adc/ad4696.c Outdated
reg_data &= ~mask;
reg_data |= data;

return ad4696_spi_reg_write(st, reg_addr, reg_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

not safe... needs a lock

drivers/iio/adc/ad4696.c Outdated
static int ad4696_enable_cnv(struct ad4696_state *st)
{
return ad4696_set_sampling_freq(st, st->sampling_freq);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

drop the function

drivers/iio/adc/ad4696.c Outdated
unsigned int vref_mv;
int sampling_freq;
u8 num_channels;
u8 data[3] ____cacheline_aligned;
Copy link
Collaborator

Choose a reason for hiding this comment

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

just use one space... also use the IIO define for DMA safe buffere... This one is not safe for all platforms and hence not the one to use

drivers/iio/adc/ad4696.c Outdated
const char *name;
enum ad4696_ids dev_id;
int max_sample_rate;
u8 max_num_channels;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: single space

drivers/iio/adc/ad4696.c Outdated
if (ret)
return ret;

*reg_data = st->data[2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, the question is... Can't we use regmap?

@RaduSabau1
Copy link
Collaborator

Hi @nunojsa, I made the changes requested by you although there is a chance I missed some. Anyway I will mark this PR as a draft since I tested it with the ZedBoard and appears like the buffer isn't working right, I managed to fix a part of it but still the behavior isn't quit the expected one.

@RaduSabau1 RaduSabau1 self-assigned this Sep 6, 2024
@RaduSabau1 RaduSabau1 changed the title AD469X driver drivers: iio: adc : Add ad4696 support Sep 6, 2024
@RaduSabau1 RaduSabau1 marked this pull request as draft September 6, 2024 11:02
@dlech
Copy link
Collaborator

dlech commented Sep 6, 2024

FYI, we recently upstreamed the driver for this family of chips. https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ad4695.c?h=testing

And @threexc is working on adding more features upstream.

@RaduSabau1 RaduSabau1 changed the title drivers: iio: adc : Add ad4696 support drivers: iio: adc : Add ad4695 support Sep 10, 2024
@RaduSabau1 RaduSabau1 marked this pull request as ready for review September 10, 2024 10:23
@RaduSabau1
Copy link
Collaborator

v2 :

  • Added driver dependencies from upstream
  • Added driver support patches from upstream
  • Added devicetree file for the zed project

@nunojsa
Copy link
Collaborator

nunojsa commented Sep 10, 2024

@RaduSabau1 please make sure to take care about CI complains...

@RaduSabau1 RaduSabau1 force-pushed the dev/ad469x branch 3 times, most recently from d5fa7db to ce18e56 Compare September 11, 2024 09:21
@RaduSabau1
include: linux : Add AD4695 driver dependencies
84c4d09 
Add driver dependencies from upstream for upcoming AD4695 driver
regarding the include files.

Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
drivers: regulator : Add AD4695 driver dependencies
3ae1ed4 
Add upstream changes dependencies for upcoming AD4695 driver regarding
the source files.

Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
dt-bindings: iio: adc: add AD4695 and similar ADCs
c4f7fed 
Add device tree bindings for AD4695 and similar ADCs.

Link: https://patchwork.kernel.org/project/linux-iio/patch/[email protected]/
Reviewed-by: Conor Dooley <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
iio: adc: ad4695: Add driver for AD4695 and similar ADCs
9442954 
This is a new driver for Analog Devices Inc. AD4695 and similar ADCs.
The initial driver supports initializing the chip including configuring
all possible LDO and reference voltage sources as well as any possible
voltage input channel wiring configuration.

Only the 4-wire SPI wiring mode where the CNV pin is tied to the CS pin
is supported at this time. And reading sample data from the ADC can only
be done in direct mode for now.

Co-developed-by: Ramona Gradinariu <[email protected]>
Signed-off-by: Ramona Gradinariu <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
Documentation: iio: Document ad4695 driver
8f48821 
The Analog Devices Inc. AD4695 (and similar chips) are complex ADCs that
will benefit from a detailed driver documentation.

This documents the current features supported by the driver.

Link: https://patchwork.kernel.org/project/linux-iio/patch/[email protected]/
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
iio: adc: Kconfig.adi : imply AD4695
0bfb050 
Imply Ad4695 in Kconfig.adi file in the IIO subsystem.

Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
arch: arm: boot: dts : Add AD4696 devicetree
7c8b82f 
Add devicetree for ad469x_fmcz/zed project

Signed-off-by: RaduSabau1 <[email protected]>
@RaduSabau1
Copy link
Collaborator

v3:

  • Solved DCO CI failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nunojsa nunojsa nunojsa left review comments

@mhennerich mhennerich Awaiting requested review from mhennerich mhennerich is a code owner

@dbogdan dbogdan Awaiting requested review from dbogdan dbogdan is a code owner

@btogorean btogorean Awaiting requested review from btogorean btogorean is a code owner

@chegbeli chegbeli Awaiting requested review from chegbeli chegbeli is a code owner

@buha buha Awaiting requested review from buha buha is a code owner

@amiclaus amiclaus Awaiting requested review from amiclaus amiclaus is a code owner

@danmois danmois Awaiting requested review from danmois danmois is a code owner

@CiprianRegus CiprianRegus Awaiting requested review from CiprianRegus CiprianRegus is a code owner

@machschmitt machschmitt Awaiting requested review from machschmitt machschmitt is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@rbolboac rbolboac

@RaduSabau1 RaduSabau1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rbolboac @nunojsa @RaduSabau1 @dlech