lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZM1Td+v3tcoaID9v@smile.fi.intel.com>
Date:   Fri, 4 Aug 2023 22:37:27 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Marcus Folkesson <marcus.folkesson@...il.com>
Cc:     Kent Gustavsson <kent@...oris.se>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        ChiYuan Huang <cy_huang@...htek.com>,
        Haibo Chen <haibo.chen@....com>,
        Ramona Bolboaca <ramona.bolboaca@...log.com>,
        Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        William Breathitt Gray <william.gray@...aro.org>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ