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

[Octavian Purdila]

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/