lkml.org 
[lkml]   [2023]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 4/4] iio: dac: mcp4922: add support for mcp48xx series chips
From
Hi,

Looks very good. A few small comments inline.
[...]
> @@ -50,6 +58,35 @@ struct mcp4922_state {
> }, \
> }
>
> +static bool mcp4922_needs_vref(int device_id)

`enum mcp4922_supported_device_ids` instead of `int`. Same for num_channels() below.

> +{
> + switch (device_id) {
> + case ID_MCP4902:
> + case ID_MCP4912:
> + case ID_MCP4921:
> + case ID_MCP4922:
> + return true;
> + default:
> + return false;
> + }
> +}
> [...]
> static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
> {
> state->mosi[1] = val & 0xff;
> @@ -108,11 +145,17 @@ static int mcp4922_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> -static const struct iio_chan_spec mcp4922_channels[4][MCP4922_NUM_CHANNELS] = {
> +static const struct iio_chan_spec mcp4922_channels[10][MCP4922_NUM_CHANNELS] = {

mcp4922_channels[][MCP4922_NUM_CHANNELS]

So it does not have to be changed again when adding additional devices
in the future.

> [...]
> @@ -197,11 +244,14 @@ static void mcp4922_remove(struct spi_device *spi)
> {
> struct iio_dev *indio_dev = spi_get_drvdata(spi);
> struct mcp4922_state *state;
> + const struct spi_device_id *id = spi_get_device_id(spi);
>
> iio_device_unregister(indio_dev);
> state = iio_priv(indio_dev);
> regulator_disable(state->vdd_reg);
> - regulator_disable(state->vref_reg);
> + if (mcp4922_needs_vref(id->driver_data)) {
Could be `if (state->vref_reg)`, this way you don't need to lookup the
spi_device_id. But either way is fine.
> + regulator_disable(state->vref_reg);
> + }
> }
>
> [...]


\
 
 \ /
  Last update: 2023-04-05 16:27    [W:0.139 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site