| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAE1zotKiYGXDbE0yVOz1ROuTxMf_Sfpn-0ghOM1dLEu1oEGZuw@mail.gmail.com>
Date: Tue, 7 Oct 2014 20:52:35 +0300
From: Octavian Purdila <octavian.purdila@...el.com>
To: Johan Hovold <johan@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linus Walleij <linus.walleij@...aro.org>,
Alexandre Courbot <gnurou@...il.com>,
Wolfram Sang <wsa@...-dreams.de>,
Samuel Ortiz <sameo@...ux.intel.com>,
Lee Jones <lee.jones@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
Daniel Baluta <daniel.baluta@...el.com>,
Laurentiu Palcu <laurentiu.palcu@...el.com>,
linux-usb@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
linux-i2c@...r.kernel.org
Subject: Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold <johan@...nel.org> wrote:
> On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:
>> From: Laurentiu Palcu <laurentiu.palcu@...el.com>
>>
>> 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@...el.com>
>> Signed-off-by: Octavian Purdila <octavian.purdila@...el.com>
>> ---
>
> [...]
>
>> +#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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists