Date: Wed, 16 Mar 2016 17:25:45 +0100
From: Slawomir Stepien <sst@poczta.fm>
To: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
Message-ID: <20160316162544.GA6212@x220>
References: <20160316113738.GB2445@x220>
 <alpine.DEB.2.02.1603161238590.30358@pmeerw.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.02.1603161238590.30358@pmeerw.net>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3023
Lines: 103

On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote:
> On Wed, 16 Mar 2016, Slawomir Stepien wrote:
> 
> > The following functionalities are supported:
> >  - write, read from volatile and non volatile memory
> >  - increase and decrease commands
> >  - read from status register
> >  - write and read to tcon register
> > 
> > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf

Thank you for all your comments.

> the driver naming is a mess: the subject says MCP414X, the file name is 
> mcp41xx, the DT compatible string says mcp4113x -- this does not match

OK. I will change that to mcp414x in version 2.

> plenty of the private API, some of which seems to be debug only?
> what is really needed to interact with a poti?

I wanted to export both the non volatile and volatile memory addresses for wiper
position access. That is bare minimum for the poti to operate.

But I also wanted to export additional features of this chip. That is way there
is increase and decrease API, and STATUS and TCON register access.

The memory_map API is a way to access all the not used by chip memory addresses.
This API I think could be deleted. But I still think that some people might find
it useful.

> comments below
>  
> > +File:	/sys/bus/iio/devices/../out_resistanceN_raw
> 
> this is already described in sysfs-bus-iio

Should I leave it and add reference to sysfs-bus-iio or delete it completely?

> > +struct mcp41xx_cfg {
> > +	unsigned long int devid;
> 
> devid is not set/used

That's true. Will fix it in v2.

> > +static int mcp41xx_exec(struct mcp41xx_data *data,
> > +		u8 addr, u8 cmd,
> > +		int *trans, size_t n)
> 
> should trans really be int, not u8?

There is a need for 9 bit value write/read. I used int just for my convenience.
However there is one piece of code missing now I see. I should check the value
of tans[0] to see if it's > 256 or lower then 0. Will add it in v2.

Using u8 will force additional bit operations. I could try using u16 to still
have the option of parsing user input as 9 bit value (eg. 256 position).

> > +{
> > +	int err;
> > +	struct spi_device *spi = data->spi;
> > +
> > +	/* Two bytes are needed for the response */
> > +	if (n != 2)
> > +		return -EINVAL;
> 
> why pass n if it is always 2?

Will fix in v2.

> > +	err = devm_iio_device_register(&spi->dev, indio_dev);
> > +	if (err) {
> > +		dev_info(&spi->dev, "Unable to register %s", indio_dev->name);
> 
> \n missing
> 
> > +		return err;
> > +	}
> > +
> > +	dev_info(&spi->dev, "Registered %s", indio_dev->name);
> 
> \n
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int mcp41xx_remove(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > +	struct mcp41xx_data *data = iio_priv(indio_dev);
> > +
> > +	mutex_destroy(&data->lock);
> > +	devm_iio_device_unregister(&spi->dev, indio_dev);
> > +	kfree(mcp41xx_attributes);
> > +
> > +	dev_info(&spi->dev, "Unregistered %s", indio_dev->name);

Also \n is missing here. Will fix in v2.

-- 
Slawomir Stepien