Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter

From: Octavian Purdila
Date: Tue Oct 07 2014 - 13:52:41 EST


On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:
>> From: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxx>
>>
>> This patch adds support for the Diolan DLN-2 I2C master module. Due
>> to hardware limitations it does not support SMBUS quick commands.
>>
>> Information about the USB protocol interface can be found in the
>> Programmer's Reference Manual [1], see section 6.2.2 for the I2C
>> master module commands and responses.
>>
>> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>>
>> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxx>
>> Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
>> ---
>
> [...]
>
>> +#define DLN2_I2C_MAX_XFER_SIZE 256
>> +#define DLN2_I2C_BUF_SIZE (DLN2_I2C_MAX_XFER_SIZE + 16)
>> +
>> +struct dln2_i2c {
>> + struct platform_device *pdev;
>> + struct i2c_adapter adapter;
>> + u32 freq;
>> + u32 min_freq;
>> + u32 max_freq;
>> + u8 port;
>> + /*
>> + * Buffer to hold the packet for read or write transfers. One
>> + * is enough since we can't have multiple transfers in
>> + * parallel on the i2c adapter.
>> + */
>> + void *buf;
>> +};
>> +
>> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
>> +{
>> + int ret;
>> + u16 cmd;
>> +
>> + if (enable)
>> + cmd = DLN2_I2C_ENABLE;
>> + else
>> + cmd = DLN2_I2C_DISABLE;
>> +
>> + ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
>> + NULL, NULL);
>
> Don't use the port member of dln2 directly here. Encode the protocol in
> the function as you already do for most other messages (and did in
> previous versions). You could even declare a
>
> struct {
> u8 port;
> } tx;
>
> for consistency.
>

Yes, I think I did this after Arnd's review on the gpio stuff where I
thought he suggested to remove the structure, when in fact he asked to
remove the __packed attribute. I will revert it back.

>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)
>> +{
>> + int ret;
>> + struct tx_data {
>> + u8 port;
>> + __le32 freq;
>> + } __packed tx;
>> +
>> + tx.port = dln2->port;
>> + tx.freq = cpu_to_le32(freq);
>> +
>> + ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
>> + NULL, NULL);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dln2->freq = freq;
>> +
>> + return 0;
>> +}
>> +
>> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)
>> +{
>> + int ret;
>> + __le32 freq;
>> + unsigned len = sizeof(freq);
>> +
>> + ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
>> + &freq, &len);
>
> Same here.
>

OK.

>> + if (ret < 0)
>> + return ret;
>> + if (len < sizeof(freq))
>> + return -EPROTO;
>> +
>> + *res = le32_to_cpu(freq);
>> +
>> + return 0;
>> +}
>> +
>
> [...]
>
>> +static ssize_t dln2_i2c_freq_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct dln2_i2c *dln2 = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n", dln2->freq);
>> +}
>> +
>> +static ssize_t dln2_i2c_freq_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct dln2_i2c *dln2 = dev_get_drvdata(dev);
>> + unsigned long freq;
>> + int ret;
>> +
>> + if (kstrtoul(buf, 0, &freq) || freq < dln2->min_freq ||
>> + freq > dln2->max_freq)
>> + return -EINVAL;
>> +
>> + ret = dln2_i2c_enable(dln2, false);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = dln2_i2c_set_frequency(dln2, freq);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = dln2_i2c_enable(dln2, true);
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(dln2_i2c_freq);
>
> You forgot to add the required documentation of the attribute to
> Documentation/ABI as requested.
>

Hmm, I did add a new entry in Documentation/ABI/ at
testing/sysfs-bus-i2c-busses-dln2. It should be right at the beginning
at the patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/