On 2018-11-06 17:37, Peter Rosin wrote:
> Hi!
>
> Some comments inline...
>
>> +
>> +static int mcp41010_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2, long mask)
>> +{
>> + int err;
>> + struct mcp41010_data *data = iio_priv(indio_dev);
>> + int channel = chan->channel;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + if (val > MCP41010_WIPER_MAX || val < 0)
>> + return -EINVAL;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + mutex_lock(&data->lock);
>> +
>> + data->buf[0] = MCP41010_WIPER_ENABLE << channel;
>> + data->buf[0] |= MCP41010_WRITE;
>
> Will this not clobber the other channel for mcp42xxx chips???
I had a peak in the datasheet, and no, it will not. It was just the naming
with ..._WIPER_ENABLE that threw me off. It assumed, from the naming, that
each channel has a separate enable bit in the command byte and that you
were required to keep the state of those intact for each and every command
you sent. After looking at the datasheet, that is obviously not the case,
and the code is fine. But may I ask for a change in naming here?
MCP41010_WIPER_CHANNEL instead of MCP41010_WIPER_ENABLE perhaps?
Cheers,
Peter
>
>> + data->buf[1] = val & 0xff;
>> +
>> + err = spi_write(data->spi, data->buf, 2);
>> + if (!err)
>> + data->value[channel] = val;
>> +
>> + mutex_unlock(&data->lock);
>> +
>> + return err;
>> +}
