Received-SPF: pass (google.com: domain of linux-kernel+bounces-178701-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 14 May 2024 16:09:32 +0300
From: Andy Shevchenko <andy@kernel.org>
To: Alisa-Dariana Roman <alisadariana@gmail.com>
Cc: michael.hennerich@analog.com, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	lars@metafoo.de, jic23@kernel.org, robh@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	lgirdwood@gmail.com, broonie@kernel.org, nuno.sa@analog.com,
	marcelo.schmitt@analog.com, bigunclemax@gmail.com,
	dlechner@baylibre.com, okan.sahin@analog.com, fr0st61te@gmail.com,
	alisa.roman@analog.com, marcus.folkesson@gmail.com,
	schnelle@linux.ibm.com, liambeguin@gmail.com
Subject: Re: [PATCH v8 6/6] iio: adc: ad7192: Add AD7194 support
Message-ID: <ZkNijKz0N7PPvmeU@smile.fi.intel.com>
References: <20240514120222.56488-1-alisa.roman@analog.com>
 <20240514120222.56488-7-alisa.roman@analog.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240514120222.56488-7-alisa.roman@analog.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
> 
> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> only chip that has no channel sequencer, num_slots should remain
> undefined.
> 
> Also modify config AD7192 description for better scaling.

Some non-critical, mostly style related comments below.

..

This...

> +#define AD7194_CH(p)		(BIT(10) | AD7194_CH_POS(p))
> +				  /* 10th bit corresponds to CON18(Pseudo) */

..should be (you have broken indentation on the comment, btw):

/* 10th bit corresponds to CON18(Pseudo) */
#define AD7194_CH(p)		(BIT(10) | AD7194_CH_POS(p))

But no need to resend because of this, let's wait others to comment, and
if everything fine I think Jonathan can massage this when applying.

..

> +#define AD7194_CH_TEMP		0x100 /* Temp sensor */

Not sure that the comment has any value here.

..

> +static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
> +{
> +	if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid AIN channel: %u\n", ain);
> +
> +	return 0;

While this uses traditional pattern, it might be better looking in a form of

	if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
		return 0;

	return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);

But at the same time I would rather expect this to be in the caller and here
to have a boolean function

static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
{
	return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
}

> +}

..

> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many channels: %u\n", num_channels);

		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);

?

Or with limit

		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
				     num_channels);


..

> +	ad7194_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad7194_channels), GFP_KERNEL);

	ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);

?

Or

	ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
				       GFP_KERNEL);

?

..

> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     ain, ARRAY_SIZE(ain));
> +		if (ret == 0) {

And here I would rather go for the traditional pattern, i.e.

		if (ret) {
			...
		} else {
			...
		}

> +			ret = ad7194_validate_ain_channel(dev, ain[0]);
> +			if (ret)
> +				return ret;
> +
> +			ret = ad7194_validate_ain_channel(dev, ain[1]);
> +			if (ret)
> +				return ret;
> +
> +			*ad7194_channels = ad7194_chan_diff;
> +			ad7194_channels->scan_index = index++;
> +			ad7194_channels->channel = ain[0];
> +			ad7194_channels->channel2 = ain[1];
> +			ad7194_channels->address = AD7194_DIFF_CH(ain[0], ain[1]);
> +		} else {
> +			ret = fwnode_property_read_u32(child, "single-channel",
> +						       &ain[0]);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Missing channel property\n");
> +
> +			ret = ad7194_validate_ain_channel(dev, ain[0]);
> +			if (ret)
> +				return ret;
> +
> +			*ad7194_channels = ad7194_chan;
> +			ad7194_channels->scan_index = index++;
> +			ad7194_channels->channel = ain[0];
> +			ad7194_channels->address = AD7194_CH(ain[0]);
> +		}
> +		ad7194_channels++;
> +	}

-- 
With Best Regards,
Andy Shevchenko