linux-kernel - Re: [PATCH 1/3] iio: amplifiers: hmc425a: Add support for HMC425A step attenuator with gpio interface

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: <5925b4f1d47306ec4376a296a1146ff024239044.camel@analog.com>
Date:   Tue, 14 Jan 2020 07:27:08 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "Bia, Beniamin" <Beniamin.Bia@...log.com>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
CC:     "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "knaack.h@....de" <knaack.h@....de>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "biabeniamin@...look.com" <biabeniamin@...look.com>
Subject: Re: [PATCH 1/3] iio: amplifiers: hmc425a: Add support for HMC425A
 step attenuator with gpio interface

On Mon, 2020-01-13 at 21:57 +0100, Lars-Peter Clausen wrote:
> [External]
> 
> 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.

We've actually [recently] had a discussion about this internally regarding
the 'indio_dev->name'.

Maybe it's a good time to ask here (now).
A lot of our userspace stuff have been searching IIO devices via the 'name'
field in sysfs, which is the name assigned here.
That creates a problem when you have multiple devices with the same driver.
Which is why, one 

So, then some questions would be:
Is a searching for IIO devices [in userspace] based on IIO device-name not
recommended? If not, what would be? Or what would be a better idea?

The ABI reads [hopefully I pulled up the right field]:
What:           /sys/bus/iio/devices/iio:deviceX/name
KernelVersion:  2.6.35
Contact:        linux-iio@...r.kernel.org
Description:
                Description of the physical chip / device for device X.
                Typically a part number.

The text in description is a bit open to interpretation, so I can't make an
assessment of what is correct.
In case there was a discussion about this, sorry for repeating some things
now.


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