On 20/03/16 11:32, Slawomir Stepien wrote:
> On Mar 20, 2016 10:25, Jonathan Cameron wrote:
>>>> +struct mcp4131_data {
>>>> + struct spi_device *spi;
>>
>> This is only used to lookup elements of your cfg array, I'd just have
>> a pointer to the relevant element of that array in here instead.
>>
>> struct mcp4131_cfg *cfg;
>>
>> and in probe do
>> data->cfg = &mcp4131_cfg[id];
>
> Great idea. I'll use it in v3.
>
>>>> + unsigned long devid;
>>>> + struct mutex lock;
>>>> + u8 tx[2], rx[2];
>>>
>>> alignment requirements for SPI transfer?
>> By which he means put them at the end of this structure and
>> mark the with __cacheline_aligned. It's not technically about alignment
>> but rather about ensuring nothing else is in the cacheline which will on some
>> spi devices be scrubbed when a transaction occurs.
>
> Thank you for this explanation. I'll move it at the and mark it with the
> attribute.
>
>>>> + data->rx[0] = 0;
>>>> + data->rx[1] = 0;
>>>
>>> initialization needed?
>>>
>>> setup of data->xfer + data->tx is done outside the lock, this seems wrong
>> agreed.
>
> I'll lock the mutex just before switching the mask in _read_raw and in
> write_raw like this:
>
> mutex_lock(&data->lock);
>
> switch(mask) {
> case IIO_CHAN_INFO_RAW:
> (...)
> }
>
> mutex_unlock(&data->lock);
>
>> Now I'd change the way you are doing this slightly so that you have
>> data->cfg pointing to mcp4131[data->devid]. Moves the 'what part am I?'
>> question to a single place in the probe function giving slightly cleaner code.
>>>> + *val = 1000 * mcp4131_cfg[data->devid].kohms;
>>>> + *val2 = mcp4131_cfg[data->devid].max_pos;
>>>> + return IIO_VAL_FRACTIONAL;
>
> Something like this:
>
> *val = 1000 * data->cfg->kohms;
> *val2 = data->cfg->max_pos;
> mutex_unlock(&data->lock);
> return IIO_VAL_FRACTIONAL;
> ?
Exactly.
>
>>>> + dev_info(&spi->dev, "Registered %s\n", indio_dev->name);
>>>
>>> I'd rather drop this message
>> Agreed, adds noise and it's easy to check if the register succeeded anyway
>> by just looking to see if the device is there in sysfs.
>
> OK
>
>>>> +static int mcp4131_remove(struct spi_device *spi)
>>>> +{
>>>> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>>> + struct mcp4131_data *data = iio_priv(indio_dev);
>>>> +
>>>> + mutex_destroy(&data->lock);
>>>
>>> no need to call
>> Hmm. This is an oddity, the mutex_destroy exists to aid in debugging locking
>> issues by explicity marking the mutex as do not use - iff the mutex
>> debugging is enabled. In this case the storage is promptly deleted anyway
>> so any attempt to use the mutex would result in a null pointer dereference
>> anyway. Hence probably not worth having it here.
>
> OK.
>
> Thank your for all the explanations. This helps a lot.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
