Messages in this thread | | | Date | Fri, 4 Aug 2023 22:37:27 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2 2/2] iio: adc: mcp3911: add support for the whole MCP39xx family |
| |
On Fri, Aug 04, 2023 at 12:02:48PM +0200, Marcus Folkesson wrote: > Microchip does have many similar chips, add support for those. > > The new supported chips are: > - microchip,mcp3910 > - microchip,mcp3912 > - microchip,mcp3913 > - microchip,mcp3914 > - microchip,mcp3918 > - microchip,mcp3919
...
> help > - Say yes here to build support for Microchip Technology's MCP3911 > - analog to digital converter. > + Say yes here to build support for Microchip Technology's MCP3910,
> + MCP3911, MCP3912, MCP3913, MCP3914, MCP3918 and MCP3919
This line is misindented. Should be <TAB><space><space>.
> + analog to digital converters.
...
> +#define MCP3910_REG_OFFCAL_CH0 0x0f > +#define MCP3910_OFFCAL(x) (MCP3910_REG_OFFCAL_CH0 + x * 6) > + > +
Single blank line is enough.
...
> +static int mcp3910_get_offset(struct mcp3911 *adc, int channel, int *val) > +{ > + return mcp3911_read(adc, MCP3910_OFFCAL(channel), val, 3);
Just to be sure, the proper endianess conversion is done in mcp3911_read() and mcp3911_write() calls?
This question applies to all calls to that APIs.
> +}
...
> + int ret = mcp3911_write(adc, MCP3910_OFFCAL(channel), val, > + 3);
This looks weird not being on a single line. Moreover it fits even 80 characters. Same applies to other similar cases.
Also, please use better approach, i.e.
int ret;
ret = ...(...); if (ret) ...
Also applies to several places.
> + if (ret) > + return ret;
...
> + /* Enable offset*/
Missing space.
...
> +static int mcp3910_get_osr(struct mcp3911 *adc, int *val) > +{ > + int ret = mcp3911_read(adc, MCP3910_REG_CONFIG0, val, 3);
Have you run checkpatch? Here should be a blank line. Same in other several places.
> + *val = FIELD_GET(MCP3910_CONFIG0_OSR, *val); > + *val = 32 << *val;
Please, use a temporary variable and assign the result only once. It will be a better code.
> + return ret; > +}
...
> +static int mcp3911_set_osr(struct mcp3911 *adc, int val) > +{ > + val = FIELD_PREP(MCP3911_CONFIG_OSR, val);
As per above.
> + return mcp3911_update(adc, MCP3911_REG_CONFIG, > + MCP3911_CONFIG_OSR, val, 2);
> +
Redundant blank line.
> +}
...
> +static int mcp3911_get_osr(struct mcp3911 *adc, int *val) > +{
As per above.
> +}
...
> + /* Set gain to 1 for all channels */
Again, wrong indentation. Can you check all your lines for the proper indentation. (Here should be just <TAB>.)
...
> + for (int i = 0; i < adc->chip->num_channels - 1; i++) { > + adc->gain[i] = 1; > + regval &= ~MCP3911_GAIN_MASK(i); > + }
Missing blank line.
> + return mcp3911_write(adc, MCP3911_REG_GAIN, regval, 1); > + > +
Too many redundant blank lines.
> +}
...
> + /* Set gain to 1 for all channels */
Mind indentation.
...
> + /* Disable offset to ignore any old values in offset register*/
Missing space.
...
> + adc->chip = (struct mcp3911_chip_info *)spi_get_device_id(spi)->driver_data;
Can't you use spi_get_device_match_data()?
...
> + /* > + * Fallback to "device-addr" due to historical mismatch between > + * dt-bindings and implementation
Missing grammatical period at the end.
> + */
...
> + device_property_read_u32(&adc->spi->dev, "device-addr", &adc->dev_addr);
With
struct device *dev = &adc->spi->dev;
lines like this will be neater.
-- With Best Regards, Andy Shevchenko
| |