| Message ID | 20230804100248.3773861-1-marcus.folkesson@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: iio: adc: mcp3911: add support for the whole MCP39xx family | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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.
Hi Andy, Thanks for your revew! [...] > > > +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. Yes it does. [...] > > > +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. Yes, checkpatch does not report any warning for this. [...] > > + adc->chip = (struct mcp3911_chip_info *)spi_get_device_id(spi)->driver_data; > > Can't you use spi_get_device_match_data()? > > ... Will go for spi_get_device_match_data(). [...] > > > + 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. Indeed, but I think I will short it down to just spi->dev. > > > Best regards, Marcus Folkesson
diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml index f7b3fde4115a..06951ec5f5da 100644 --- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml @@ -18,7 +18,13 @@ description: | properties: compatible: enum: + - microchip,mcp3910 - microchip,mcp3911 + - microchip,mcp3912 + - microchip,mcp3913 + - microchip,mcp3914 + - microchip,mcp3918 + - microchip,mcp3919 reg: maxItems: 1