Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/3] iio: amplifiers: hmc425a: Add support for HMC425A
 step attenuator with gpio interface
To:     Beniamin Bia <beniamin.bia@analog.com>, jic23@kernel.org
Cc:     Michael.Hennerich@analog.com, pmeerw@pmeerw.net,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        biabeniamin@outlook.com, knaack.h@gmx.de, robh+dt@kernel.org,
        mark.rutland@arm.com, devicetree@vger.kernel.org,
        Alexandru Ardelean <alexandru.ardelean@analog.com>
References: <20200113141555.16117-1-beniamin.bia@analog.com>
From:   Lars-Peter Clausen <lars@metafoo.de>
Message-ID: <5ae63616-5749-da51-b0b2-85cdcaa948f3@metafoo.de>
Date:   Mon, 13 Jan 2020 21:57:32 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20200113141555.16117-1-beniamin.bia@analog.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 1/13/20 3:15 PM, Beniamin Bia wrote:
[...]
> +static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
> +{
> +	struct hmc425a_state *st = iio_priv(indio_dev);
> +	int i, *values;
> +
> +	values = kmalloc_array(st->chip_info->num_gpios, sizeof(int),
> +			       GFP_KERNEL);
> +	if (!values)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < st->chip_info->num_gpios; i++)
> +		values[i] = (value >> i) & 1;
> +
> +	gpiod_set_array_value_cansleep(st->gpios->ndescs, st->gpios->desc,
> +				       values);

This API got changed a while ago in upstream, see
https://github.com/analogdevicesinc/linux/commit/b9762bebc6332b40c33e03dea03e30fa12d9e3ed

> +	kfree(values);
> +	return 0;
> +}
[...]
> +static int hmc425a_probe(struct platform_device *pdev)
> +{
[...]
> +
> +	platform_set_drvdata(pdev, indio_dev);

drvdata is never accessed, no need to set it.

> +	mutex_init(&st->lock);
> +
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = np->name;

I know ADI likes to do this in its non upstream drivers, but the above
is not IIO ABI compliant. The name is supposed to identify the type of
the device, which means for this driver should be static "hmc425a".
Maybe consider adding a field to the hmc425a_chip_info for this.

> +	indio_dev->info = &hmc425a_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&pdev->dev, indio_dev);
> +}