| Message ID | 20230807071831.4152183-1-marcus.folkesson@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3,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 Mon, Aug 07, 2023 at 09:18:31AM +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 ... > +#define MCP3910_STATUSCOM_DRHIZ BIT(20) Is it deliberately using spaces? If so, why? ... > +static int mcp3910_get_osr(struct mcp3911 *adc, int *val) > +{ > + int ret, osr; > + > + ret = mcp3911_read(adc, MCP3910_REG_CONFIG0, val, 3); > + osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val); > + *val = 32 << osr; > + return ret; I believe this is wrong order. Or bad code. The rule of thumb is not pollute the output variable if we know the error happened. Same applies to another function. > +} ... > - ret = mcp3911_config(adc); > + ret = device_property_read_u32(&adc->spi->dev, "microchip,device-addr", &adc->dev_addr); Why not spi->dev? Ditto for other uses like this.
On Mon, Aug 07, 2023 at 06:53:21PM +0300, Andy Shevchenko wrote: > On Mon, Aug 07, 2023 at 09:18:31AM +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 > > ... > > > +#define MCP3910_STATUSCOM_DRHIZ BIT(20) > > Is it deliberately using spaces? If so, why? No, probably due to my with my new vim setup.. > > ... > > > +static int mcp3910_get_osr(struct mcp3911 *adc, int *val) > > +{ > > + int ret, osr; > > + > > + ret = mcp3911_read(adc, MCP3910_REG_CONFIG0, val, 3); > > > + osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val); > > + *val = 32 << osr; > > + return ret; > > I believe this is wrong order. Or bad code. The rule of thumb is not pollute > the output variable if we know the error happened. > > Same applies to another function. > > > +} > > ... > > > - ret = mcp3911_config(adc); > > + ret = device_property_read_u32(&adc->spi->dev, "microchip,device-addr", &adc->dev_addr); > > Why not spi->dev? Ditto for other uses like this. After all, I think it is better to stick sith adc->spi-dev to be consistent with the rest of the probe function. Change to spi->dev should probably be a seperate patch. Do you agree? > > -- > With Best Regards, > Andy Shevchenko > >
On 08/08/2023 09:22, Marcus Folkesson wrote: >> >> ... >> >>> - ret = mcp3911_config(adc); >>> + ret = device_property_read_u32(&adc->spi->dev, "microchip,device-addr", &adc->dev_addr); >> >> Why not spi->dev? Ditto for other uses like this. > > After all, I think it is better to stick sith adc->spi-dev to be > consistent with the rest of the probe function. Change to spi->dev > should probably be a seperate patch. > Do you agree? > You can first simplify the usages and then add new family support already using spi->dev. Best regards, Krzysztof
On Tue, Aug 08, 2023 at 09:22:24AM +0200, Marcus Folkesson wrote: > On Mon, Aug 07, 2023 at 06:53:21PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 07, 2023 at 09:18:31AM +0200, Marcus Folkesson wrote: ... > > > - ret = mcp3911_config(adc); > > > + ret = device_property_read_u32(&adc->spi->dev, "microchip,device-addr", &adc->dev_addr); > > > > Why not spi->dev? Ditto for other uses like this. > > After all, I think it is better to stick sith adc->spi-dev to be > consistent with the rest of the probe function. Change to spi->dev > should probably be a seperate patch. > Do you agree? Make sure you first switch to use shorter form and then add this one. In that case I agree.
On Tue, Aug 08, 2023 at 09:49:43AM +0200, Krzysztof Kozlowski wrote: > On 08/08/2023 09:22, Marcus Folkesson wrote: ... > >>> + ret = device_property_read_u32(&adc->spi->dev, "microchip,device-addr", &adc->dev_addr); > >> > >> Why not spi->dev? Ditto for other uses like this. > > > > After all, I think it is better to stick sith adc->spi-dev to be > > consistent with the rest of the probe function. Change to spi->dev > > should probably be a seperate patch. > > Do you agree? > > > > You can first simplify the usages and then add new family support > already using spi->dev. He-he, answered the same :-)
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