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

[Ardelean, Alexandru]

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@xxxxxxxxxxxxxxx
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);
> > +}