Re: [PATCH v5 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Sat, Dec 13, 2014 at 05:17:42PM +0530, Muthu Mani wrote: > Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor > CYUSBS234 USB-Serial Bridge controller. > > Details about the device can be found at: > http://www.cypress.com/?rID=84126 > > Separate cell drivers are available for I2C and GPIO. SPI and UART > are not supported yet. > > Signed-off-by: Muthu Mani > Signed-off-by: Rajaram Regupathy > --- The patches in this series are still white-space damaged. Try sending it to yourself before reposting to the mailings lists again. And as I already asked you, make sure to remove your corporate confidentiality footers from mails you post to the lists. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Mon, Dec 08, 2014 at 04:00:13PM +, Muthu Mani wrote: > > On Sat, Nov 29, 2014 at 11:49:04AM +0530, Muthu Mani wrote: > > > Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor > > > CYUSBS234 USB-Serial Bridge controller. > > > > > > Details about the device can be found at: > > > http://www.cypress.com/?rID=84126 > > > > > > Signed-off-by: Muthu Mani > > > Signed-off-by: Rajaram Regupathy > > > --- > > > > This patch as well as the other two patches in the series is white-space > > damaged. Please always make sure to run your patches through > > checkpatch.pl before submission (and/or fix your mailer). > > > > Also use git-send-email when submitting your series so that the individual > > patches get threaded properly. > > Sorry about the inconvenience. > I ran checkpatch.pl for all the patches before I submitted them using > git-send-email. > checkpatch doesn't show any error. Then the problem could be your mail server. All (leading) tabs have been converted to spaces. > I used the following command to submit patch. Please let me know if anything > wrong. > $ git format-patch -s -n -v4 master..Driver_v4 > $ git send-email --smtp-encryption=tls --smtp-server= > --smtp-user=m...@cypress.com --smtp-server-port= --to="Samuel Ortiz > " --to="Lee Jones " > --to="Wolfram Sang " --to="Linus Walleij > " --to="Alexandre Courbot " > --to="gre...@linuxfoundation.org" --to="Johan Hovold " > --cc="linux-...@vger.kernel.org" --cc="linux-ker...@vger.kernel.org" > --cc="linux-i2c@vger.kernel.org" --cc="linux-g...@vger.kernel.org" > v4-0001-mfd-add-support-for-Cypress-CYUSBS234-USB-Serial-.patch Have a look at git-send-email's --thread switch. Depending on your settings you may only need to specify all three patches to git-send-email at once to have them threaded properly. > > And always increase (or add a "resend" prefix) when resending. I got two > > v4 series in my inbox. No idea what's the difference, if any. > > In the second v4 patch, I added Greg KH and usb group. There is no code > changes. > I will take care when I resend again. > > Thanks, > Muthu > > > > > Thanks, > > Johan > > This message and any attachments may contain Cypress (or its > subsidiaries) confidential information. If it has been received in > error, please advise the sender and immediately delete this message. Make sure not to include such disclaimers when posting to public mailing lists. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Sat, Nov 29, 2014 at 11:49:04AM +0530, Muthu Mani wrote: > Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor > CYUSBS234 USB-Serial Bridge controller. > > Details about the device can be found at: > http://www.cypress.com/?rID=84126 > > Signed-off-by: Muthu Mani > Signed-off-by: Rajaram Regupathy > --- This patch as well as the other two patches in the series is white-space damaged. Please always make sure to run your patches through checkpatch.pl before submission (and/or fix your mailer). Also use git-send-email when submitting your series so that the individual patches get threaded properly. And always increase (or add a "resend" prefix) when resending. I got two v4 series in my inbox. No idea what's the difference, if any. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: dln2: simplify return flow for dln2_i2c_enable
On Tue, Nov 11, 2014 at 05:20:37PM +0100, Julia Lawall wrote:
> On Tue, 11 Nov 2014, Octavian Purdila wrote:
>
> > On Tue, Nov 11, 2014 at 2:26 PM, Johan Hovold wrote:
> > > On Tue, Nov 11, 2014 at 02:20:57PM +0200, Octavian Purdila wrote:
> > >> This fixes the following kbuild test robot warning:
> > >>
> > >> >> drivers/i2c/busses/i2c-dln2.c:70:1-4: WARNING: end returns can be
> > >> >> simplified if negative or 0 value
> > >>
> > >> Reported-by: kbuild test robot
> > >> Reported-by: Julia Lawall
> > >>
> > >> Signed-off-by: Octavian Purdila
> > >> ---
> > >> drivers/i2c/busses/i2c-dln2.c | 7 +--
> > >> 1 file changed, 1 insertion(+), 6 deletions(-)
> > >>
> > >> diff --git a/drivers/i2c/busses/i2c-dln2.c
> > >> b/drivers/i2c/busses/i2c-dln2.c
> > >> index 010a5fa..b3fb86a 100644
> > >> --- a/drivers/i2c/busses/i2c-dln2.c
> > >> +++ b/drivers/i2c/busses/i2c-dln2.c
> > >> @@ -54,7 +54,6 @@ struct dln2_i2c {
> > >>
> > >> static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> > >> {
> > >> - int ret;
> > >> u16 cmd;
> > >> struct {
> > >> u8 port;
> > >> @@ -67,11 +66,7 @@ static int dln2_i2c_enable(struct dln2_i2c *dln2,
> > >> bool enable)
> > >> else
> > >> cmd = DLN2_I2C_DISABLE;
> > >>
> > >> - ret = dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
> > >> - if (ret < 0)
> > >> - return ret;
> > >> -
> > >> - return 0;
> > >> + return dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
> > >
> > > This looks like a bogus warning. It's not generally equivalent (ret > 0)
> > > and is not mandated by any style guide lines.
> > >
> >
> > In this particular it should be equivalent (with the previous fix) and
> > it saves 5 lines, so I think its worth it.
>
> It's marked as a warning because it is not generally equivalent. But
> there may be many cases where it is equivalent, and in this case the
> documentation for the function said that it should have been. So I think
> that the warning is useful.
I still think "warning" is too strong, "hint" would perhaps be more
appropriate, if at all needed.
Sure we can save four lines of code this way, but we also hide the
return value of the function so that instead of just looking at the
function itself I now have to look at the documentation of the final
function call (and hope it is up to date) to figure out the return
value (e.g. it may return the number of bytes transfered on success).
I also believe using a temporary is preferred for purely aesthetic
reasons in case the final function call has enough parameters that it
needs to use continuation lines.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: dln2: simplify return flow for dln2_i2c_enable
On Tue, Nov 11, 2014 at 02:20:57PM +0200, Octavian Purdila wrote:
> This fixes the following kbuild test robot warning:
>
> >> drivers/i2c/busses/i2c-dln2.c:70:1-4: WARNING: end returns can be
> >> simplified if negative or 0 value
>
> Reported-by: kbuild test robot
> Reported-by: Julia Lawall
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/i2c/busses/i2c-dln2.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
> index 010a5fa..b3fb86a 100644
> --- a/drivers/i2c/busses/i2c-dln2.c
> +++ b/drivers/i2c/busses/i2c-dln2.c
> @@ -54,7 +54,6 @@ struct dln2_i2c {
>
> static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> {
> - int ret;
> u16 cmd;
> struct {
> u8 port;
> @@ -67,11 +66,7 @@ static int dln2_i2c_enable(struct dln2_i2c *dln2, bool
> enable)
> else
> cmd = DLN2_I2C_DISABLE;
>
> - ret = dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
> - if (ret < 0)
> - return ret;
> -
> - return 0;
> + return dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
This looks like a bogus warning. It's not generally equivalent (ret > 0)
and is not mandated by any style guide lines.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mfd: dln2: fix _dln2_transfer return code
On Tue, Nov 11, 2014 at 02:20:56PM +0200, Octavian Purdila wrote: > If wait_for_completion_interruptible_timeout returns a positive value > it may be propagated as the return value of _dln2_transfer. This > contradicts the documentation of the function and exposes unnecessary > internals to the callers. > > This patch makes sure to set the return value to 0 in that case. > > Reported-by: Julia Lawall > Signed-off-by: Octavian Purdila > --- > drivers/mfd/dln2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c > index 9765a17..f0747a1 100644 > --- a/drivers/mfd/dln2.c > +++ b/drivers/mfd/dln2.c > @@ -462,7 +462,8 @@ static int _dln2_transfer(struct dln2_dev *dln2, u16 > handle, u16 cmd, > if (!ret) > ret = -ETIMEDOUT; > goto out_free_rx_slot; > - } > + } else > + ret = 0; You need braces on both branches (without commenting on the fix itself). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 1/3] mfd: add support for Diolan DLN-2 devices
On Thu, Nov 06, 2014 at 01:45:46PM +0200, Octavian Purdila wrote: > On Wed, Nov 5, 2014 at 7:11 PM, Johan Hovold wrote: > > On Wed, Nov 05, 2014 at 07:04:59PM +0200, Octavian Purdila wrote: > >> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > >> Master Adapter DLN-2. Details about the device can be found here: > >> > >> https://www.diolan.com/i2c/i2c_interface.html. > >> > >> Information about the USB protocol can be found in the Programmer's > >> Reference Manual [1], see section 1.7. > >> > >> Because the hardware has a single transmit endpoint and a single > >> receive endpoint the communication between the various DLN2 drivers > >> and the hardware will be muxed/demuxed by this driver. > >> > >> Each DLN2 module will be identified by the handle field within the DLN2 > >> message header. If a DLN2 module issues multiple commands in parallel > >> they will be identified by the echo counter field in the message header. > >> > >> The DLN2 modules can use the dln2_transfer() function to issue a > >> command and wait for its response. They can also register a callback > >> that is going to be called when a specific event id is generated by > >> the device (e.g. GPIO interrupts). The device uses handle 0 for > >> sending events. > >> > >> [1] https://www.diolan.com/downloads/dln-api-manual.pdf > >> > >> Signed-off-by: Octavian Purdila > > > > Reviewed-by: Johan Hovold > > > > Just noticed checkpatch complains about two typos in the header file > > (since v9?). ;) > > > > Ah nice new checkpatch feature, I used checkpath before rebasing the > tree and missed those. I will fix the typos and submit v11 with your > review-by tags. Thanks a lot for your awesome reviews Johan ! You're welcome. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 1/3] mfd: add support for Diolan DLN-2 devices
On Wed, Nov 05, 2014 at 07:04:59PM +0200, Octavian Purdila wrote: > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > Master Adapter DLN-2. Details about the device can be found here: > > https://www.diolan.com/i2c/i2c_interface.html. > > Information about the USB protocol can be found in the Programmer's > Reference Manual [1], see section 1.7. > > Because the hardware has a single transmit endpoint and a single > receive endpoint the communication between the various DLN2 drivers > and the hardware will be muxed/demuxed by this driver. > > Each DLN2 module will be identified by the handle field within the DLN2 > message header. If a DLN2 module issues multiple commands in parallel > they will be identified by the echo counter field in the message header. > > The DLN2 modules can use the dln2_transfer() function to issue a > command and wait for its response. They can also register a callback > that is going to be called when a specific event id is generated by > the device (e.g. GPIO interrupts). The device uses handle 0 for > sending events. > > [1] https://www.diolan.com/downloads/dln-api-manual.pdf > > Signed-off-by: Octavian Purdila Reviewed-by: Johan Hovold Just noticed checkpatch complains about two typos in the header file (since v9?). ;) Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Wed, Nov 05, 2014 at 07:05:01PM +0200, Octavian Purdila wrote: > From: Daniel Baluta > > This patch adds GPIO and IRQ support for the Diolan DLN-2 GPIO module. > > Information about the USB protocol interface can be found in the > Programmer's Reference Manual [1], see section 2.9 for the GPIO > module commands and responses. > > [1] https://www.diolan.com/downloads/dln-api-manual.pdf > > Signed-off-by: Daniel Baluta > Signed-off-by: Octavian Purdila > Acked-by: Linus Walleij Reviewed-by: Johan Hovold -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/4] mfd: add support for Diolan DLN-2 devices
On Mon, Oct 27, 2014 at 06:31:09PM +0200, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/mfd/Kconfig | 11 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/dln2.c | 761
> +++
> include/linux/mfd/dln2.h | 103 +++
> 4 files changed, 876 insertions(+)
> create mode 100644 drivers/mfd/dln2.c
> create mode 100644 include/linux/mfd/dln2.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index c9200d3..4538815a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -189,6 +189,17 @@ config MFD_DA9063
> Additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_DLN2
> + tristate "Diolan DLN2 support"
> + select MFD_CORE
> + depends on USB
> + help
> +
> + This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter
> + DLN-2. Additional drivers such as I2C_DLN2, GPIO_DLN2,
> + etc. must be enabled in order to use the functionality of
> + the device.
> +
> config MFD_MC13XXX
> tristate
> depends on (SPI_MASTER || I2C)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 61f8dbf..2cd7e74 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -175,6 +175,7 @@ obj-$(CONFIG_MFD_STW481X) += stw481x.o
> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
> obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> +obj-$(CONFIG_MFD_DLN2) += dln2.o
>
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> new file mode 100644
> index 000..b3946ef
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
[...]
> +struct dln2_header {
> + __le16 size;
> + __le16 id;
> + __le16 echo;
> + __le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> + struct dln2_header hdr;
> + __le16 result;
> +} __packed;
> +
__packed not needed on either struct above.
[...]
> +/*
> + * Returns true if a valid transfer slot is found. In this case the URB must
> not
> + * be resubmitted imediately in dln2_rx as we need the data when
> dln2_transfer
s/imediately/immediately/
> + * is woke up. It will be resubmitted there.
> + */
> +static bool dln2_transfer_complete(struct dln2_dev *dln2, struct urb *urb,
> +u16 handle, u16 rx_slot)
> +{
> + struct device *dev = &dln2->interface->dev;
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> + struct dln2_rx_context *rxc;
> + bool valid_slot = false;
> +
> + rxc = &rxs->slots[rx_slot];
> +
> + /*
> + * No need to disable interrupts as this lock is not taken in interrupt
> + * context elsewhere in this driver. This function (or its callers) are
> + * also not exported to other modules.
> + */
> + spin_lock(&rxs->lock);
> + if (rxc->in_use && !rxc->urb) {
> + rxc->urb = urb;
> + complete(&rxc->done);
> + valid_slot = true;
> + }
> + spin_unlock(&rxs->lock);
> +
> + if (!valid_slot)
> + dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> +
> + return valid_slot;
> +}
> +
> +static void dln2_run_event_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
> + void *data, int len)
> +{
> + struct dln2_event_cb_entry *i;
> +
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(i, &dln2->event_cb_list, list)
> + if (i->id == id)
> + i->callback(i->pdev, echo, data, len);
No need to continue the search if id is found as it will be unique in
the list.
> +
> + rcu_read_unlock();
> +}
> +
> +static void dln2_rx(struct urb *urb)
>
Re: [PATCH v9 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Mon, Oct 27, 2014 at 06:31:10PM +0200, Octavian Purdila wrote: > From: Laurentiu Palcu > > 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 > Signed-off-by: Octavian Purdila Reviewed-by: Johan Hovold Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Mon, Oct 27, 2014 at 06:31:12PM +0200, Octavian Purdila wrote:
> From: Daniel Baluta
>
> This patch adds GPIO and IRQ support for the Diolan DLN-2 GPIO module.
[...]
> +static void dln2_gpio_pin_set_out_val(struct dln2_gpio *dln2,
> + unsigned int pin, int value)
> +{
> + struct dln2_gpio_pin_val req = {
> + .pin = cpu_to_le16(pin),
> + .value = cpu_to_le16(value),
Drop cpu_to_le16 (value is u8).
> + };
> +
> + dln2_transfer_tx(dln2->pdev, DLN2_GPIO_PIN_SET_OUT_VAL, &req,
> + sizeof(req));
> +}
> +
> +#define DLN2_GPIO_DIRECTION_IN 0
> +#define DLN2_GPIO_DIRECTION_OUT 1
> +
> +static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + struct dln2_gpio_pin req = {
> + .pin = cpu_to_le16(offset),
> + };
> + struct dln2_gpio_pin_val rsp;
> + int len = sizeof(rsp);
> + int ret;
> +
> + ret = dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
> + if (ret < 0)
> + return ret;
> +
> + /* cache the pin direction */
> + ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
> + &req, sizeof(req), &rsp, &len);
> + if (ret < 0)
> + return ret;
> + if (len < sizeof(rsp) || req.pin != rsp.pin) {
> + ret = -EPROTO;
> + goto out_disable;
> + }
> +
> + switch (rsp.value) {
> + case DLN2_GPIO_DIRECTION_IN:
> + clear_bit(offset, dln2->output_enabled);
> + return 0;
> + case DLN2_GPIO_DIRECTION_OUT:
> + set_bit(offset, dln2->output_enabled);
> + return 0;
> + default:
> + ret = -EPROTO;
> + goto out_disable;
> + }
> +
> +out_disable:
> + dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
> + return ret;
> +}
> +
> +static void dln2_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> + dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
> +}
> +
> +static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> + if (test_bit(offset, dln2->output_enabled))
> + return GPIOF_DIR_OUT;
> +
> + return GPIOF_DIR_IN;
> +}
> +
> +static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + int dir;
> +
> + dir = dln2_gpio_get_direction(chip, offset);
> + if (dir < 0)
> + return dir;
> +
> + if (dir == GPIOF_DIR_IN)
> + return dln2_gpio_pin_get_in_val(dln2, offset);
> +
> + return dln2_gpio_pin_get_out_val(dln2, offset);
> +}
> +
> +static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> + dln2_gpio_pin_set_out_val(dln2, offset, value);
> +}
> +
> +static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
> +unsigned dir)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + struct dln2_gpio_pin_val req = {
> + .pin = cpu_to_le16(offset),
> + .value = cpu_to_le16(dir),
Same here.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices
On Mon, Oct 27, 2014 at 03:21:10PM +0200, Octavian Purdila wrote:
> On Thu, Oct 23, 2014 at 6:16 PM, Johan Hovold wrote:
> > On Wed, Oct 15, 2014 at 05:48:08PM +0300, Octavian Purdila wrote:
> > > +struct dln2_event_cb_entry {
> > > + struct list_head list;
> > > + u16 id;
> > > + struct platform_device *pdev;
> > > + dln2_event_cb_t callback;
> > > +};
> > > +
> > > +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> > > +dln2_event_cb_t rx_cb)
> > > +{
> > > + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> > > + struct dln2_event_cb_entry *i, *new_cb;
> >
> > Can you use a name which does not have the same suffix as the actual
> > callback function (i.e. "_cb"). Just call it "entry" or something.
> >
>
> OK.
>
> > > + unsigned long flags;
> > > + int ret = 0;
> > > +
> > > + new_cb = kmalloc(sizeof(*new_cb), GFP_KERNEL);
> >
> > Use kzalloc here.
>
> Why kzalloc? All of the fields are initialized below.
It's good practise to clear any data structure at allocation. The cost
is negligible.
> > > + if (!new_cb)
> > > + return -ENOMEM;
> > > +
> > > + new_cb->id = id;
> > > + new_cb->callback = rx_cb;
> > > + new_cb->pdev = pdev;
> > > +
> > > + spin_lock_irqsave(&dln2->event_cb_lock, flags);
> > > +
> > > + list_for_each_entry(i, &dln2->event_cb_list, list) {
> > > + if (i->id == id) {
> > > + ret = -EBUSY;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!ret)
> > > + list_add_rcu(&new_cb->list, &dln2->event_cb_list);
> > > +
> > > + spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> > > +
> > > + if (ret)
> > > + kfree(new_cb);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL(dln2_register_event_cb);
> > > + ret = mfd_add_hotplug_devices(dev, dln2_devs,
> > > ARRAY_SIZE(dln2_devs));
> >
> > So this now depends on 15bb4d6e8534 ("mfd: core: Add helper function to
> > register hotplug devices") in the MFD tree.
> >
> > Please mention what tree the patch is against in your cover letter (I
> > noticed you had rebased when it no longer applied to 3.17).
> >
> > You should drop the gpiolib patch now that v3.18-rc1 is out as well.
>
> This series is based against the Lee's for-mfd-next-v3.19 tree that
> does not yet contain the gpiolib patch.
Ok, but make sure to include that information in the cover letter.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Wed, Oct 15, 2014 at 05:48:09PM +0300, Octavian Purdila wrote:
> +static int dln2_i2c_xfer(struct i2c_adapter *adapter,
> + struct i2c_msg *msgs, int num)
> +{
> + struct dln2_i2c *dln2 = i2c_get_adapdata(adapter);
> + struct i2c_msg *pmsg;
> + struct device *dev = &dln2->pdev->dev;
You want to use the i2c-adapter device here (not the platform device).
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + int ret;
> +
> + pmsg = &msgs[i];
> +
> + if (pmsg->len > DLN2_I2C_MAX_XFER_SIZE) {
> + dev_warn(dev, "maximum transfer size exceeded\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (pmsg->flags & I2C_M_RD) {
> + ret = dln2_i2c_read(dln2, pmsg->addr, pmsg->buf,
> + pmsg->len);
> + if (ret < 0)
> + return ret;
> +
> + pmsg->len = ret;
> + } else {
> + ret = dln2_i2c_write(dln2, pmsg->addr, pmsg->buf,
> + pmsg->len);
> + if (ret != pmsg->len)
> + return -EPROTO;
> + }
> + }
> +
> + return num;
> +}
[...]
> +static int dln2_i2c_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct dln2_i2c *dln2;
> + struct device *dev = &pdev->dev;
> + struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + dln2 = devm_kzalloc(dev, sizeof(*dln2), GFP_KERNEL);
> + if (!dln2)
> + return -ENOMEM;
> +
> + dln2->buf = devm_kmalloc(dev, DLN2_I2C_BUF_SIZE, GFP_KERNEL);
> + if (!dln2->buf)
> + return -ENOMEM;
> +
> + dln2->pdev = pdev;
> + dln2->port = pdata->port;
> +
> + /* setup i2c adapter description */
> + dln2->adapter.owner = THIS_MODULE;
> + dln2->adapter.class = I2C_CLASS_HWMON;
> + dln2->adapter.algo = &dln2_i2c_usb_algorithm;
> + dln2->adapter.dev.parent = dev;
> + i2c_set_adapdata(&dln2->adapter, dln2);
> + snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), "%s-%s-%d",
> + "dln2-i2c", dev_name(pdev->dev.parent), dln2->port);
> +
> + platform_set_drvdata(pdev, dln2);
> +
> + /* initialize the i2c interface */
No longer "initialisation" since you dropped the frequency setup?
> + ret = dln2_i2c_enable(dln2, true);
> + if (ret < 0) {
> + dev_err(dev, "failed to initialize adapter: %d\n", ret);
Same here.
> + return ret;
> + }
> +
> + /* and finally attach to i2c layer */
> + ret = i2c_add_adapter(&dln2->adapter);
> + if (ret < 0) {
> + dev_err(dev, "failed to add I2C adapter: %d\n", ret);
> + goto out_disable;
> + }
> +
> + return 0;
> +
> +out_disable:
> + dln2_i2c_enable(dln2, false);
> +
> + return ret;
> +}
Looks good otherwise.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 15, 2014 at 05:48:08PM +0300, Octavian Purdila wrote:
Here's some late feedback due to travels. You managed to get two updates
in there so commenting on the diff from v6.
> +struct dln2_event_cb_entry {
> + struct list_head list;
> + u16 id;
> + struct platform_device *pdev;
> + dln2_event_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +dln2_event_cb_t rx_cb)
> +{
> + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> + struct dln2_event_cb_entry *i, *new_cb;
Can you use a name which does not have the same suffix as the actual
callback function (i.e. "_cb"). Just call it "entry" or something.
> + unsigned long flags;
> + int ret = 0;
> +
> + new_cb = kmalloc(sizeof(*new_cb), GFP_KERNEL);
Use kzalloc here.
> + if (!new_cb)
> + return -ENOMEM;
> +
> + new_cb->id = id;
> + new_cb->callback = rx_cb;
> + new_cb->pdev = pdev;
> +
> + spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> + list_for_each_entry(i, &dln2->event_cb_list, list) {
> + if (i->id == id) {
> + ret = -EBUSY;
> + break;
> + }
> + }
> +
> + if (!ret)
> + list_add_rcu(&new_cb->list, &dln2->event_cb_list);
> +
> + spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> + if (ret)
> + kfree(new_cb);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(dln2_register_event_cb);
> +
> +void dln2_unregister_event_cb(struct platform_device *pdev, u16 id)
> +{
> + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> + struct dln2_event_cb_entry *i;
> + unsigned long flags;
> + bool found = false;
> +
> + spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> + list_for_each_entry(i, &dln2->event_cb_list, list) {
> + if (i->id == id) {
> + list_del_rcu(&i->list);
> + found = true;
> + break;
> + }
> + }
> +
> + spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> + if (found) {
> + synchronize_rcu();
> + kfree(i);
> + }
> +}
> +EXPORT_SYMBOL(dln2_unregister_event_cb);
> +
Please add a comment describing the return value (i.e. when the urb had
been saved and shouldn't be resubmitted).
Also could rename this helper so it doesn't appear to be a variant of
dln2_transfer (e.g. something with "complete" in the name).
> +static bool dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> + u16 handle, u16 rx_slot)
> +{
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> + struct dln2_rx_context *rxc;
> + bool ret = false;
> +
> + rxc = &rxs->slots[rx_slot];
> +
> + /*
> + * No need to disable interrupts as this lock is not taken in
> + * interrupt context elsewhere in this driver and this
> + * function (or its callers) are not exported to other
> + * modules.
Why do you break comment lines already at 70 chars?
> + */
> + spin_lock(&rxs->lock);
> + if (rxc->in_use && !rxc->urb) {
> + rxc->urb = urb;
> + complete(&rxc->done);
> + ret = true;
> + }
> + spin_unlock(&rxs->lock);
> +
> + return ret;
> +}
> +
> +static void dln2_run_event_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
> + void *data, int len)
> +{
> + struct dln2_event_cb_entry *i;
> +
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(i, &dln2->event_cb_list, list)
> + if (i->id == id)
> + i->callback(i->pdev, echo, data, len);
> +
> + rcu_read_unlock();
> +}
> +
> +static void dln2_rx(struct urb *urb)
> +{
> + struct dln2_dev *dln2 = urb->context;
> + struct dln2_header *hdr = urb->transfer_buffer;
> + struct device *dev = &dln2->interface->dev;
> + u16 id, echo, handle, size;
> + u8 *data;
> + int len;
> + int err;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + case -EPIPE:
> + /* this urb is terminated, clean up */
> + dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
> + return;
> + default:
> + dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
> + goto out;
> + }
> +
> + if (urb->actual_length < sizeof(struct dln2_header)) {
> + dev_err(dev, "short response: %d\n", urb->actual_length);
> + goto out;
> + }
> +
> + handle = le16_to_cpu(hdr->handle);
> + id = le16_to_cpu(hdr->id);
> + echo = le16_to_cpu(hdr->echo);
> + size = le16_to_cpu(hdr->size);
> +
> + if (size != urb->actual_length) {
> + dev_err(dev, "size misma
Re: [PATCH v7 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Oct 09, 2014 at 11:27:12PM +0300, Octavian Purdila wrote:
> On Thu, Oct 9, 2014 at 10:44 PM, Joe Perches wrote:
> > On Thu, 2014-10-09 at 22:22 +0300, Octavian Purdila wrote:
> >> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> > []
> > +struct dln2_mod_rx_slots {
> > + /* RX slots bitmap */
> > + unsigned long bmap;
> >
> > Probably better as:
> > DECLARE_BITMAP(bmap, DLN2_MAX_RX_SLOTS);
> >
> > Then a lot of &ptr->bmap uses can be ptr->bmap
> >
>
> Originally I was using DECLARE_BITMAP, but during the review process
> Johan suggested to use unsigned long. Now that I think about it, it
> sounds better to use DECLARE_BITMAP and of couse keep using
> find_first_bit, set_bit, etc. Johan do you see any issue with that?
No, that's fine as long as you keep the bitops.
> >> +struct dln2_dev {
> >> + struct usb_device *usb_dev;
> >> + struct usb_interface *interface;
> >> + u8 ep_in;
> >> + u8 ep_out;
> >> +
> >> + struct urb *rx_urb[DLN2_MAX_URBS];
> >> + void *rx_buf[DLN2_MAX_URBS];
> >> +
> >> + struct dln2_mod_rx_slots mod_rx_slots[DLN2_HANDLES];
> >> +
> >> + struct list_head event_cb_list;
> >> + spinlock_t event_cb_lock;
> >> +
> >> + bool disconnect;
> >> + int active_transfers;
> >> + wait_queue_head_t disconnect_wq;
> >> + spinlock_t disconnect_lock;
> >> +};
> >
> > Maybe reorder the bools and u8s to pack this a bit better?
>
> I prefer to keep it this way, it's not wasting a lot since you will
> only have a handful of these devices, and it keeps the related data
> together.
I agree.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 08, 2014 at 03:33:28PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold wrote:
> > On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> >> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold wrote:
> >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >> >
> >> >> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> >> >> + u16 handle, u16 rx_slot)
> >> >> +{
> >> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> >> >> + struct dln2_rx_context *rxc;
> >> >> + struct device *dev = &dln2->interface->dev;
> >> >> +
> >> >> + spin_lock(&rxs->lock);
> >> >
> >> > You must use spin_lock_irqsave here as you call it from the completion
> >> > handler.
> >>
> >> Why? AFAICS the completion handler gets called from the HCD irq handler:
> >
> > The completion handler is currently called with local interrupts
> > disabled but that is about to change once all drivers have been updated:
> >
> > http://marc.info/?l=linux-usb&m=137353360511003&w=2
> >
> > In this case you could probably get away with not disabling interrupts
> > anyway, but using the irqsave versions would make it obvious.
> >
>
> I was not assuming that interrupts are disabled while running the
> completion handler. Since that spinlock is not touched by any other
> interrupt context code I don't think irqsave is necessary.
No, it isn't necessary so leave it as it is.
But you are exporting interfaces to other drivers and it may appear to
someone that those could possibly end up indirectly calling a function
taking that lock in IRQ context. We know that isn't the case now, but I
bet someone will post conversion patch for that spinlock at some point.
;)
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Thu, Oct 09, 2014 at 11:59:50AM +0100, Lee Jones wrote:
> On Thu, 09 Oct 2014, Johan Hovold wrote:
> > On Thu, Oct 09, 2014 at 08:40:29AM +0100, Lee Jones wrote:
> > > On Mon, 06 Oct 2014, Muthu Mani wrote:
> > > > diff --git a/include/linux/mfd/cyusbs23x.h
> > > > b/include/linux/mfd/cyusbs23x.h
> >
> > > > +/* Serial interfaces (I2C, SPI, UART) differ in interface subclass */
> > > > +enum cy_scb_modes {
> > > > + CY_USBS_SCB_DISABLED = 0,
> > > > + CY_USBS_SCB_UART = 1,
> > > > + CY_USBS_SCB_SPI = 2,
> > > > + CY_USBS_SCB_I2C = 3
> > >
> > > No need to number these.
> >
> > As it's not an arbitrary enumeration, I think they should be initialised
> > explicitly.
>
> No need. You are protected by the C Standard:
>
> 6.7.2.2 Enumeration specifiers
>
> "If the first enumerator has no =, the value of its enumeration
> constant is 0. Each subsequent enumerator with no = defines its
> enumeration constant as the value of the constant expression obtained
> by adding 1 to the value of the previous enumeration constant."
>
> There's nothing arbitrary about that.
I obviously wasn't suggesting that the definition of an enum (and the
values of its constants) in c was arbitrary.
My point was that the values of the USB interface subclasses (defined
through the enum) are not arbitrary. In this case they just happen to be
zero-based and consecutive. You cannot reorder, or remove an unused
item, without breaking the driver. By initialising each constant
explicitly this would become apparent.
Using preprocessor defines could be an alternative if you really do not
like initialised enumeration constants.
> > They could be defined in the mfd driver though, as they only
> > appear to be needed during probe.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Thu, Oct 09, 2014 at 08:40:29AM +0100, Lee Jones wrote:
> On Mon, 06 Oct 2014, Muthu Mani wrote:
> > +static int update_ep_details(struct usb_interface *interface,
> > + struct cyusbs23x *cyusbs)
> > +{
> > + struct usb_host_interface *iface_desc;
> > + struct usb_endpoint_descriptor *ep;
> > + int i;
> > +
> > + iface_desc = interface->cur_altsetting;
> > +
> > + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> > +
> > + ep = &iface_desc->endpoint[i].desc;
> > +
> > + if (!cyusbs->bulk_in_ep_num && usb_endpoint_is_bulk_in(ep))
> > + cyusbs->bulk_in_ep_num = ep->bEndpointAddress;
> > + if (!cyusbs->bulk_out_ep_num && usb_endpoint_is_bulk_out(ep))
> > + cyusbs->bulk_out_ep_num = ep->bEndpointAddress;
> > + if (!cyusbs->intr_in_ep_num && usb_endpoint_is_int_in(ep))
> > + cyusbs->intr_in_ep_num = ep->bEndpointAddress;
> > + }
>
> All of the USB specific code in this driver will require a USB Ack.
I'll review it once the incomplete gpio-driver issue has been resolved.
> > + dev_dbg(&interface->dev, "%s intr_in=%d, bulk_in=%d, bulk_out=%d\n",
> > + __func__, cyusbs->intr_in_ep_num ,
> > + cyusbs->bulk_in_ep_num, cyusbs->bulk_out_ep_num);
> > +
> > + if (!cyusbs->bulk_in_ep_num || !cyusbs->bulk_out_ep_num ||
> > + !cyusbs->intr_in_ep_num)
> > + return -ENODEV;
> > +
> > + return 0;
> > +}
[...]
> > diff --git a/include/linux/mfd/cyusbs23x.h b/include/linux/mfd/cyusbs23x.h
> > +/* Serial interfaces (I2C, SPI, UART) differ in interface subclass */
> > +enum cy_scb_modes {
> > + CY_USBS_SCB_DISABLED = 0,
> > + CY_USBS_SCB_UART = 1,
> > + CY_USBS_SCB_SPI = 2,
> > + CY_USBS_SCB_I2C = 3
>
> No need to number these.
As it's not an arbitrary enumeration, I think they should be initialised
explicitly. They could be defined in the mfd driver though, as they only
appear to be needed during probe.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold wrote:
> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >
> >> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> >> + u16 handle, u16 rx_slot)
> >> +{
> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> >> + struct dln2_rx_context *rxc;
> >> + struct device *dev = &dln2->interface->dev;
> >> +
> >> + spin_lock(&rxs->lock);
> >
> > You must use spin_lock_irqsave here as you call it from the completion
> > handler.
>
> Why? AFAICS the completion handler gets called from the HCD irq handler:
The completion handler is currently called with local interrupts
disabled but that is about to change once all drivers have been updated:
http://marc.info/?l=linux-usb&m=137353360511003&w=2
In this case you could probably get away with not disabling interrupts
anyway, but using the irqsave versions would make it obvious.
> >> +static void dln2_disconnect(struct usb_interface *interface)
> >> +{
> >> + struct dln2_dev *dln2 = usb_get_intfdata(interface);
> >> + int i, j;
> >> +
> >> + /* don't allow starting new transfers */
> >> + spin_lock(&dln2->disconnect_lock);
> >> + dln2->disconnect = true;
> >> + spin_unlock(&dln2->disconnect_lock);
> >> +
> >> + /* cancel in progress transfers */
> >> + for (i = 0; i < DLN2_HANDLES; i++) {
> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&rxs->lock, flags);
> >
> > Just stick to spin_lock in this function.
>
> AFAICS disconnect is called from a kernel thread. Are there guarantees
> that we can't get a call to the completion routine while we are
> running it?
Brain fart, nevermind.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: > diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 > b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 > new file mode 100644 > index 000..ad55af6 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 This is a nonstandard path that does not match the attribute path (or any other convention followed under ABI/). I suggest you rename it sysfs-class-i2c-adapter-dln2 > @@ -0,0 +1,5 @@ > +What:/sys/bus/i2c/devices/.../dln2_i2c_freq I think you should rename the attribute "bus_frequency" and name the attribute group "dln2". That way the attribute will show up as ...//dln2/bus_frequency Then the "What:" field should read What: /sys/class/i2c-adapter//dln2/bus_frequency > +Date:September 2014 > +Contact: Octavian Purdila > +Description: > + This attribute shows/sets the frequency (in Hz) of the I2C bus. I'd reword this "Set the frequency...". Please also describe under what circumstances this may fail, that is, when setting a frequency less than or greater than the min or max frequencies reported by the device. Perhaps you should even consider exporting those two values? Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Tue, Oct 07, 2014 at 09:01:27PM +0300, Octavian Purdila wrote: > On Tue, Oct 7, 2014 at 8:10 PM, Johan Hovold wrote: > > On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote: > >> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold wrote: > >> > > >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote: > >> > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > >> > > Master Adapter DLN-2. Details about the device can be found here: > >> > > > >> > >> > >> > >> > > + > >> > > + ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL); > >> > > + if (ret < 0) > >> > > + return ret; > >> > > >> > Is it really worth having this helper only to save a couple of lines on > >> > a dev_err? If you do all resubmissions on completion inline in the > >> > handler, you only have three places where usb_submit_urb is called. > >> > >> I moved the completion in the handler as you suggested. I have kept > >> the helper, would you prefer to remove it? > > > > Moved the "completion"? I was suggesting that the URB resubmission > > should be done inline the URB completion handler. > > > > [ "Completion" may be a little ambiguous. The URB callback is called an > > URB completion handler. Not be confused with the completion structures > > you use to wait for responses. ] > > > > Sorry, I meant to say resubmission instead of completion. > > > It's fine to keep the helper as long as it's clear that the urb has been > > "cached" and should not be resubmitted (inline) in the completion > > handler in that case. > > Not sure I follow you here. I kept the helper and I call it from the > completion handler, from free_rx_slot and from dln2_setup_rx_ubs. Ah sorry, I was referring to your other helper dln2_rx_transfer(). I still think you should do away with the dln2_submit_urb() helper as it needlessly hides what's on going without any real gain. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> + u16 handle, u16 rx_slot)
> +{
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> + struct dln2_rx_context *rxc;
> + struct device *dev = &dln2->interface->dev;
> +
> + spin_lock(&rxs->lock);
You must use spin_lock_irqsave here as you call it from the completion
handler.
> +
> + rxc = &rxs->slots[rx_slot];
> +
> + if (rxc->in_use && !rxc->urb) {
> + rxc->urb = urb;
> + complete(&rxc->done);
> + /* URB will be resubmitted in free_rx_slot */
> + } else {
> + dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> + dln2_submit_urb(dln2, urb, GFP_ATOMIC);
> + }
> +
> + spin_unlock(&rxs->lock);
> +}
> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> + const void *obuf, unsigned obuf_len,
> + void *ibuf, unsigned *ibuf_len)
> +{
> + int ret = 0;
> + u16 result;
> + int rx_slot;
> + struct dln2_response *rsp;
> + struct dln2_rx_context *rxc;
> + struct device *dev;
> + const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> + spin_lock(&dln2->disconnect_lock);
How did you test this version? You never initialise disconnect_lock so
lockdep complains loudly when calling _dln2_transfer during probe.
> + if (!dln2->disconnect)
> + dln2->active_transfers++;
> + else
> + ret = -ENODEV;
> + spin_unlock(&dln2->disconnect_lock);
> +
> + if (ret)
> + return ret;
> +
> + rx_slot = alloc_rx_slot(dln2, handle);
> + if (rx_slot < 0) {
> + ret = rx_slot;
> + goto out_decr;
> + }
> +
> + dev = &dln2->interface->dev;
> +
> + ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> + if (ret < 0) {
> + dev_err(dev, "USB write failed: %d\n", ret);
> + goto out_free_rx_slot;
> + }
> +
> + rxc = &rxs->slots[rx_slot];
> +
> + ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> + if (ret <= 0) {
> + if (!ret)
> + ret = -ETIMEDOUT;
> + goto out_free_rx_slot;
> + }
> +
> + if (!rxc->urb) {
> + ret = -ENODEV;
> + goto out_free_rx_slot;
> + }
> +
> + /* if we got here we know that the response header has been checked */
> + rsp = rxc->urb->transfer_buffer;
> +
> + if (rsp->hdr.size < sizeof(*rsp)) {
> + ret = -EPROTO;
> + goto out_free_rx_slot;
> + }
> +
> + result = le16_to_cpu(rsp->result);
> + if (result) {
> + dev_dbg(dev, "%d received response with error %d\n",
> + handle, result);
> + ret = -EREMOTEIO;
> + goto out_free_rx_slot;
> + }
> +
> + if (!ibuf) {
> + ret = 0;
> + goto out_free_rx_slot;
> + }
> +
> + if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
> + *ibuf_len = rsp->hdr.size - sizeof(*rsp);
> +
> + memcpy(ibuf, rsp + 1, *ibuf_len);
> +
> +out_free_rx_slot:
> + free_rx_slot(dln2, handle, rx_slot);
> +out_decr:
> + spin_lock(&dln2->disconnect_lock);
> + dln2->active_transfers--;
> + spin_unlock(&dln2->disconnect_lock);
> + if (dln2->disconnect)
> + wake_up(&dln2->disconnect_wq);
> +
> + return ret;
> +}
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> + struct dln2_dev *dln2 = usb_get_intfdata(interface);
> + int i, j;
> +
> + /* don't allow starting new transfers */
> + spin_lock(&dln2->disconnect_lock);
> + dln2->disconnect = true;
> + spin_unlock(&dln2->disconnect_lock);
> +
> + /* cancel in progress transfers */
> + for (i = 0; i < DLN2_HANDLES; i++) {
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rxs->lock, flags);
Just stick to spin_lock in this function.
> +
> + /* cancel all response waiters */
> + for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
> + struct dln2_rx_context *rxc = &rxs->slots[j];
> +
> + if (rxc->in_use)
> + complete(&rxc->done);
> + }
> +
> + spin_unlock_irqrestore(&rxs->lock, flags);
> + }
> +
> + /* wait for transfers to end */
> + wait_event(dln2->disconnect_wq, !dln2->active_transfers);
> +
> + mfd_remove_devices(&interface->dev);
> +
> + dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> + const struct usb_device_id *usb_id)
> +{
> + struct usb_host_interface *hostif = i
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Tue, Oct 07, 2014 at 08:52:35PM +0300, Octavian Purdila wrote: > On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold wrote: > > On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: > >> +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. Ah, sorry, I missed that. I'll look at it tomorrow. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote: > On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold wrote: > > > > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote: > > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > > > Master Adapter DLN-2. Details about the device can be found here: > > > > > > > > > + > > > + ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL); > > > + if (ret < 0) > > > + return ret; > > > > Is it really worth having this helper only to save a couple of lines on > > a dev_err? If you do all resubmissions on completion inline in the > > handler, you only have three places where usb_submit_urb is called. > > > > I moved the completion in the handler as you suggested. I have kept > the helper, would you prefer to remove it? Moved the "completion"? I was suggesting that the URB resubmission should be done inline the URB completion handler. [ "Completion" may be a little ambiguous. The URB callback is called an URB completion handler. Not be confused with the completion structures you use to wait for responses. ] It's fine to keep the helper as long as it's clear that the urb has been "cached" and should not be resubmitted (inline) in the completion handler in that case. > > > > > + id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum; > > > > After giving this some more thought, I think you should just > > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the > > other USB MFD drivers and add a convenience helper to register > > hotpluggable MFDs here: > > > > OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges > first and I have to resubmit mine, I will switch to using > mfd_add_hotplug_devices. Lee merged the helper function earlier today. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Thu, Sep 25, 2014 at 07:07:34PM +0300, Octavian Purdila wrote: > From: Daniel Baluta > > This patch adds GPIO and IRQ support for the Diolan DLN-2 GPIO module. > > Information about the USB protocol interface can be found in the > Programmer's Reference Manual [1], see section 2.9 for the GPIO > module commands and responses. > > [1] https://www.diolan.com/downloads/dln-api-manual.pdf > > Signed-off-by: Daniel Baluta > Signed-off-by: Octavian Purdila > --- Looks good. I'll provide my reviewed-by tag to the final submission. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:
> From: Laurentiu Palcu
>
> 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
> Signed-off-by: Octavian Purdila
> ---
[...]
> +#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.
> + 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.
> + 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.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Mon, Oct 06, 2014 at 06:22:29PM +0530, Muthu Mani wrote: > Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor > CYUSBS234 USB-Serial Bridge controller. > > Details about the device can be found at: > http://www.cypress.com/?rID=84126 > > Signed-off-by: Muthu Mani > Signed-off-by: Rajaram Regupathy > --- Please resend this series and include a revision changelog per patch (you can keep it under the cut-off line). Also, make sure not to drop thread participants from CC when resending. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/dln2.c | 751
> +++
> include/linux/mfd/dln2.h | 67 +
> 4 files changed, 828 insertions(+)
> create mode 100644 drivers/mfd/dln2.c
> create mode 100644 include/linux/mfd/dln2.h
[...]
> +#define DLN2_HW_ID 0x200
> +#define DLN2_USB_TIMEOUT 200 /* in ms */
> +#define DLN2_MAX_RX_SLOTS16
> +#define DLN2_MAX_URBS16
> +#define DLN2_RX_BUF_SIZE 512
> +
> +enum dln2_handle {
> + DLN2_HANDLE_EVENT,
This is the only handle that was fixed (0), right? Initialise this one
explicitly in case any one ever tries to reorder them.
> + DLN2_HANDLE_CTRL,
> + DLN2_HANDLE_GPIO,
> + DLN2_HANDLE_I2C,
> + DLN2_HANDLES
> +};
> +
> +/*
> + * Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data.
> + */
> +struct dln2_rx_context {
> + /* completion used to wait for a response */
> + struct completion done;
> +
> + /* if non-NULL the URB contains the response */
> + struct urb *urb;
> +
> + /* if true then this context is used to wait for a response */
> + bool in_use;
> +};
> +
> +/*
> + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to identify the module in
> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> + * slots field and find the receive context for a particular request.
> + */
> +struct dln2_mod_rx_slots {
> + /* RX slots bitmap */
> + unsigned long bmap;
> +
> + /* used to wait for a free RX slot */
> + wait_queue_head_t wq;
> +
> + /* used to wait for an RX operation to complete */
> + struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> +
> + /* avoid races between alloc/free_rx_slot and dln2_rx_transfer */
> + spinlock_t lock;
> +};
> +
> +struct dln2_dev {
> + struct usb_device *usb_dev;
> + struct usb_interface *interface;
> + u8 ep_in;
> + u8 ep_out;
> +
> + struct urb *rx_urb[DLN2_MAX_URBS];
> + void *rx_buf[DLN2_MAX_URBS];
> +
> + struct dln2_mod_rx_slots mod_rx_slots[DLN2_HANDLES];
> +
> + struct list_head event_cb_list;
> + spinlock_t event_cb_lock;
> +
> + bool disconnect;
> + int active_transfers;
> + wait_queue_head_t disconnect_wq;
> + spinlock_t disconnect_lock;
> +};
> +
> +struct dln2_event_cb_entry {
> + struct list_head list;
> + u16 id;
> + struct platform_device *pdev;
> + dln2_event_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +dln2_event_cb_t rx_cb)
> +{
> + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> + struct dln2_event_cb_entry *i, *new;
> + unsigned long flags;
> + int ret = 0;
> +
> + new = kmalloc(sizeof(*new), GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + new->id = id;
> + new->callback = rx_cb;
> + new->pdev = pdev;
> +
> + spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> + list_for_each_entry(i, &dln2->event_cb_list, list) {
> + if (i->id == id) {
> + ret = -EBUSY;
> + break;
> + }
> + }
> +
> + if (!ret)
> + list_add_rcu(&new->list, &dln2->event_cb_list);
> +
> + spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> + if (ret)
> + kfree(new);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(dln2_register_event_c
Re: [PATCH 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter
On Thu, Sep 25, 2014 at 05:47:11AM +, Muthu Mani wrote:
> > -Original Message-
> > From: Johan Hovold [mailto:jhov...@gmail.com] On Behalf Of Johan Hovold
> > > +static int cy_gpio_direction_input(struct gpio_chip *chip,
> > > + unsigned offset) {
> > > + return 0;
> >
> > Aren't the GPIOs output-only?
>
> No.
Then you should implement this callback to configure the pin as an
input.
Or can the configuration only be made through your configuration tools
and stored in eeprom?
Then you need to retrieve the actual config and implement these
callbacks (e.g. fail if not matching the eeprom config) along with
get_direction().
> > > +}
> > > +
> > > +static int cy_gpio_direction_output(struct gpio_chip *chip,
> > > + unsigned offset, int value) {
> > > + return 0;
> > > +}
> > > +
> > > +static int cyusbs23x_gpio_probe(struct platform_device *pdev) {
> > > + struct cyusbs23x *cyusbs;
> > > + struct cyusbs_gpio *cy_gpio;
> > > + int ret = 0;
> > > +
> > > + dev_dbg(&pdev->dev, "%s\n", __func__);
> > > +
> > > + cyusbs = dev_get_drvdata(pdev->dev.parent);
> > > +
> > > + cy_gpio = devm_kzalloc(&pdev->dev, sizeof(*cy_gpio), GFP_KERNEL);
> > > + if (cy_gpio == NULL)
> > > + return -ENOMEM;
> > > +
> > > + cy_gpio->cyusbs = cyusbs;
> > > + /* registering gpio */
> > > + cy_gpio->gpio.label = "cyusbs23x gpio";
> > > + cy_gpio->gpio.dev = &pdev->dev;
> > > + cy_gpio->gpio.owner = THIS_MODULE;
> > > + cy_gpio->gpio.base = -1;
> > > + cy_gpio->gpio.ngpio = 12; /* total GPIOs */
> >
> > I think you need to read out the gpio config from the device eeprom and
> > implement a request() callback where you verify that a requested gpio is
> > actually available (mode dependent, and there are never more than 10 gpios
> > availble).
>
> In this initial driver, all GPIOs are made available to user space.
> Additional functionalities will be added in the next version of the driver.
I think this needs to be implemented now. It's needed for direction
handling as well as mentioned above.
> > > + cy_gpio->gpio.can_sleep = true;
> > > + cy_gpio->gpio.set = cy_gpio_set;
> > > + cy_gpio->gpio.get = cy_gpio_get;
> > > + cy_gpio->gpio.direction_input = cy_gpio_direction_input;
> > > + cy_gpio->gpio.direction_output = cy_gpio_direction_output;
> > > + ret = gpiochip_add(&cy_gpio->gpio);
> > > + if (ret < 0) {
> > > + dev_err(cy_gpio->gpio.dev, "could not add gpio");
> > > + goto error;
> > > + }
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter
On Tue, Sep 30, 2014 at 01:08:29PM +0200, Johan Hovold wrote:
> On Thu, Sep 25, 2014 at 11:21:58AM +0530, Muthu Mani wrote:
> > +static int cy_gpio_direction_input(struct gpio_chip *chip,
> > + unsigned offset)
> > +{
> > + return 0;
> > +}
> > +
> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
> > + unsigned offset, int value)
> > +{
> > + return 0;
> > +}
>
> This is not a correct implementation of these. You're are supposed to
> return what direction the gpio is configured for. You should also add a
> call back to set the direction.
That was backwards; these are supposed to set the direction, and then
you should also implement the get_direction callback.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter
On Thu, Sep 25, 2014 at 11:21:58AM +0530, Muthu Mani wrote:
> Adds support for USB-GPIO interface of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
>
> The GPIO get/set can be done through vendor command on control endpoint.
>
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
>
> Signed-off-by: Muthu Mani
> Signed-off-by: Rajaram Regupathy
> ---
> drivers/gpio/Kconfig | 13 +++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-cyusbs23x.c | 182
> ++
> 3 files changed, 196 insertions(+)
> create mode 100644 drivers/gpio/gpio-cyusbs23x.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9de1515..932e07c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -886,6 +886,19 @@ config GPIO_BCM_KONA
>
> comment "USB GPIO expanders:"
>
> +config GPIO_CYUSBS23X
> + tristate "CYUSBS23x GPIO support"
> + depends on MFD_CYUSBS23X && USB
> + help
> + Say yes here to access the GPIO signals of Cypress
> + Semiconductor CYUSBS23x USB Serial Bridge Controller.
> +
> + This driver enables the GPIO interface of CYUSBS23x USB Serial
> + Bridge controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called gpio-cyusbs23x.
> +
> config GPIO_VIPERBOARD
> tristate "Viperboard GPIO a & b support"
> depends on MFD_VIPERBOARD && USB
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d024e3..3ad89f1 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_GPIO_BT8XX)+= gpio-bt8xx.o
> obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> obj-$(CONFIG_GPIO_CS5535)+= gpio-cs5535.o
> obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> +obj-$(CONFIG_GPIO_CYUSBS23X) += gpio-cyusbs23x.o
> obj-$(CONFIG_GPIO_DA9052)+= gpio-da9052.o
> obj-$(CONFIG_GPIO_DA9055)+= gpio-da9055.o
> obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o
> diff --git a/drivers/gpio/gpio-cyusbs23x.c b/drivers/gpio/gpio-cyusbs23x.c
> new file mode 100644
> index 000..ad83b00
> --- /dev/null
> +++ b/drivers/gpio/gpio-cyusbs23x.c
> @@ -0,0 +1,182 @@
> +/*
> + * GPIO subdriver for Cypress CYUSBS234 USB-Serial Bridge controller.
> + * Details about the device can be found at:
> + *http://www.cypress.com/?rID=84126
> + * All GPIOs are exposed for get operation and only unused GPIOs can be set.
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Muthu Mani
> + *
> + * Additional contributors include:
> + * Rajaram Regupathy
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +#define CY_GPIO_GET_LEN (2)
No parentheses.
> +
> +struct cyusbs_gpio {
> + struct gpio_chip gpio;
> + struct cyusbs23x *cyusbs;
> +};
> +
> +static int cy_gpio_get(struct gpio_chip *chip,
> + unsigned offset)
> +{
> + int ret;
> + char *buf;
> + u16 wIndex, wValue;
> + struct cyusbs_gpio *gpio =
> + container_of(chip, struct cyusbs_gpio, gpio);
Add helper macro?
> + struct cyusbs23x *cyusbs = gpio->cyusbs;
> +
> + dev_dbg(&cyusbs->usb_intf->dev, "%s: %d\n", __func__,
> + offset);
Drop this.
> + wValue = offset;
> + wIndex = 0;
> + buf = kmalloc(CY_GPIO_GET_LEN, GFP_KERNEL);
Error handling missing.
> +
> + mutex_lock(&cyusbs->lock);
Locking not needed.
> + ret = usb_control_msg(cyusbs->usb_dev,
> + usb_rcvctrlpipe(cyusbs->usb_dev, 0),
> + CY_GPIO_GET_VALUE_CMD,
> + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> + wValue, wIndex, buf, CY_GPIO_GET_LEN, 2000);
timeout define.
> + mutex_unlock(&cyusbs->lock);
> +
> + dev_dbg(&cyusbs->usb_intf->dev, "%s: %d %02x %02x\n", __func__,
> + ret, buf[0], buf[1]);
Use the gpio_chip device rather than usb-interface device in gpio
callbacks for debug and error messages.
> +
> + if (ret == CY_GPIO_GET_LEN) {
> + if (buf[0] == 0)
> + ret = buf[1];
> + else
> + ret = -EINVAL;
> + } else {
> + ret = -EREMOTEIO;
> + }
Just use -EIO for both error cases.
> +
> + kfree(buf);
> + return ret;
> +}
> +
> +static void cy_gpio_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + int ret;
> + u16 wIndex, wValue;
> + struct cyusbs_gpio *gpio =
> + container_of(chip, struct cyusbs_gpio, gpio);
Re: [PATCH v2 2/3] i2c: add support for Cypress CYUSBS234 USB-I2C adapter
On Thu, Sep 25, 2014 at 11:21:15AM +0530, Muthu Mani wrote:
> Adds support for USB-I2C interface of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
>
> The read/write operation is setup using vendor command through control
> endpoint
> and actual data transfer happens through bulk in/out endpoints.
>
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
>
> Signed-off-by: Muthu Mani
> Signed-off-by: Rajaram Regupathy
> ---
> drivers/i2c/busses/Kconfig | 12 +
> drivers/i2c/busses/Makefile| 1 +
> drivers/i2c/busses/i2c-cyusbs23x.c | 514
> +
> 3 files changed, 527 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-cyusbs23x.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..1fdc6ec 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -848,6 +848,18 @@ config I2C_RCAR
>
> comment "External I2C/SMBus adapter drivers"
>
> +config I2C_CYUSBS23X
> +tristate "CYUSBS23x I2C adapter"
> +depends on MFD_CYUSBS23X && USB
> +help
> + Say yes if you would like to access Cypress CYUSBS23x I2C device.
> +
> + This driver enables the I2C interface of CYUSBS23x USB Serial Bridge
> + controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called i2c-cyusbs23x.
> +
> config I2C_DIOLAN_U2C
> tristate "Diolan U2C-12 USB adapter"
> depends on USB
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..cbf28cb 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
> obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
>
> # External I2C/SMBus adapter drivers
> +obj-$(CONFIG_I2C_CYUSBS23X) += i2c-cyusbs23x.o
> obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o
> obj-$(CONFIG_I2C_PARPORT)+= i2c-parport.o
> obj-$(CONFIG_I2C_PARPORT_LIGHT) += i2c-parport-light.o
> diff --git a/drivers/i2c/busses/i2c-cyusbs23x.c
> b/drivers/i2c/busses/i2c-cyusbs23x.c
> new file mode 100644
> index 000..5cf60f0
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cyusbs23x.c
> @@ -0,0 +1,514 @@
> +/*
> + * I2C subdriver for Cypress CYUSBS234 USB-Serial Bridge controller.
> + * Details about the device can be found at:
> + *http://www.cypress.com/?rID=84126
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Rajaram Regupathy
> + *
> + * Additional contributors include:
> + * Muthu Mani
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +/*
> + * It exposes sysfs entries under the i2c adapter for getting the i2c
> transfer
> + * status, reset i2c read/write module, get/set nak and stop bits.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +#define CY_I2C_MODE_WRITE 1
> +#define CY_I2C_MODE_READ 0
Make this an enum that you pass to get_status and reset.
> +
> +#define CY_I2C_XFER_STATUS_LEN 3
> +
> +struct cyusbs_i2c_config {
> + u32 frequency;/* Frequency of operation. Only valid values are
> + 100KHz and 400KHz */
Multi-line comments should be on the form
/*
* ...
*/
> + u8 slave_addr;/* Slave address to be used when in slave mode */
> + u8 is_msb_first; /* Whether to transmit MSB first */
> + u8 is_master; /* Whether block is to be configured as a master:
> + 1 - The block functions as I2C master;
> + 0 - The block functions as I2C slave */
> + u8 s_ignore; /* Ignore general call in slave mode */
> + u8 clock_stretch; /* Whether to stretch clock in case of no FIFO
> + availability */
> + u8 is_loopback; /* Whether to loop back TX data to RX. Valid
> + only for debug purposes */
> + u8 reserved[6]; /* Reserved for future use */
> +} __packed;
> +
> +struct cyusbs_i2c {
> + struct i2c_adapter i2c_adapter;
> + struct cyusbs_i2c_config *i2c_config;
> +
> + bool is_stop_bit;
> + bool is_nak_bit;
What are these for? Document along with attributes (see below).
Seems like use_ rather than is_ would be more appropriate.
> +};
> +
> +#define to_cyusbs_i2c(a) container_of(a, struct cyusbs_i2c, i2c_adapter)
> +
> +static int cy_i2c_get_status(struct device *d, char *buf, u16 mode);
> +static int cy_i2c_reset(struct device *d, u16 mode);
> +
> +static ssize_t i2c_read_status_show(struct device *d,
> +
Re: [PATCH 2/3] i2c: add support for Cypress CYUSBS234 USB-I2C adapter
On Thu, Sep 25, 2014 at 05:46:16AM +, Muthu Mani wrote:
> > > +static int cy_i2c_xfer(struct i2c_adapter *adapter,
> > > +struct i2c_msg *msgs, int num) {
> > > + int ret = 0;
> > > + struct cyusbs_i2c *cy_i2c;
> > > + struct cyusbs23x *cyusbs = (struct cyusbs23x
> > > +*)adapter->algo_data;
> > > +
> > > + dev_dbg(&adapter->dev, "%s\n", __func__);
> > > +
> > > + if (num > 1) {
> > > + dev_err(&adapter->dev, "i2c_msg number is > 1\n");
> > > + return -EIO;
> > > + }
> >
> > Why not handle multiple messages?
>
> This iteration of the driver is designed to handle only a single
> message at a time.
> It can be expanded to handle multiple messages in future.
No, please do that now. It's just a matter of adding a for loop.
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Thu, Sep 25, 2014 at 11:20:12AM +0530, Muthu Mani wrote:
> Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
>
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
>
> Signed-off-by: Muthu Mani
> Signed-off-by: Rajaram Regupathy
> ---
> drivers/mfd/Kconfig | 12 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/cyusbs23x.c | 190
> ++
> include/linux/mfd/cyusbs23x.h | 57 +
> 4 files changed, 260 insertions(+)
> create mode 100644 drivers/mfd/cyusbs23x.c
> create mode 100644 include/linux/mfd/cyusbs23x.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..a31e9e3 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -116,6 +116,18 @@ config MFD_ASIC3
> This driver supports the ASIC3 multifunction chip found on many
> PDAs (mainly iPAQ and HTC based ones)
>
> +config MFD_CYUSBS23X
> +tristate "Cypress CYUSBS23x USB Serial Bridge controller"
> + select MFD_CORE
> + depends on USB
> + default n
> + help
> + Say yes here if you want support for Cypress Semiconductor
> + CYUSBS23x USB-Serial Bridge controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called cyusbs23x.
> +
> config PMIC_DA903X
> bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..fc5bcd1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ si476x-core-y := si476x-cmd.o si476x-prop.o si476x-i2c.o
> obj-$(CONFIG_MFD_SI476X_CORE)+= si476x-core.o
>
> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> +obj-$(CONFIG_MFD_CYUSBS23X) += cyusbs23x.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> obj-$(CONFIG_MFD_PM8921_CORE)+= pm8921-core.o ssbi.o
> obj-$(CONFIG_TPS65911_COMPARATOR)+= tps65911-comparator.o
> diff --git a/drivers/mfd/cyusbs23x.c b/drivers/mfd/cyusbs23x.c
> new file mode 100644
> index 000..cf2d53b
> --- /dev/null
> +++ b/drivers/mfd/cyusbs23x.c
> @@ -0,0 +1,190 @@
> +/*
> + * Cypress USB-Serial Bridge Controller USB adapter driver
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Muthu Mani
> + *
> + * Additional contributors include:
> + * Rajaram Regupathy
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +/*
> + * This is core MFD driver for Cypress Semiconductor CYUSBS234 USB-Serial
> + * Bridge controller. CYUSBS234 offers a single channel serial interface
> + * (I2C/SPI/UART). It can be configured to enable either of I2C, SPI, UART
> + * interfaces. The GPIOs are also available to access.
> + * Details about the device can be found at:
> + *http://www.cypress.com/?rID=84126
> + *
> + * Separate cell drivers are available for I2C and GPIO. SPI and UART are not
> + * supported yet. All GPIOs are exposed for get operation. However, only
> + * unused GPIOs can be set.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +static const struct usb_device_id cyusbs23x_usb_table[] = {
> + { USB_DEVICE(0x04b4, 0x0004) }, /* Cypress Semiconductor */
> + { } /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, cyusbs23x_usb_table);
> +
> +static const struct mfd_cell cyusbs23x_i2c_devs[] = {
> + {
> + .name = "cyusbs23x-i2c",
> + },
> + {
> + .name = "cyusbs23x-gpio",
> + },
> +};
> +
> +static int update_ep_details(struct usb_interface *interface,
> + struct cyusbs23x *cyusbs)
> +{
> + struct usb_host_interface *iface_desc;
> + struct usb_endpoint_descriptor *ep;
> + int i;
> +
> + dev_dbg(&interface->dev, "%s\n", __func__);
Drop this.
> +
> + iface_desc = interface->cur_altsetting;
> +
> + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> +
> + dev_dbg(&interface->dev, "%s %d/%d\n",
> + __func__, i, iface_desc->desc.bNumEndpoints);
Drop this one as well. lsusb can always be used to give the endpoint
configuration (count).
> +
> + ep = &iface_desc->endpoint[i].desc;
> +
> + if (!cyusbs->bulk_in_ep_num &&
> + usb_endpoint_is_bulk_in(ep)) {
Indent continuation lines at least two tabs (throughout).
But why are you even breaking this one? Merge the lines and remove the
braces.
> + cyusbs->bulk_in_ep_num = ep->bEndpointAddress;
> + }
> + if (!cyusbs->bulk_out_ep_
Re: [PATCH v5 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Sep 25, 2014 at 01:41:16PM +0300, Octavian Purdila wrote: > On Thu, Sep 25, 2014 at 1:30 PM, Johan Hovold wrote: > > On Thu, Sep 25, 2014 at 01:25:24PM +0300, Octavian Purdila wrote: > > > >> Johan, I think we don't really need the spinlock, the disconnect flag > >> and an atomic counter should work. Do you see any issues with that? > > > > No, you need to test and increment atomically so the lock is needed. > > > > Consider what could happen if you get a disconnect after testing but > > before incrementing. > > I am still not seeing the problem. We would continue with the > increment, we will try to send which will fail and go on the error > path where we will decrement and wake_up. What am I missing? The whole point of the counter and flag is to make sure that no transfers are started after the flag is set. If you remove the lock you cannot guarantee that. Disconnect sets the flag (after you test it in transfer() but before incrementing the counter), checks the counter which is 0 and proceeds with deregistration and deallocation. Then transfer() gets to run... Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Sep 25, 2014 at 01:25:24PM +0300, Octavian Purdila wrote: > Johan, I think we don't really need the spinlock, the disconnect flag > and an atomic counter should work. Do you see any issues with that? No, you need to test and increment atomically so the lock is needed. Consider what could happen if you get a disconnect after testing but before incrementing. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Sep 24, 2014 at 05:54:15PM +0300, Octavian Purdila wrote:
> On Wed, Sep 24, 2014 at 4:54 PM, Johan Hovold wrote:
> > On Wed, Sep 24, 2014 at 04:36:22PM +0300, Octavian Purdila wrote:
> >> On Wed, Sep 24, 2014 at 1:48 PM, Johan Hovold wrote:
> >> > On Fri, Sep 19, 2014 at 11:22:42PM +0300, Octavian Purdila wrote:
> >>
> >>
> >>
> >> >> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> >> >> + * slots field and find the receive context for a particular
> >> >> + * request.
> >> >> + */
> >> >> +struct dln2_mod_rx_slots {
> >> >> + /* RX slots bitmap */
> >> >> + unsigned long bmap;
> >> >> +
> >> >> + /* used to wait for a free RX slot */
> >> >> + wait_queue_head_t wq;
> >> >> +
> >> >> + /* used to wait for an RX operation to complete */
> >> >> + struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> >> >> +
> >> >> + /* device has been disconnected */
> >> >> + bool disconnected;
> >> >
> >> > This belongs in the dln2_dev struct.
> >> >
> >> > I think you're overcomplicating the disconnect handling by intertwining
> >> > it with your slots.
> >> >
> >> > Add a lock, an active-transfer counter, a disconnected flag, and a wait
> >> > queue to struct dln2_dev.
> >> >
> >>
> >> I agree that disconnected is better suited in dln2_dev.
> >>
> >> However, I don't think that we need the active-transfer counter and a
> >> new wait queue. We can simply use the existing waiting queues and the
> >> implicit alloc_rx_slot()/free_rx_slot() calls to see if we are still
> >> waiting for I/O.
> >
> > Just because you can reuse them doesn't mean it's a good idea. By
> > separating a generic disconnect solution from your custom slot
> > implementation we get something that is way easier to verify for
> > correctness and that could also be reused in other drivers.
>
> Maybe I miss-understood what you are proposing, let me try to
> summarize it to see if I got it right.
>
> You are suggesting to add a counter, increment it in alloc_rx_slot(),
> decrement it in free_rx_slot().
No increment it at the start of _dln2_transfer, and decrement it before
returning from that function.
> Then add a new waitqueue in dln2_dev
> and in free_rx_slot() wake it up while in disconnect do a wait_event()
> on it and check for the counter.
Where you also wake the disconnect (or wait-until-sent) wait queue.
> Also, alloc_rx_slot() should fail if
> the disconnect flag is set.
That is not required, but you can bail out early after alloc_rx_slot if
the disconnect flag is set (no locking).
> In this case we are still coupled to the slots implementation, in the
> sense that you would need to understand the slots implementation to
> understand how the disconnect works. We are also doing two wake-up
> operations which I find redundant and which does not add much value in
> clarity (since we still need to wake-up all completions for each
> handle).
>
> I do agree that using a counter instead of checking the bitmaps is
> cleaner though.
You only need to the wake up if disconnected is set when returning from
_dln2_transfer.
Sure, the optimisation bit -- to abort any ongoing transfer -- still
requires some insight into the slot implementation.
But this way everything disconnect related (correctness-wise) is
isolated to _dln2_transfer and dln2_disconnect.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Sep 24, 2014 at 04:36:22PM +0300, Octavian Purdila wrote:
> On Wed, Sep 24, 2014 at 1:48 PM, Johan Hovold wrote:
> > On Fri, Sep 19, 2014 at 11:22:42PM +0300, Octavian Purdila wrote:
>
>
>
> >> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> >> + * slots field and find the receive context for a particular
> >> + * request.
> >> + */
> >> +struct dln2_mod_rx_slots {
> >> + /* RX slots bitmap */
> >> + unsigned long bmap;
> >> +
> >> + /* used to wait for a free RX slot */
> >> + wait_queue_head_t wq;
> >> +
> >> + /* used to wait for an RX operation to complete */
> >> + struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> >> +
> >> + /* device has been disconnected */
> >> + bool disconnected;
> >
> > This belongs in the dln2_dev struct.
> >
> > I think you're overcomplicating the disconnect handling by intertwining
> > it with your slots.
> >
> > Add a lock, an active-transfer counter, a disconnected flag, and a wait
> > queue to struct dln2_dev.
> >
>
> I agree that disconnected is better suited in dln2_dev.
>
> However, I don't think that we need the active-transfer counter and a
> new wait queue. We can simply use the existing waiting queues and the
> implicit alloc_rx_slot()/free_rx_slot() calls to see if we are still
> waiting for I/O.
Just because you can reuse them doesn't mean it's a good idea. By
separating a generic disconnect solution from your custom slot
implementation we get something that is way easier to verify for
correctness and that could also be reused in other drivers.
>
>
> >> +
> >> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> >> + const void *obuf, unsigned obuf_len,
> >> + void *ibuf, unsigned *ibuf_len)
> >> +{
> >> + int ret = 0;
> >> + u16 result;
> >> + int rx_slot;
> >> + unsigned long flags;
> >> + struct dln2_response *rsp;
> >> + struct dln2_rx_context *rxc;
> >> + struct device *dev;
> >> + const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> >> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> >> +
> >
> > Check the disconnected flag before incrementing the transfer count
> > (while holding the device lock) here. Then decrement counter before
> > returning and wake up an waiters if disconnected below.
> >
> > That is sufficient to implement wait-until-io-has-completed. Anything
> > else you do below and in the helper functions is only to speed things
> > up at disconnect (and can be done without locking the device).
> >
> >> + rx_slot = alloc_rx_slot(rxs);
> >> + if (rx_slot < 0)
> >> + return rx_slot;
> >> +
> >> + dev = &dln2->interface->dev;
> >> +
> >> + ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> >> + if (ret < 0) {
> >> + free_rx_slot(dln2, rxs, rx_slot);
> >
> > goto out_free_rx_slot
> >
> >> + dev_err(dev, "USB write failed: %d", ret);
> >> + return ret;
> >> + }
> >> +
> >> + rxc = &rxs->slots[rx_slot];
> >> +
> >> + ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> >> + if (ret <= 0) {
> >> + if (!ret)
> >> + ret = -ETIMEDOUT;
> >> + goto out_free_rx_slot;
> >> + }
> >> +
> >> + spin_lock_irqsave(&rxs->lock, flags);
> >> +
> >> + if (!rxc->urb) {
> >
> > Just check the disconnected flag directly here. Locking not needed (see
> > below).
> >
>
> I think we need the check here for the case when we cancel the
> completion and no response has been received yet. In that case rx->urb
> will be NULL (even if we remove the rx->urb = NULL statement in
> dln2_stop).
But the disconnect flag will also be set and makes it more obvious what
is going on.
[...]
> >> +static void dln2_disconnect(struct usb_interface *interface)
> >> +{
> >> + struct dln2_dev *dln2 = usb_get_intfdata(interface);
> >> +
> >
> > First set the disconnected flag directly here to prevent any new
> > transfers from starting.
> >
> >> + dln2_stop(dln2);
> >
> > Then do all the completions (to speed things up somewhat).
> >
> > Then wait for the transfer counter to reach 0.
> >
> >> + mfd_remove_devices(&interface->dev);
> >> + dln2_free(dln2);
> >> +}
> >> +
>
> As I mentioned above, I don't think we need the transfer counter, we
> can rely on the slots bitmap. Yes, a counter is simpler but it also
> requires adding a new waiting queue and a new lock.
That's really not a big deal. See above.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Fri, Sep 19, 2014 at 11:22:45PM +0300, Octavian Purdila wrote:
> +struct dln2_gpio {
> + struct platform_device *pdev;
> + struct gpio_chip gpio;
> +
> + /*
> + * Cache pin direction to save us one transfer, since the
> + * hardware has separate commands to read the in and out
> + * values. Bit set for out, bit clear for in.
> + */
> + DECLARE_BITMAP(pin_dir, DLN2_GPIO_MAX_PINS);
Using the more common name output_enabled might be preferred? Then you
can even get rid of (part of) the comment.
> +
> + DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
> + DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
> + DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
> + struct dln2_irq_work *irq_work;
> +};
[...]
> +#define DLN2_GPIO_DIRECTION_IN 0
> +#define DLN2_GPIO_DIRECTION_OUT 1
> +
> +static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + struct dln2_gpio_pin req = {
> + .pin = cpu_to_le16(offset),
> + };
> + struct dln2_gpio_pin_val rsp;
> + int len = sizeof(rsp);
> + int ret;
> +
> + ret = dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
> + if (ret < 0)
> + return ret;
> +
> + /* cache the pin direction */
> + ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
> + &req, sizeof(req), &rsp, &len);
> + if (ret < 0)
> + return ret;
> + if (len < sizeof(rsp) || req.pin != rsp.pin)
> + return -EPROTO;
Disable the pin?
> +
> + switch (rsp.value) {
> + case DLN2_GPIO_DIRECTION_IN:
> + clear_bit(offset, dln2->pin_dir);
> + return 0;
> + case DLN2_GPIO_DIRECTION_OUT:
> + set_bit(offset, dln2->pin_dir);
> + return 0;
> + default:
> + return -EPROTO;
Disable the pin?
> + }
> +}
[...]
> +static struct platform_driver dln2_gpio_driver = {
> + .driver.name= "dln2-gpio",
> + .driver.owner = THIS_MODULE,
No need to set the owner field when using the module_platform_driver
macro.
> + .probe = dln2_gpio_probe,
> + .remove = dln2_gpio_remove,
> +};
> +
> +module_platform_driver(dln2_gpio_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta +MODULE_DESCRIPTION("Driver for the Diolan DLN2 GPIO interface");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dln2-gpio");
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] gpiolib: add irq_not_threaded flag to gpio_chip
On Wed, Sep 24, 2014 at 10:54:51AM +0200, Linus Walleij wrote: > On Fri, Sep 19, 2014 at 10:22 PM, Octavian Purdila > wrote: > > > Some GPIO chips (e.g. the DLN2 USB adapter) have blocking get/set > > operation but do not need a threaded irq handler. > > > > Signed-off-by: Octavian Purdila > > Usually I don't apply patches adding interfaces with no users, but > this seems very useful, so patch applied. I guess your driver will > appear on v3.19+ so then you can rely on this having been merged > for v3.18. Octavian, please include this one in any future revision of you series so that it is self-contained (at least until v3.18-rc1 is out) nonetheless. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Fri, Sep 19, 2014 at 11:22:43PM +0300, Octavian Purdila wrote:
> +struct dln2_i2c {
> + struct platform_device *pdev;
> + struct i2c_adapter adapter;
> + u32 freq;
> + u32 min_freq;
> + u32 max_freq;
> + /*
> + * 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.
> + */
> + union {
> + struct {
> + u8 port;
> + u8 addr;
> + u8 mem_addr_len;
> + __le32 mem_addr;
> + __le16 buf_len;
> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> + } __packed tx;
> + struct {
> + __le16 buf_len;
> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> + } __packed rx;
> + } *buf;
Please declare these structs separately from the dln2_i2c struct so you
can use temporary pointers below.
Perhaps just allocate a large enough buffer (or page) here and keep you
transfer-buffer declarations in the two functions where they are used if
you prefer.
> +};
>
> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> + struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> + int ret;
> + u8 port;
> + u16 cmd;
> +
> + port = pdata->port;
Store the port number in the dln2_i2c struct rather than access it
through the platform data for every I/O operation.
> +
> + if (enable)
> + cmd = DLN2_I2C_ENABLE;
> + else
> + cmd = DLN2_I2C_DISABLE;
> +
> + ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(port), NULL, NULL);
> + 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;
> + struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> +
> + tx.port = pdata->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;
> + struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> + u8 port = pdata->port;
> + __le32 freq;
> + unsigned len = sizeof(freq);
> +
> + ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(port),
> + &freq, &len);
> + if (ret < 0)
> + return ret;
> + if (len < sizeof(freq))
> + return -EPROTO;
> +
> + *res = le32_to_cpu(freq);
> +
> + return 0;
> +}
> +
> +static int dln2_i2c_setup(struct dln2_i2c *dln2)
> +{
> + int ret;
> +
> + ret = dln2_i2c_enable(dln2, false);
> + if (ret < 0)
> + return ret;
> +
> + ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MIN_FREQUENCY,
> + &dln2->min_freq);
> + if (ret < 0)
> + return 0;
return ret
> +
> + ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MAX_FREQUENCY,
> + &dln2->max_freq);
> + if (ret < 0)
> + return 0;
return ret
> +
> + ret = dln2_i2c_set_frequency(dln2, DLN2_I2C_FREQ_STD);
> + if (ret < 0)
> + return ret;
> +
> + ret = dln2_i2c_enable(dln2, true);
> +
> + return ret;
> +}
> +
> +static int dln2_i2c_write(struct dln2_i2c *dln2, u8 addr,
> + u8 *data, u16 data_len)
> +{
> + struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> + int ret;
> + unsigned len;
> +
> + dln2->buf->tx.port = pdata->port;
> + dln2->buf->tx.addr = addr;
> + dln2->buf->tx.mem_addr_len = 0;
> + dln2->buf->tx.mem_addr = 0;
> + dln2->buf->tx.buf_len = cpu_to_le16(data_len);
> + memcpy(dln2->buf->tx.buf, data, data_len);
> +
> + len = sizeof(dln2->buf->tx) + data_len - DLN2_I2C_MAX_XFER_SIZE;
> + ret = dln2_transfer(dln2->pdev, DLN2_I2C_WRITE, &dln2->buf->tx, len,
> + NULL, NULL);
Use a temporary pointer to the tx struct above for readability.
> + if (ret < 0)
> + return ret;
> +
> + return data_len;
> +}
> +
> +static int dln2_i2c_read(struct dln2_i2c *dln2, u16 addr, u8 *data,
> + u16 data_len)
> +{
> + struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> + int ret;
> + struct {
> + u8 port;
> + u8 addr;
> + u8 mem_addr_len;
> + __le32 mem_addr;
> + __le16 buf_len;
> + } __packed tx;
> + unsigned rx_len;
> +
> + tx.port = pdata->port;
> + tx.addr =
Re: [PATCH v5 1/4] mfd: add support for Diolan DLN-2 devices
On Fri, Sep 19, 2014 at 11:22:42PM +0300, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/dln2.c | 758
> +++
> include/linux/mfd/dln2.h | 67 +
> 4 files changed, 835 insertions(+)
> create mode 100644 drivers/mfd/dln2.c
> create mode 100644 include/linux/mfd/dln2.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..7bcf895 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -183,6 +183,15 @@ config MFD_DA9063
> Additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_DLN2
> + tristate "Diolan DLN2 support"
> + select MFD_CORE
> + depends on USB
> + help
> + This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter DLN-2.
> + Additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> config MFD_MC13XXX
> tristate
> depends on (SPI_MASTER || I2C)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..591988d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
> obj-$(CONFIG_MFD_AS3722) += as3722.o
> obj-$(CONFIG_MFD_STW481X)+= stw481x.o
> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
> +obj-$(CONFIG_MFD_DLN2) += dln2.o
>
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> new file mode 100644
> index 000..36c53cd
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
> @@ -0,0 +1,758 @@
> +/*
> + * Driver for the Diolan DLN-2 USB adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + * i2c-diolan-u2c.c
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct dln2_header {
> + __le16 size;
> + __le16 id;
> + __le16 echo;
> + __le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> + struct dln2_header hdr;
> + __le16 result;
> +} __packed;
> +
> +#define DLN2_GENERIC_MODULE_ID 0x00
> +#define DLN2_GENERIC_CMD(cmd)DLN2_CMD(cmd,
> DLN2_GENERIC_MODULE_ID)
> +#define CMD_GET_DEVICE_VER DLN2_GENERIC_CMD(0x30)
> +#define CMD_GET_DEVICE_SNDLN2_GENERIC_CMD(0x31)
> +
> +#define DLN2_HW_ID 0x200
> +#define DLN2_USB_TIMEOUT 200 /* in ms */
> +#define DLN2_MAX_RX_SLOTS16
> +#define DLN2_MAX_URBS16
> +#define DLN2_RX_BUF_SIZE 512
> +
> +#define DLN2_HANDLE_EVENT0
> +#define DLN2_HANDLE_CTRL 1
> +#define DLN2_HANDLE_GPIO 2
> +#define DLN2_HANDLE_I2C 3
> +#define DLN2_HANDLES 4
This should be an enum.
> +
> +
> +/*
> + * Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data.
> + */
> +struct dln2_rx_context {
> + /* completion used to wait a response */
wait for
> + struct completion done;
> +
> + /* if non-NULL the URB contains the response */
> + struct urb *urb;
> +
> + /* if true then this context is used to wait for a respo
Re: [PATCH 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter
On Mon, Sep 22, 2014 at 03:04:18PM +0530, Muthu Mani wrote:
> Adds support for USB-GPIO interface of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
>
> The GPIO get/set can be done through vendor command
> on control endpoint.
>
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
>
> Signed-off-by: Muthu Mani
> Signed-off-by: Rajaram Regupathy
> ---
> drivers/gpio/Kconfig | 13 +++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-cyusbs23x.c | 190
> ++
> 3 files changed, 204 insertions(+)
> create mode 100644 drivers/gpio/gpio-cyusbs23x.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9de1515..932e07c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -886,6 +886,19 @@ config GPIO_BCM_KONA
>
> comment "USB GPIO expanders:"
>
> +config GPIO_CYUSBS23X
> + tristate "CYUSBS23x GPIO support"
> + depends on MFD_CYUSBS23X && USB
> + help
> + Say yes here to access the GPIO signals of Cypress
> + Semiconductor CYUSBS23x USB Serial Bridge Controller.
> +
> + This driver enables the GPIO interface of CYUSBS23x USB Serial
> + Bridge controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called gpio-cyusbs23x.
> +
> config GPIO_VIPERBOARD
> tristate "Viperboard GPIO a & b support"
> depends on MFD_VIPERBOARD && USB
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d024e3..3ad89f1 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_GPIO_BT8XX)+= gpio-bt8xx.o
> obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> obj-$(CONFIG_GPIO_CS5535)+= gpio-cs5535.o
> obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> +obj-$(CONFIG_GPIO_CYUSBS23X) += gpio-cyusbs23x.o
> obj-$(CONFIG_GPIO_DA9052)+= gpio-da9052.o
> obj-$(CONFIG_GPIO_DA9055)+= gpio-da9055.o
> obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o
> diff --git a/drivers/gpio/gpio-cyusbs23x.c b/drivers/gpio/gpio-cyusbs23x.c
> new file mode 100644
> index 000..8aa3ab6
> --- /dev/null
> +++ b/drivers/gpio/gpio-cyusbs23x.c
> @@ -0,0 +1,190 @@
> +/*
> + * Cypress USB-Serial Bridge Controller GPIO driver
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Muthu Mani
> + *
> + * Additional contributors include:
> + * Rajaram Regupathy
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +#define CY_GPIO_GET_LEN (2)
> +
> +struct cyusbs_gpio {
> + struct gpio_chip gpio;
> + struct cyusbs23x *cyusbs;
> +};
> +
> +static int cy_gpio_get(struct gpio_chip *chip,
> + unsigned offset)
> +{
> + int ret;
> + char buf[CY_GPIO_GET_LEN];
Buffers used for USB (DMA) transfers cannot be allocated on the stack.
Use kmalloc and friends.
> + __u16 wIndex, wValue;
u16 throughout.
> + struct cyusbs_gpio *gpio =
> + container_of(chip, struct cyusbs_gpio, gpio);
> + struct cyusbs23x *cyusbs = gpio->cyusbs;
> +
> + dev_dbg(&cyusbs->usb_intf->dev, "%s: %d\n", __func__,
> + offset);
> + wValue = offset;
> + wIndex = 0;
> +
> + mutex_lock(&cyusbs->lock);
> + ret = usb_control_msg(cyusbs->usb_dev,
> + usb_rcvctrlpipe(cyusbs->usb_dev, 0),
> + CY_GPIO_GET_VALUE_CMD,
> + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> + wValue, wIndex, buf, CY_GPIO_GET_LEN, 2000);
> + mutex_unlock(&cyusbs->lock);
> +
> + dev_dbg(&cyusbs->usb_intf->dev, "%s: %d %02x %02x\n", __func__,
> + ret, buf[0], buf[1]);
> +
> + if (ret == CY_GPIO_GET_LEN) {
> + if (buf[0] == 0)
> + ret = buf[1];
> + else
> + ret = -EINVAL;
> + } else {
> + ret = -EREMOTEIO;
> + }
> +
> + return ret;
> +}
> +
> +static void cy_gpio_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + int ret;
> + __u16 wIndex, wValue;
> + struct cyusbs_gpio *gpio =
> + container_of(chip, struct cyusbs_gpio, gpio);
> + struct cyusbs23x *cyusbs = gpio->cyusbs;
> +
> + dev_dbg(&cyusbs->usb_intf->dev, "%s: %d\n", __func__,
> + offset);
> + wValue = offset;
> + wIndex = value;
> +
> + mutex_lock(&cyusbs->lock);
> + ret = usb_control_msg(cyusbs->usb_dev,
> + usb_sndctrlpipe(cyusbs->usb_dev, 0),
> + CY_GPIO_SET_VALUE_CMD,
> +
Re: [PATCH 2/3] i2c: add support for Cypress CYUSBS234 USB-I2C adapter
On Mon, Sep 22, 2014 at 03:03:46PM +0530, Muthu Mani wrote:
> Adds support for USB-I2C interface of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
>
> The read/write operation is setup using vendor command through
> control endpoint and actual data transfer happens through
> bulk in/out endpoints.
>
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
>
> Signed-off-by: Muthu Mani
> Signed-off-by: Rajaram Regupathy
> ---
> drivers/i2c/busses/Kconfig | 12 +
> drivers/i2c/busses/Makefile| 1 +
> drivers/i2c/busses/i2c-cyusbs23x.c | 534
> +
> 3 files changed, 547 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-cyusbs23x.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..1fdc6ec 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -848,6 +848,18 @@ config I2C_RCAR
>
> comment "External I2C/SMBus adapter drivers"
>
> +config I2C_CYUSBS23X
> +tristate "CYUSBS23x I2C adapter"
> +depends on MFD_CYUSBS23X && USB
> +help
> + Say yes if you would like to access Cypress CYUSBS23x I2C device.
> +
> + This driver enables the I2C interface of CYUSBS23x USB Serial Bridge
> + controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called i2c-cyusbs23x.
> +
> config I2C_DIOLAN_U2C
> tristate "Diolan U2C-12 USB adapter"
> depends on USB
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..cbf28cb 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
> obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
>
> # External I2C/SMBus adapter drivers
> +obj-$(CONFIG_I2C_CYUSBS23X) += i2c-cyusbs23x.o
> obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o
> obj-$(CONFIG_I2C_PARPORT)+= i2c-parport.o
> obj-$(CONFIG_I2C_PARPORT_LIGHT) += i2c-parport-light.o
> diff --git a/drivers/i2c/busses/i2c-cyusbs23x.c
> b/drivers/i2c/busses/i2c-cyusbs23x.c
> new file mode 100644
> index 000..6fade25
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cyusbs23x.c
> @@ -0,0 +1,534 @@
> +/*
> + * Cypress USB-Serial Bridge Controller I2C-USB adapter driver
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Rajaram Regupathy
> + *
> + * Additional contributors include:
> + * Muthu Mani
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +#define CYUSBS23X_I2C_NAME "cyusbs23x_i2c"
> +
> +#pragma pack(1)
> +struct cyusbs_i2c_config {
> + __u32 frequency; /* Frequency of operation. Only valid values are
> + 100KHz and 400KHz */
u32 -- drop the __ prefix throughout.
> +__u8 slave_addr; /* Slave address to be used when in slave mode */
> + bool is_msb_first; /* Whether to transmit MSB first */
Use an explicitly sized type (e.g. u8) and not bool for this struct.
> + bool is_master;/* Whether block is to be configured as a master:
> + 1 - The block functions as I2C master;
> + 0 - The block functions as I2C slave */
> + bool s_ignore; /* Ignore general call in slave mode */
> + bool clock_stretch;/* Whether to stretch clock in case of no FIFO
> + availability */
> + bool is_loopback; /* Whether to loop back TX data to RX. Valid
> + only for debug purposes */
> + __u8 reserved[6]; /* Reserved for future use */
> +};
> +#pragma pack()
Use __packed rather than pragma if at all needed.
> +
> +struct cyusbs_i2c {
> + struct i2c_adapter i2c_adapter;
> + struct cyusbs_i2c_config i2c_config;
Buffers used for USB (DMA) transfers needs to be allocated separately
from the containing struct.
> +
> + bool is_stop_bit;
> + bool is_nak_bit;
> +};
> +
> +#define CY_I2C_MODE_WRITE 1
> +#define CY_I2C_MODE_READ 0
> +
> +static int cy_i2c_get_status(struct device *d, char *buf, __u16 mode);
> +static int cy_i2c_reset(struct device *d, __u16 mode);
> +
> +static ssize_t show_i2c_read_status(struct device *d,
> + struct device_attribute *attr, char *buf)
> +{
> + dev_dbg(d, "%s\n", __func__);
> +
> + return cy_i2c_get_status(d, buf, CY_I2C_MODE_READ);
> +}
> +static DEVICE_ATTR(i2c_read_status, 0444, show_i2c_read_status, NULL);
What are these attributes used for? Can't they be handled through the
i2c-framew
Re: [PATCH 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller
On Mon, Sep 22, 2014 at 03:02:52PM +0530, Muthu Mani wrote:
> Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
>
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
Ok, so this is a bit of an odd bird.
First of all it has a single channel serial interface and is not even
configured in i2c mode by default. The three modes uart, i2c, and spi
are mutually exclusive and switching modes and pin configuration appears
to require a Cypress (proprietary?) tool.
You currently expose all 12 pins as gpios although some of these would
be allocated for the serial interface. It appears this information could
(should) be retrieved from flash at probe time.
You should probably also read-out the preprogrammed mode and let the
i2c-subdriver's probe fail unless in i2c mode.
>
> Signed-off-by: Muthu Mani
> Signed-off-by: Rajaram Regupathy
> ---
> drivers/mfd/Kconfig | 12
> drivers/mfd/Makefile | 1 +
> drivers/mfd/cyusbs23x.c | 163
> ++
> include/linux/mfd/cyusbs23x.h | 51 +
> 4 files changed, 227 insertions(+)
> create mode 100644 drivers/mfd/cyusbs23x.c
> create mode 100644 include/linux/mfd/cyusbs23x.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..a31e9e3 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -116,6 +116,18 @@ config MFD_ASIC3
> This driver supports the ASIC3 multifunction chip found on many
> PDAs (mainly iPAQ and HTC based ones)
>
> +config MFD_CYUSBS23X
> +tristate "Cypress CYUSBS23x USB Serial Bridge controller"
> + select MFD_CORE
> + depends on USB
> + default n
> + help
> + Say yes here if you want support for Cypress Semiconductor
> + CYUSBS23x USB-Serial Bridge controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called cyusbs23x.
> +
> config PMIC_DA903X
> bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..fc5bcd1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ si476x-core-y := si476x-cmd.o si476x-prop.o si476x-i2c.o
> obj-$(CONFIG_MFD_SI476X_CORE)+= si476x-core.o
>
> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> +obj-$(CONFIG_MFD_CYUSBS23X) += cyusbs23x.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> obj-$(CONFIG_MFD_PM8921_CORE)+= pm8921-core.o ssbi.o
> obj-$(CONFIG_TPS65911_COMPARATOR)+= tps65911-comparator.o
> diff --git a/drivers/mfd/cyusbs23x.c b/drivers/mfd/cyusbs23x.c
> new file mode 100644
> index 000..824f020
> --- /dev/null
> +++ b/drivers/mfd/cyusbs23x.c
> @@ -0,0 +1,163 @@
> +/*
> + * Cypress USB-Serial Bridge Controller USB adapter driver
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Muthu Mani
> + *
> + * Additional contributors include:
> + * Rajaram Regupathy
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +static const struct usb_device_id cyusbs23x_usb_table[] = {
> + { USB_DEVICE(0x04b4, 0x0004) }, /* Cypress Semiconductor */
> + { } /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, cyusbs23x_usb_table);
> +
> +static const struct mfd_cell cyusbs23x_i2c_devs[] = {
> + {
> + .name = "cyusbs23x-i2c",
> + },
> + {
> + .name = "cyusbs23x-gpio",
> + },
> +};
> +
> +static int update_ep_details(struct usb_interface *interface,
> + struct cyusbs23x *cyusbs)
> +{
> + struct usb_host_interface *iface_desc;
> + struct usb_endpoint_descriptor *ep;
> + int i;
> +
> + dev_dbg(&interface->dev, "%s\n", __func__);
> +
> + iface_desc = interface->cur_altsetting;
> +
> + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> +
> + dev_dbg(&interface->dev, "%s %d/%d\n",
> + __func__, i, iface_desc->desc.bNumEndpoints);
> +
> + ep = &iface_desc->endpoint[i].desc;
> +
> + if (!cyusbs->bulk_in_ep_num &&
> + usb_endpoint_is_bulk_in(ep)) {
> + cyusbs->bulk_in_ep_num = ep->bEndpointAddress;
> + }
> + if (!cyusbs->bulk_out_ep_num &&
> + usb_endpoint_is_bulk_out(ep)) {
> + cyusbs->bulk_out_ep_num = ep->bEndpointAddress;
> + }
> + if (!cyusbs->intr_in_ep_num &&
> + usb_endp
Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Thu, Sep 18, 2014 at 06:54:34PM +0300, Octavian Purdila wrote: > On Thu, Sep 18, 2014 at 3:46 PM, Johan Hovold wrote: > > On Thu, Sep 18, 2014 at 03:43:07PM +0300, Octavian Purdila wrote: > >> On Thu, Sep 18, 2014 at 1:54 PM, Johan Hovold wrote: > >> > On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote: > > > >> > Either way, it looks like this could race with get_direction() if you > >> > get a set_direction() while get_direction() is retrieving the direction > >> > from the device. > >> > > >> > This would break gpio_get(). > >> > > >> I don't think gpio_set_direction() and gpio_get() are allowed to race. > > > > I wrote that set_direction() and get_direction() could race, which in > > turn would break gpio_get() as you would be caching the wrong > > direction setting. > > > > OK, I now see the problem. I think doing this in get_direction() will > fix the issue: > > if (!test_and_set_bit(offset, dln2->pin_dir_set)) > set/clear_bit(offset, dln2->pin_dir); > > because gpiolib calls get_direction() while requesting a pin and > request cannot race with itself. Which means that get_direction() can > not race with itself the first time it is called, when the set/clear > operation will be run. > > And because we know that get_direction() is called first, we can even > remove the set/clear operation from set_direction(). Why not simply fetch the direction in request() and get rid of the pin_dir_set bitmask? Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:
> +MODULE_AUTHOR("Octavian Purdila ");
> +MODULE_DESCRIPTION(DRIVER_NAME " driver");
> +MODULE_LICENSE("GPL");
Just noticed both the i2c and gpio driver is lacking a MODULE_ALIAS to
enable module auto-loading.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Thu, Sep 18, 2014 at 03:43:07PM +0300, Octavian Purdila wrote: > On Thu, Sep 18, 2014 at 1:54 PM, Johan Hovold wrote: > > On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote: > > Either way, it looks like this could race with get_direction() if you > > get a set_direction() while get_direction() is retrieving the direction > > from the device. > > > > This would break gpio_get(). > > > I don't think gpio_set_direction() and gpio_get() are allowed to race. I wrote that set_direction() and get_direction() could race, which in turn would break gpio_get() as you would be caching the wrong direction setting. But perhaps gpiolib prevents this from happening. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:
> +static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
> +{
> + int ret;
> + int slot;
> +
> + /* No need to timeout here, the wait is bounded by the timeout
> + * in _dln2_transfer
> + */
> + ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
> + if (ret < 0)
> + return ret;
> +
> + return slot;
> +}
> +
> +static void free_rx_slot(struct dln2_dev *dln2, struct dln2_mod_rx_slots
> *rxs,
> + int slot)
> +{
> + struct urb *urb = NULL;
> + unsigned long flags;
> + struct dln2_rx_context *rxc;
> + struct device *dev = &dln2->interface->dev;
> + int ret;
> +
> + spin_lock_irqsave(&rxs->lock, flags);
> +
> + clear_bit(slot, &rxs->bmap);
> +
> + rxc = &rxs->slots[slot];
> + rxc->connected = false;
> + urb = rxc->urb;
> + rxc->urb = NULL;
> + reinit_completion(&rxc->done);
> +
> + spin_unlock_irqrestore(&rxs->lock, flags);
> +
> + if (urb) {
> + ret = usb_submit_urb(urb, GFP_KERNEL);
> + if (ret < 0)
> + dev_err(dev, "failed to re-submit RX URB: %d\n", ret);
> + }
> +
> + wake_up_interruptible(&rxs->wq);
> +}
> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> + u16 handle, u16 rx_slot)
> +{
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> + struct dln2_rx_context *rxc;
> + struct device *dev = &dln2->interface->dev;
> + int err;
> +
> + spin_lock(&rxs->lock);
> + rxc = &rxs->slots[rx_slot];
> + if (rxc->connected) {
> + rxc->urb = urb;
> + complete(&rxc->done);
> + } else {
> + dev_warn(dev, "dropping response %d/%d", handle, rx_slot);
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> + if (err < 0)
> + dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> + }
> + spin_unlock(&rxs->lock);
> +}
> +static void dln2_rx(struct urb *urb)
> +{
> + int err;
> + struct dln2_dev *dln2 = urb->context;
> + struct dln2_header *hdr = urb->transfer_buffer;
> + struct device *dev = &dln2->interface->dev;
> + u16 id, echo, handle, size;
> + u8 *data;
> + int len;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + case -EPIPE:
> + /* this urb is terminated, clean up */
> + dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
> + return;
> + default:
> + dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
> + goto out;
> + }
> +
> + if (urb->actual_length < sizeof(struct dln2_header)) {
> + dev_err(dev, "short response: %d\n", urb->actual_length);
> + goto out;
> + }
> +
> + handle = le16_to_cpu(hdr->handle);
> + id = le16_to_cpu(hdr->id);
> + echo = le16_to_cpu(hdr->echo);
> + size = le16_to_cpu(hdr->size);
> +
> + if (size != urb->actual_length) {
> + dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d
> actual %d\n",
> + handle, id, echo, size, urb->actual_length);
> + goto out;
> + }
> +
> + if (handle >= DLN2_MAX_MODULES) {
> + dev_warn(dev, "RX: invalid handle %d\n", handle);
> + goto out;
> + }
> +
> + data = urb->transfer_buffer + sizeof(struct dln2_header);
> + len = urb->actual_length - sizeof(struct dln2_header);
> +
> + if (handle == DLN2_HANDLE_EVENT) {
> + dln2_run_rx_callbacks(dln2, id, echo, data, len);
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> + if (err < 0)
> + goto out_submit_failed;
> + } else {
> + dln2_rx_transfer(dln2, urb, handle, echo);
> + }
> +
> + return;
> +
> +out:
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> +out_submit_failed:
> + if (err < 0)
> + dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +}
> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> + const void *obuf, unsigned obuf_len,
> + void *ibuf, unsigned *ibuf_len)
> +{
> + int ret = 0;
> + u16 result;
> + int rx_slot;
> + struct dln2_response *rsp;
> + struct dln2_rx_context *rxc;
> + struct device *dev = &dln2->interface->dev;
> + const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> + struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> + rx_slot = alloc_rx_slot(rxs);
> + if (rx_slot < 0)
> + return rx_slot;
> +
> + ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> + if (ret < 0) {
> + free_rx_slot(dln2, rxs, rx_slot);
Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote:
> +#define DLN2_GPIO_DIRECTION_IN 0
> +#define DLN2_GPIO_DIRECTION_OUT 1
> +
> +static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> + int ret;
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + struct dln2_gpio_pin req = {
> + .pin = cpu_to_le16(offset),
> + };
> + struct dln2_gpio_pin_val rsp;
> + int len = sizeof(rsp);
> +
> + if (test_bit(offset, dln2->pin_dir_set)) {
> + rsp.value = !!test_bit(offset, dln2->pin_dir);
> + goto report;
> + }
> +
> + ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
> + &req, sizeof(req), &rsp, &len);
> + if (ret < 0)
> + return ret;
> + if (len < sizeof(rsp) || req.pin != rsp.pin)
> + return -EPROTO;
> + set_bit(offset, dln2->pin_dir_set);
You shouldn't set this flag until you've stored the direction.
> +
> +report:
> + switch (rsp.value) {
> + case DLN2_GPIO_DIRECTION_IN:
> + clear_bit(offset, dln2->pin_dir);
> + return 1;
> + case DLN2_GPIO_DIRECTION_OUT:
> + set_bit(offset, dln2->pin_dir);
> + return 0;
> + default:
> + return -EPROTO;
> + }
> +}
> +
> +static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + int dir;
> +
> + dir = dln2_gpio_get_direction(chip, offset);
> + if (dir < 0)
> + return dir;
> +
> + if (dir)
> + return dln2_gpio_pin_get_in_val(dln2, offset);
> +
> + return dln2_gpio_pin_get_out_val(dln2, offset);
> +}
> +
> +static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> + dln2_gpio_pin_set_out_val(dln2, offset, value);
> +}
> +
> +static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
> +unsigned dir)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + struct dln2_gpio_pin_val req = {
> + .pin = cpu_to_le16(offset),
> + .value = cpu_to_le16(dir),
> + };
> +
> + set_bit(offset, dln2->pin_dir_set);
Set flag after you store the direction below.
> + if (dir == DLN2_GPIO_DIRECTION_OUT)
> + set_bit(offset, dln2->pin_dir);
> + else
> + clear_bit(offset, dln2->pin_dir);
Either way, it looks like this could race with get_direction() if you
get a set_direction() while get_direction() is retrieving the direction
from the device.
This would break gpio_get().
> +
> + return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
> + &req, sizeof(req), NULL, NULL);
> +}
> +static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
> +unsigned type, unsigned period)
> +{
> + struct {
> + __le16 pin;
> + u8 type;
> + __le16 period;
> + } __packed req = {
> + .pin = cpu_to_le16(pin),
> + .type = type,
> + .period = cpu_to_le16(period),
> + };
> +
> + return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_EVENT_CFG,
> + &req, sizeof(req), NULL, NULL);
> +}
> +
> +static void dln2_irq_work(struct work_struct *w)
> +{
> + struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
> + struct dln2_gpio *dln2 = iw->dln2;
> + u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
> +
> + if (test_bit(iw->pin, dln2->irqs_enabled))
> + dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
> + else
> + dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
> +}
> +
> +static void dln2_irq_enable(struct irq_data *irqd)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> + struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> + int pin = irqd_to_hwirq(irqd);
> +
> + set_bit(pin, dln2->irqs_enabled);
> + schedule_work(&dln2->irq_work[pin].work);
> +}
> +
> +static void dln2_irq_disable(struct irq_data *irqd)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> + struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> + int pin = irqd_to_hwirq(irqd);
> +
> + clear_bit(pin, dln2->irqs_enabled);
> + schedule_work(&dln2->irq_work[pin].work);
> +}
> +static int dln2_gpio_probe(struct platform_device *pdev)
> +{
> + struct dln2_gpio *dln2;
> + struct device *dev = &pdev->dev;
> + int pins = dln2_gpio_get_pin_count(pdev);
Again, don't do non-trivial intialisations when declaring your variables.
> + int i, ret;
> +
> + if (pins < 0) {
> + d
Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Thu, Sep 18, 2014 at 11:49:19AM +0300, Octavian Purdila wrote:
> On Thu, Sep 18, 2014 at 11:19 AM, Johan Hovold wrote:
> > On Wed, Sep 17, 2014 at 01:07:51PM +0300, Octavian Purdila wrote:
> >> On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold wrote:
> >>
> >>
> >>
> >> >> + /*
> >> >> + * 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.
> >> >> + */
> >> >> + union {
> >> >> + struct {
> >> >> + u8 port;
> >> >> + u8 addr;
> >> >> + u8 mem_addr_len;
> >> >> + __le32 mem_addr;
> >> >> + __le16 buf_len;
> >> >> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> >> + } __packed tx;
> >> >> + struct {
> >> >> + __le16 buf_len;
> >> >> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> >> + } __packed rx;
> >> >> + } buf;
> >> >
> >> > While this works in this case due to the extra copy you do in
> >> > dln2_transfer, allocating buffers that would (generally) be used for DMA
> >> > transfers as part of a larger structure is a recipe for trouble.
> >> >
> >> > It's probably better to allocate separately, if only to prevent people
> >> > from thinking there might be a bug here.
> >> >
> >>
> >> Just to make sure I understand this, what could the issues be? The
> >> buffers not being aligned or not allocated in continuous physical
> >> memory?
> >
> > Yes, the buffer (and any subsequent field) would have to be cache-line
> > aligned to avoid corruption due to cache-line sharing on some systems.
> >
>
> Ah, ok, makes sense now. But is it safe to use kmalloc() in this case?
> Does kmalloc() prevent cache line sharing?
Yes, it does (as long as you allocate the buffer separately from the
containing struct).
> >>
> >>
> >> >> +
> >> >> + rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
> >> >> + if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
> >> >> + return -EPROTO;
> >> >> +
> >> >> + if (data_len > rx_buf_len)
> >> >> + data_len = rx_buf_len;
> >> >
> >> > You're still not checking that the received data does not overflow the
> >> > supplied buffer as I already commented on v3.
> >> >
> >> >> +
> >> >> + memcpy(data, dln2->buf.rx.buf, data_len);
> >> >> +
> >> >> + return data_len;
> >> >> +}
> >>
> >> Hmm, perhaps I am missing something, but we never transfer more then
> >> data_len, where data_len is the size of the buffer supplied by the
> >> user.
> >
> > That is the amount of data you request from the device, but you never
> > check how much is actually returned.
> >
>
> Actually we check the receive buffer size here:
>
> if (data_len > rx_buf_len)
> data_len = rx_buf_len;
>
> rx_buf_len is the i2c received payload size while rx_len is the length
> of received message
Bah, you're right. You never explicitly check for overflow, but also
never use more than data_len bytes of the returned buffer.
I think you should add explicit checks, and return an error in this case
rather than silently truncate the data.
> > You really should clean up the error handling of this function as it is
> > currently not very readable.
> >
>
> Perhaps adding some comments similar to the the explanation above would help?
It's more the logic of this function I find a bit twisted. I think you
should clean it up and consider adding appropriately named (temporary)
variables to make it more readable.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Wed, Sep 17, 2014 at 01:07:51PM +0300, Octavian Purdila wrote:
> On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold wrote:
>
>
>
> >> + /*
> >> + * 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.
> >> + */
> >> + union {
> >> + struct {
> >> + u8 port;
> >> + u8 addr;
> >> + u8 mem_addr_len;
> >> + __le32 mem_addr;
> >> + __le16 buf_len;
> >> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> + } __packed tx;
> >> + struct {
> >> + __le16 buf_len;
> >> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> + } __packed rx;
> >> + } buf;
> >
> > While this works in this case due to the extra copy you do in
> > dln2_transfer, allocating buffers that would (generally) be used for DMA
> > transfers as part of a larger structure is a recipe for trouble.
> >
> > It's probably better to allocate separately, if only to prevent people
> > from thinking there might be a bug here.
> >
>
> Just to make sure I understand this, what could the issues be? The
> buffers not being aligned or not allocated in continuous physical
> memory?
Yes, the buffer (and any subsequent field) would have to be cache-line
aligned to avoid corruption due to cache-line sharing on some systems.
>
>
> >> +
> >> + rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
> >> + if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
> >> + return -EPROTO;
> >> +
> >> + if (data_len > rx_buf_len)
> >> + data_len = rx_buf_len;
> >
> > You're still not checking that the received data does not overflow the
> > supplied buffer as I already commented on v3.
> >
> >> +
> >> + memcpy(data, dln2->buf.rx.buf, data_len);
> >> +
> >> + return data_len;
> >> +}
>
> Hmm, perhaps I am missing something, but we never transfer more then
> data_len, where data_len is the size of the buffer supplied by the
> user.
That is the amount of data you request from the device, but you never
check how much is actually returned.
You really should clean up the error handling of this function as it is
currently not very readable.
>
>
> >> +
> >> + platform_set_drvdata(pdev, dln2);
> >> +
> >> + ret = device_create_file(dev, &dev_attr_freq);
> >> + if (ret < 0) {
> >> + dev_err(dev, "failed to add freq attribute\n");
> >> + return ret;
> >> + }
> >
> > There are a couple of problems here. First, you should not make this an
> > attribute of the platform device, which is created before any driver is
> > bound (might not ever happen).
> >
> > Instead add the attribute to the i2c adapter below. However, you need to
> > do this using device attribute groups to avoid racing with userspace (as
> > you are when using device_create_file after the device itself has been
> > created).
> >
> > You should probably also make your attribute name less generic by adding
> > a "dln2_"-prefix.
>
> Thanks for the detailed review and explanations, as always :)
You're welcome.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Tue, Sep 09, 2014 at 10:24:45PM +0300, Octavian Purdila wrote:
> From: Laurentiu Palcu
>
> 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
> Signed-off-by: Octavian Purdila
> ---
> drivers/i2c/busses/Kconfig| 10 ++
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-dln2.c | 390
> ++
> 3 files changed, 401 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-dln2.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..6afc17e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1021,4 +1021,14 @@ config SCx200_ACB
> This support is also available as a module. If so, the module
> will be called scx200_acb.
>
> +config I2C_DLN2
> + tristate "Diolan DLN-2 USB I2C adapter"
> + depends on MFD_DLN2
> + help
> + If you say yes to this option, support will be included for Diolan
> + DLN2, a USB to I2C interface.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-dln2.
> +
> endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..3118fea 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
> obj-$(CONFIG_I2C_PCA_ISA)+= i2c-pca-isa.o
> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> +obj-$(CONFIG_I2C_DLN2) += i2c-dln2.o
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
> new file mode 100644
> index 000..ac09ec4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-dln2.c
> @@ -0,0 +1,390 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-I2C adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + * i2c-diolan-u2c.c
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "dln2-i2c"
> +
> +#define DLN2_I2C_MODULE_ID 0x03
> +#define DLN2_I2C_CMD(cmd)DLN2_CMD(cmd, DLN2_I2C_MODULE_ID)
> +
> +/* I2C commands */
> +#define DLN2_I2C_GET_PORT_COUNT DLN2_I2C_CMD(0x00)
> +#define DLN2_I2C_ENABLE DLN2_I2C_CMD(0x01)
> +#define DLN2_I2C_DISABLE DLN2_I2C_CMD(0x02)
> +#define DLN2_I2C_IS_ENABLED DLN2_I2C_CMD(0x03)
> +#define DLN2_I2C_SET_FREQUENCY DLN2_I2C_CMD(0x04)
> +#define DLN2_I2C_GET_FREQUENCY DLN2_I2C_CMD(0x05)
> +#define DLN2_I2C_WRITE DLN2_I2C_CMD(0x06)
> +#define DLN2_I2C_READDLN2_I2C_CMD(0x07)
> +#define DLN2_I2C_SCAN_DEVICESDLN2_I2C_CMD(0x08)
> +#define DLN2_I2C_PULLUP_ENABLE DLN2_I2C_CMD(0x09)
> +#define DLN2_I2C_PULLUP_DISABLE DLN2_I2C_CMD(0x0A)
> +#define DLN2_I2C_PULLUP_IS_ENABLED DLN2_I2C_CMD(0x0B)
> +#define DLN2_I2C_TRANSFERDLN2_I2C_CMD(0x0C)
> +#define DLN2_I2C_SET_MAX_REPLY_COUNT DLN2_I2C_CMD(0x0D)
> +#define DLN2_I2C_GET_MAX_REPLY_COUNT DLN2_I2C_CMD(0x0E)
> +#define DLN2_I2C_GET_MIN_FREQUENCY DLN2_I2C_CMD(0x40)
> +#define DLN2_I2C_GET_MAX_FREQUENCY DLN2_I2C_CMD(0x41)
> +
> +#define DLN2_I2C_FREQ_STD10
> +
> +#define DLN2_I2C_MAX_XFER_SIZE 256
> +
> +struct dln2_i2c {
> + struct platform_device *pdev;
> + struct i2c_adapter adapter;
> + uint32_t freq;
> + uint32_t min_freq;
> + uint32_t max_freq;
Please use u32 throughout for consistency.
> + /*
> + * 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.
> + */
> + union {
> + struct {
> + u8 port;
> + u8 addr;
> + u8 mem_addr_len;
> + __le32 mem_addr;
> + __le16 buf_len;
> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> + } __packed tx;
> + struct {
> + __le16 buf_len;
> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> + } __packed rx;
> + } buf;
Wh
Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/dln2.c | 681
> +++
> include/linux/mfd/dln2.h | 71 +
> 4 files changed, 762 insertions(+)
> create mode 100644 drivers/mfd/dln2.c
> create mode 100644 include/linux/mfd/dln2.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..7bcf895 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -183,6 +183,15 @@ config MFD_DA9063
> Additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_DLN2
> + tristate "Diolan DLN2 support"
> + select MFD_CORE
> + depends on USB
> + help
> + This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter DLN-2.
> + Additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> config MFD_MC13XXX
> tristate
> depends on (SPI_MASTER || I2C)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..591988d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
> obj-$(CONFIG_MFD_AS3722) += as3722.o
> obj-$(CONFIG_MFD_STW481X)+= stw481x.o
> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
> +obj-$(CONFIG_MFD_DLN2) += dln2.o
>
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> new file mode 100644
> index 000..b551b5e
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
> @@ -0,0 +1,681 @@
> +/*
> + * Driver for the Diolan DLN-2 USB adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + * i2c-diolan-u2c.c
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "usb-dln2"
> +
> +struct dln2_header {
> + __le16 size;
> + __le16 id;
> + __le16 echo;
> + __le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> + struct dln2_header hdr;
> + __le16 result;
> +} __packed;
> +
> +#define DLN2_GENERIC_MODULE_ID 0x00
> +#define DLN2_GENERIC_CMD(cmd)DLN2_CMD(cmd,
> DLN2_GENERIC_MODULE_ID)
> +#define CMD_GET_DEVICE_VER DLN2_GENERIC_CMD(0x30)
> +#define CMD_GET_DEVICE_SNDLN2_GENERIC_CMD(0x31)
> +
> +#define DLN2_HW_ID 0x200
> +#define DLN2_USB_TIMEOUT 200 /* in ms */
> +#define DLN2_MAX_RX_SLOTS16
> +#define DLN2_MAX_MODULES 5
Reduce to 4 until you implement support for more modules and save some
memory meanwhile? (Or is id 4 already used?)
> +#define DLN2_MAX_URBS16
> +#define DLN2_RX_BUF_SIZE 512
> +
> +#define DLN2_HANDLE_EVENT0
> +#define DLN2_HANDLE_CTRL 1
> +#define DLN2_HANDLE_GPIO 2
> +#define DLN2_HANDLE_I2C 3
> +
> +/*
> + * Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data.
> + */
> +struct dln2_rx_context {
> + struct completion done;
> + struct urb *urb;
> + bool connected;
> +};
> +
> +/*
> + *
Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices
On Wed, Sep 17, 2014 at 10:25:18AM +0300, Octavian Purdila wrote: > On Wed, Sep 17, 2014 at 2:21 AM, Lee Jones wrote: > > > > On Tue, 09 Sep 2014, Octavian Purdila wrote: > > > > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > > > Master Adapter DLN-2. Details about the device can be found here: > > > > > > https://www.diolan.com/i2c/i2c_interface.html. > > > > > > Information about the USB protocol can be found in the Programmer's > > > Reference Manual [1], see section 1.7. > > > > This driver really needs a USB Ack before I can accept it. > > > > Greg, Johan, is the driver acceptable now? I started looking through v4 yesterday and found a few more things. Will send you some you comments shortly. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Fri, Sep 05, 2014 at 07:04:51PM +0300, Octavian Purdila wrote: > On Fri, Sep 5, 2014 at 6:38 PM, Johan Hovold wrote: > > On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote: > > In general, how well have these patches been tested with disconnect > > events? At least gpiolib is known to blow up (sooner or later) when a > > gpiochip is removed when having requested gpios. > > I do disconnect tests regularly. Since switching to the new irq > interface the following patch is needed: > > https://lkml.org/lkml/2014/9/5/408 > > With it and the current patch sets things seems to work well. I see no comments from Linus W on that patch? And I can confirm that things do blow up. After disconnecting while having a gpio exported, I get the familiar OOPS below when reconnecting the device. This has also been reported here: https://lkml.org/lkml/2014/8/4/303 [ 711.232574] gpiochip_find_base: found new base at 234 [ 711.232696] Unable to handle kernel NULL pointer dereference at virtual address 0030 [ 711.232727] pgd = c0004000 [ 711.232757] [0030] *pgd= [ 711.232849] Internal error: Oops: 17 [#1] PREEMPT ARM [ 711.232879] Modules linked in: i2c_dln2 gpio_dln2 dln2 netconsole [last unloaded: i2c_dln2] [ 711.233032] CPU: 0 PID: 16 Comm: khubd Tainted: GW 3.17.0-rc3 #1 [ 711.233093] task: df2b5480 ti: df2b6000 task.ti: df2b6000 [ 711.233123] PC is at gpiochip_add+0x7c/0x378 [ 711.233184] LR is at vprintk_emit+0x284/0x628 [ 711.233215] pc : []lr : []psr: 000f0093 [ 711.233215] sp : df2b7900 ip : df2b77f8 fp : df2b792c [ 711.233276] r10: df4dd800 r9 : ffe0 r8 : 00ea [ 711.233306] r7 : c06ba8b0 r6 : a00f0013 r5 : c06ba8d0 r4 : df452804 [ 711.27] r3 : r2 : r1 : 00ec r0 : 00ea [ 711.233367] Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel [ 711.233398] Control: 10c5387d Table: 9f4c0019 DAC: 0015 [ 711.233459] Process khubd (pid: 16, stack limit = 0xdf2b6240) [ 711.233551] Stack: (0xdf2b7900 to 0xdf2b8000) [ 711.233581] 7900: c0055f6c df452e08 0020 df4dd810 df452800 bf033640 ffe0 [ 711.233642] 7920: df2b795c df2b7930 bf0334b0 c023ae78 bf0332e8 df4dd810 bf033978 c06de2c8 [ 711.233673] 7940: bf033978 c06c081c 000e df2b7974 df2b7960 c02893e0 bf0332f4 [ 711.233703] 7960: c0eb2cf0 df4dd810 df2b79ac df2b7978 c0287314 c02893b0 df2b799c c02895f4 [ 711.233764] 7980: df2b79ac bf033978 df4dd810 c0287574 df4acc20 c06c081c df41f480 [ 711.233795] 79a0: df2b79c4 df2b79b0 c02875c4 c02871d4 df4dd810 df2b79ec df2b79c8 [ 711.233856] 79c0: c02853e8 c0287580 df0414d8 df4cf094 df4acc20 df4dd810 df4dd844 c06bded0 [ 711.233886] 79e0: df2b7a0c df2b79f0 c0287154 c028538c df041400 df4dd818 df4dd810 c06bded0 [ 711.233917] 7a00: df2b7a2c df2b7a10 c02865c0 c02870d8 df2b5480 df4dd818 df4dd810 [ 711.233978] 7a20: df2b7a64 df2b7a30 c02844fc c0286534 df2b7a58 df2b7a40 c0282b8c c02160a0 [ 711.234008] 7a40: df4dd800 df4dd810 0010 bf02ec18 df2b7a84 df2b7a68 [ 711.234069] 7a60: c02890c8 c02840a4 df4acc20 df4dd800 0010 df2b7ac4 df2b7a88 [ 711.234100] 7a80: c02a571c c0288ffc c010c048 dfc0 df4dd810 00d0 df41f484 [ 711.234130] 7aa0: bf02ec54 df4acc20 0002 df2b7b0c df2b7ac8 [ 711.234191] 7ac0: c02a5918 c02a54ec 0040 bf02e380 df41f480 [ 711.234222] 7ae0: df4acc20 df4e4000 0040 bf02e380 df4acc00 df4acc20 df4e4040 [ 711.234283] 7b00: df2b7b54 df2b7b10 bf02e7bc c02a586c df4dbac8 [ 711.234313] 7b20: df2b7b3c df4acc00 c0430dc8 df41a868 df41a800 bf02efc0 df4acc20 df4dbac8 [ 711.234344] 7b40: df2b7b8c df2b7b58 c030bc38 bf02e604 c016f220 df4acc00 [ 711.234405] 7b60: df2b7b8c c0eb2cf0 df4acc20 c06de2c8 bf02efc0 c06c6c88 000e [ 711.234436] 7b80: df2b7bc4 df2b7b90 c0287314 c030ba88 df4acc00 c0287574 df41a868 bf02efc0 [ 711.234497] 7ba0: df4acc20 c0287574 df41a868 c06c6c88 0001 df2b7bdc df2b7bc8 [ 711.234527] 7bc0: c02875c4 c02871d4 df4acc20 df2b7c04 df2b7be0 c02853e8 c0287580 [ 711.234588] 7be0: df2936d8 df4cf194 df41a868 df4acc20 df4acc54 c06c6ca0 df2b7c24 df2b7c08 [ 711.234619] 7c00: c0287154 c028538c df293600 df4acc28 df4acc20 c06c6ca0 df2b7c44 df2b7c28 [ 711.234649] 7c20: c02865c0 c02870d8 df2b5480 df4acc28 df4acc20 df2b7c7c df2b7c48 [ 711.234710] 7c40: c02844fc c0286534 df2b7c64 df2b7c58 c0430dc8 c0309a04 c06e0d88 [ 711.234741] 7c60: df41a868 df4acc00 df41a800 df407650 df2b7d04 df2b7c80 c0309a74 c02840a4 [ 711.234802] 7c80: 0001 1388 df499090 df2b7cbc c016f194 [ 711.234832] 7ca0: c06d7dbd df347000 0001 df477b40 df41a804 0001 df407600 c06c6e3c [ 711.234924] 7cc0: c06c6ca0 c03087f8 00
Re: [PATCH v3 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Mon, Sep 08, 2014 at 08:15:07PM +0300, Octavian Purdila wrote: > Just one more question on this subject: is the following allowed: > > int ret, len; > > or should it be: > > int ret; > int len; I try to avoid it, at least unless obviously related such as min/max or x/y. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote: > --- /dev/null > +++ b/drivers/gpio/gpio-dln2.c > @@ -0,0 +1,537 @@ > +/* > + * Driver for the Diolan DLN-2 USB-GPIO adapter > + * > + * Copyright (c) 2014 Intel Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation, version 2. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include It seems you don't need all these includes (usb, ptrace, wait...). Only include what you actually use. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Mon, Sep 08, 2014 at 06:57:29PM +0300, Octavian Purdila wrote:
> On Mon, Sep 8, 2014 at 5:44 PM, Johan Hovold wrote:
>
>
>
> Hi Johan,
>
> Again, thanks for the detailed review, I am addressing your review
> comments as we speak. Some questions below.
>
>
>
> > > + int ret, len;
> > > + struct tx_data {
> > > + u8 port;
> > > + u8 addr;
> > > + u8 mem_addr_len;
> > > + __le32 mem_addr;
> > > + __le16 buf_len;
> > > + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> > > + } __packed tx;
> >
> > Allocate these buffers dynamically (possibly at probe).
> >
>
> I double checked this, and DLN2_I2C_MAX_XFER_SIZE should actually be <
> 64 as the USB endpoint configuration max packet size is 64. In this
> case, can we keep it on the stack?
It's better to lift that restriction and allocate it dynamically. Using
larger buffers (> EP size) is also more efficient.
>
>
> > > + int ret, buf_len, rx_len = sizeof(rx);
> >
> > Again, one declaration per line.
>
> AFAICS there are many places where declaration on the same line
> (initialization included) are used. When did this became a coding
> style issue?
It's ugly, hurts readability, and can also obfuscate the fact that your
function really needs to be refactored.
And it's in the CodingStyle:
"To this end, use just one data declaration per line (no commas
for multiple data declarations)."
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:
> From: Daniel Baluta
>
> This patch adds GPIO and IRQ support for the Diolan DLN-2 GPIO module.
>
> Information about the USB protocol interface can be found in the
> Programmer's Reference Manual [1], see section 2.9 for the GPIO
> module commands and responses.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Daniel Baluta
> Signed-off-by: Octavian Purdila
> ---
> drivers/gpio/Kconfig | 12 ++
> drivers/gpio/Makefile| 1 +
> drivers/gpio/gpio-dln2.c | 537
> +++
> 3 files changed, 550 insertions(+)
> create mode 100644 drivers/gpio/gpio-dln2.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9de1515..6a9e352 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -897,4 +897,16 @@ config GPIO_VIPERBOARD
>River Tech's viperboard.h for detailed meaning
>of the module parameters.
>
> +config GPIO_DLN2
> + tristate "Diolan DLN2 GPIO support"
> + depends on USB && MFD_DLN2
Just MFD_DLN2.
> + select GPIOLIB_IRQCHIP
> +
> + help
> + Select this option to enable GPIO driver for the Diolan DLN2
> + board.
> +
> + This driver can also be built as a module. If so, the module
> + will be called gpio-dln2.
> +
> endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d024e3..eaa97a0 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_DA9052)+= gpio-da9052.o
> obj-$(CONFIG_GPIO_DA9055)+= gpio-da9055.o
> obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o
> +obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o
> obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o
> obj-$(CONFIG_GPIO_EM)+= gpio-em.o
> obj-$(CONFIG_GPIO_EP93XX)+= gpio-ep93xx.o
> diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
> new file mode 100644
> index 000..f8c0bcb
> --- /dev/null
> +++ b/drivers/gpio/gpio-dln2.c
> @@ -0,0 +1,537 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-GPIO adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "dln2-gpio"
> +
> +#define DLN2_GPIO_ID 0x01
> +
> +#define DLN2_GPIO_GET_PORT_COUNT DLN2_CMD(0x00, DLN2_GPIO_ID)
> +#define DLN2_GPIO_GET_PIN_COUNT DLN2_CMD(0x01, DLN2_GPIO_ID)
> +#define DLN2_GPIO_SET_DEBOUNCE DLN2_CMD(0x04, DLN2_GPIO_ID)
> +#define DLN2_GPIO_GET_DEBOUNCE DLN2_CMD(0x05, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PORT_GET_VAL DLN2_CMD(0x06, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_VALDLN2_CMD(0x0B, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_OUT_VALDLN2_CMD(0x0C, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_OUT_VALDLN2_CMD(0x0D, DLN2_GPIO_ID)
> +#define DLN2_GPIO_CONDITION_MET_EV DLN2_CMD(0x0F, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_ENABLE DLN2_CMD(0x10, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_DISABLEDLN2_CMD(0x11, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_DIRECTION DLN2_CMD(0x13, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_DIRECTION DLN2_CMD(0x14, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_EVENT_CFG DLN2_CMD(0x1E, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_EVENT_CFG DLN2_CMD(0x1F, DLN2_GPIO_ID)
> +
> +#define DLN2_GPIO_EVENT_NONE 0
> +#define DLN2_GPIO_EVENT_CHANGE 1
> +#define DLN2_GPIO_EVENT_LVL_HIGH 2
> +#define DLN2_GPIO_EVENT_LVL_LOW 3
> +#define DLN2_GPIO_EVENT_CHANGE_RISING0x11
> +#define DLN2_GPIO_EVENT_CHANGE_FALLING 0x21
> +#define DLN2_GPIO_EVENT_MASK 0x0F
> +
> +#define DLN2_GPIO_MAX_PINS 32
> +
> +struct dln2_irq_work {
> + struct work_struct work;
> + struct dln2_gpio *dln2;
> + int pin, type;
One declaration per line.
Please consider my previous style comments also for this patch.
> +};
> +
> +struct dln2_remove_work {
> + struct delayed_work work;
> + struct dln2_gpio *dln2;
> +};
> +
> +struct dln2_gpio {
> + struct platform_device *pdev;
> + struct gpio_chip gpio;
> +
> + DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
> + DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
> + DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
> + struct dln2_irq_work irq_work[DLN2_GPIO_MAX_PINS];
> + struct dln2_remove_work remove_work;
> +};
> +
> +struct dln2_gpio_pin {
> + __le16 pin;
> +} __packe
Re: [PATCH v3 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Fri, Sep 05, 2014 at 06:17:58PM +0300, Octavian Purdila wrote:
> From: Laurentiu Palcu
>
> 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
> Signed-off-by: Octavian Purdila
> ---
> drivers/i2c/busses/Kconfig| 10 ++
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-dln2.c | 301
> ++
> 3 files changed, 312 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-dln2.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..4873161 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1021,4 +1021,14 @@ config SCx200_ACB
> This support is also available as a module. If so, the module
> will be called scx200_acb.
>
> +config I2C_DLN2
> + tristate "Diolan DLN-2 USB I2C adapter"
> + depends on USB && MFD_DLN2
MFD_DLN2 should be sufficient.
> + help
> + If you say yes to this option, support will be included for Diolan
> + DLN2, a USB to I2C interface.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-dln2.
> +
> endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..3118fea 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
> obj-$(CONFIG_I2C_PCA_ISA)+= i2c-pca-isa.o
> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> +obj-$(CONFIG_I2C_DLN2) += i2c-dln2.o
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
> new file mode 100644
> index 000..93e85ff
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-dln2.c
> @@ -0,0 +1,301 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-I2C adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + * i2c-diolan-u2c.c
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
No newline.
> +#include
> +#include
> +
> +#define DRIVER_NAME "dln2-i2c"
> +
> +#define DLN2_I2C_MODULE_ID 0x03
> +#define DLN2_I2C_CMD(cmd)DLN2_CMD(cmd, DLN2_I2C_MODULE_ID)
> +
> +/* I2C commands */
> +#define DLN2_I2C_GET_PORT_COUNT DLN2_I2C_CMD(0x00)
> +#define DLN2_I2C_ENABLE DLN2_I2C_CMD(0x01)
> +#define DLN2_I2C_DISABLE DLN2_I2C_CMD(0x02)
> +#define DLN2_I2C_IS_ENABLED DLN2_I2C_CMD(0x03)
> +#define DLN2_I2C_SET_FREQUENCY DLN2_I2C_CMD(0x04)
> +#define DLN2_I2C_GET_FREQUENCY DLN2_I2C_CMD(0x05)
> +#define DLN2_I2C_WRITE DLN2_I2C_CMD(0x06)
> +#define DLN2_I2C_READDLN2_I2C_CMD(0x07)
> +#define DLN2_I2C_SCAN_DEVICESDLN2_I2C_CMD(0x08)
> +#define DLN2_I2C_PULLUP_ENABLE DLN2_I2C_CMD(0x09)
> +#define DLN2_I2C_PULLUP_DISABLE DLN2_I2C_CMD(0x0A)
> +#define DLN2_I2C_PULLUP_IS_ENABLED DLN2_I2C_CMD(0x0B)
> +#define DLN2_I2C_TRANSFERDLN2_I2C_CMD(0x0C)
> +#define DLN2_I2C_SET_MAX_REPLY_COUNT DLN2_I2C_CMD(0x0D)
> +#define DLN2_I2C_GET_MAX_REPLY_COUNT DLN2_I2C_CMD(0x0E)
> +#define DLN2_I2C_GET_MIN_FREQUENCY DLN2_I2C_CMD(0x40)
> +#define DLN2_I2C_GET_MAX_FREQUENCY DLN2_I2C_CMD(0x41)
> +
> +#define DLN2_I2C_FREQ_FAST 40
> +#define DLN2_I2C_FREQ_STD10
> +
> +#define DLN2_I2C_MAX_XFER_SIZE 256
> +
> +struct dln2_i2c {
> + struct platform_device *pdev;
> + struct i2c_adapter adapter;
> +};
> +
> +static uint frequency = DLN2_I2C_FREQ_STD; /* I2C clock frequency in Hz */
> +
> +module_param(frequency, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(frequency, "I2C clock frequency in hertz");
These seems like a very bad idea. Why set one common frequency for all
connected USB-I2C devices using a module parameter? That might have made
sense a long time ago with embedded i2c-controller, but certainly does
not with usb-i2c controllers.
This should probably be set through sysfs on a per-device basis.
> +
> +static int dln2_i2c_set_state(struct dln2_i2c *dln2, u8 state)
> +{
> + int ret;
> + u8 port = 0;
So these devices can apparently have more than one i2
Re: [PATCH v3 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 08, 2014 at 04:20:34PM +0300, Octavian Purdila wrote:
> On Mon, Sep 8, 2014 at 2:32 PM, Johan Hovold wrote:
> > > +static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&rxs->lock, flags);
> > > +
> > > + *slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
> > > +
> > > + if (*slot < DLN2_MAX_RX_SLOTS) {
> > > + struct dln2_rx_context *rxc = &rxs->slots[*slot];
> > > +
> > > + init_completion(&rxc->done);
> >
> > Initialise the completions when you allocate them (and there's no need
> > to re-init here).
>
> You are right, but I think we need a reinit_completion(). Technically
> we don't, as we don't use complete_all(), but I feel it is cleaner
> that way. I will move the initialization in probe and add the
> reinit_completion() in free_rx_slot. Is that OK with you?
Sure, that's fine.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 08, 2014 at 01:32:33PM +0200, Johan Hovold wrote:
> On Fri, Sep 05, 2014 at 06:17:57PM +0300, Octavian Purdila wrote:
> > +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> > + void *obuf, int obuf_len, void *ibuf, int *ibuf_len)
> > +{
> > + /* if we got here we know that the response header has been checked */
> > + rsp = rxc->urb->transfer_buffer;
> > + result = le16_to_cpu(rsp->result);
>
> Yes, but you haven't verified that rsp->hdr.size > 0, so you may still
> be reading stale data.
I meant that you haven't verified that the payload size > 1 (the header
size is included in rsp->hdr.size and result is two byte wide).
> > +
> > + if (result) {
> > + dev_dbg(dev, "%d received response with error %d\n",
> > + handle, result);
> > + ret = -EREMOTEIO;
> > + goto out_free_rx_slot;
> > + }
> > +
> > + if (!ibuf) {
> > + ret = 0;
> > + goto out_free_rx_slot;
> > + }
> > +
> > + if (*ibuf_len > rxc->urb->actual_length - sizeof(*rsp))
> > + *ibuf_len = rxc->urb->actual_length - sizeof(*rsp);
>
> And then you get an underflow here, although that doesn't seem to cause
> any troubles in this case.
Unless ibuf_len is -1...
> But why isn't ibuf_len unsigned?
>
> > +
> > + memcpy(ibuf, rsp + 1, *ibuf_len);
> > +
> > +out_free_rx_slot:
> > + free_rx_slot(dln2, rxs, rx_slot);
> > +
> > + return ret;
> > +}
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] mfd: add support for Diolan DLN-2 devices
On Fri, Sep 05, 2014 at 06:17:57PM +0300, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/dln2.c | 653
> +++
> include/linux/mfd/dln2.h | 61 +
> 4 files changed, 724 insertions(+)
> create mode 100644 drivers/mfd/dln2.c
> create mode 100644 include/linux/mfd/dln2.h
First of all, this is much cleaner than the first non-MFD version. I'll
have a look at the subdrivers shortly as well.
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..7bcf895 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -183,6 +183,15 @@ config MFD_DA9063
> Additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_DLN2
> + tristate "Diolan DLN2 support"
> + select MFD_CORE
> + depends on USB
> + help
> + This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter DLN-2.
> + Additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> config MFD_MC13XXX
> tristate
> depends on (SPI_MASTER || I2C)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..591988d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
> obj-$(CONFIG_MFD_AS3722) += as3722.o
> obj-$(CONFIG_MFD_STW481X)+= stw481x.o
> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
> +obj-$(CONFIG_MFD_DLN2) += dln2.o
>
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> new file mode 100644
> index 000..81ff58e
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
> @@ -0,0 +1,653 @@
> +/*
> + * Driver for the Diolan DLN-2 USB adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + * i2c-diolan-u2c.c
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "usb-dln2"
> +
> +struct dln2_header {
> + __le16 size;
> + __le16 id;
> + __le16 echo;
> + __le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> + struct dln2_header hdr;
> + __le16 result;
> +} __packed;
> +
> +#define DLN2_GENERIC_MODULE_ID 0x00
> +#define DLN2_GENERIC_CMD(cmd)DLN2_CMD(cmd,
> DLN2_GENERIC_MODULE_ID)
> +#define CMD_GET_DEVICE_VER DLN2_GENERIC_CMD(0x30)
> +#define CMD_GET_DEVICE_SNDLN2_GENERIC_CMD(0x31)
> +
> +#define DLN2_HW_ID 0x200
> +#define DLN2_USB_TIMEOUT 200 /* in ms */
> +#define DLN2_MAX_RX_SLOTS16
> +#define DLN2_MAX_MODULES 5
> +#define DLN2_MAX_URBS16
> +
> +#define DLN2_HANDLE_GPIO_EVENT 0
Just DLN2_HANDLE_EVENT?
> +#define DLN2_HANDLE_CTRL 1
> +#define DLN2_HANDLE_GPIO 2
> +#define DLN2_HANDLE_I2C 3
> +
> +/* Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data. */
The recommended style for multi-line comments is
/*
* ...
*/
This applies to all three patches.
> +struct dln2_rx_cont
Re: [PATCH v3 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:
> +static int dln2_do_remove(struct dln2_gpio *dln2)
> +{
> + /* When removing the DLN2 USB device, gpiochip_remove may fail
> + * due to i2c drivers holding a GPIO pin. However, the i2c bus
> + * will eventually be removed triggering an i2c driver remove
> + * which will release the GPIO pin. So retrying the operation
> + * later should succeed. */
> + int ret = gpiochip_remove(&dln2->gpio);
> + struct device *dev = dln2->gpio.dev;
> +
> + if (ret < 0) {
> + if (ret == -EBUSY)
> + schedule_delayed_work(&dln2->remove_work.work, HZ/10);
> + else
> + dev_warn(dev, "error removing gpio chip: %d\n", ret);
> + } else {
> + kfree(dln2);
> + }
> +
> + return ret;
> +}
Apparently, the return value from gpiochip_remove is going away:
"Start to kill off the return value from gpiochip_remove() by
removing the __must_check attribute and removing all checks
inside the drivers/gpio directory. The rationale is: well what
were we supposed to do if there is an error code? Not much:
print an error message. And gpiolib already does that. So make
this function return void eventually."
https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg03468.html
Also, have you considered what happens if there are gpios exported
through sysfs? These may never be released.
In general, how well have these patches been tested with disconnect
events? At least gpiolib is known to blow up (sooner or later) when a
gpiochip is removed when having requested gpios.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Wed, Sep 03, 2014 at 04:39:48PM +0300, Octavian Purdila wrote: > On Tue, Sep 2, 2014 at 6:23 PM, Johan Hovold wrote: > > That should be possible using the regmap bus read and write operations. > > I took a closer look on the regmap bus read/write operations and I > think they are not fit for what we need in the driver. The driver uses > a request/response model which, IMHO, does not fit well with a > register read/write API. Yes, maybe we can emulate it, but why do > that? > > >> (Also creating a regmap class for a particular device seems over > >> engineering since nobody else is going to use it) > > > > Possibly, but it would allow subdrivers to be implemented using a > > standard interface and also provide register caching for free. > > Using a standard interface is nice, but I think that using the right > interface type is more important. This hardware does not use registers > but a messages to communicate with the OS. You might be right, and as I mentioned, I haven't looked that closely at the protocol yet. I'll take a look at your updated I/O interface and how you use it. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 02, 2014 at 11:45:55AM +0300, Octavian Purdila wrote: > On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones wrote: > > On Mon, 01 Sep 2014, Johan Hovold wrote: > >> I haven't looked at the details of the protocol for the device in > >> question, but it might even be possible to use regmap here (as I > >> mentioned in my comments on v1). > > > > Obviously that would be preferred. > > > > Octavian, did you look into that? > > > Yes, I did. Since this is the first time I am looking at regmap I may > be wrong but I don't see a way to use it. The dln2 i2c driver needs to > be able to send and receive arbitrary size buffers and this does not > seem possible to do with the regmap API. That should be possible using the regmap bus read and write operations. > (Also creating a regmap class for a particular device seems over > engineering since nobody else is going to use it) Possibly, but it would allow subdrivers to be implemented using a standard interface and also provide register caching for free. The event callbacks of the device in questions would not fit this scheme though, but perhaps only that part needs to be driver specific. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 02, 2014 at 09:00:10AM +0100, Lee Jones wrote: > On Mon, 01 Sep 2014, Johan Hovold wrote: > > No, no. USB is not a function of the MFD device, it's the transport. > > Thus there should be no USB MFD-cell. No subdriver can work without it. > > > > And the USB id belongs in the MFD-driver in the same way that an > > i2c id (address) does. > > > > Just like an MFD device with i2c as a transport, this driver would > > function as an arbiter to a shared resource (i.e. the register space). > > The reason it seems much more USB-centric than an i2c-mfd driver is that > > that transport API is simpler and some code have also already been > > generalised (e.g. regmap), whereas we appear to have only two USB > > mfd-drivers thus far. > > > > The viperboard is perhaps a bad example in so far that it has pushed the > > transport details down into the subdrivers (and thus into gpio, i2c and > > iio subsystems) instead of handling it one place. > > Thanks for your explanation. I take your point about the USB ID and I > did say I was guessing that the USB part should exist as a child > device. > > So after your comments I decided to do a little investigation. It > appears that this MFD driver is _just_ using the common API which all > other devices utilising USB comms are forced to use. Is that correct? Yes, it's using the low-level USB API, but there's a lot of higher-level interfaces in place for (fairly) standard things such as the USB class drivers or the USB serial subsystem. > If so, I have a question. Is there no way to hide more of the USB > specifics inside a better, simpler API? It looks like the drivers > which use USB are subjected to a lot (too much) of what might be > considered internals. Or is it just that the client has to tinker > with too many dials to get anything sensible out? *shudders* Unfortunately, anything that does not already have a driver is likely to use some vendor-specific protocol and therefore must use the low-level API. > > I haven't looked at the details of the protocol for the device in > > question, but it might even be possible to use regmap here (as I > > mentioned in my comments on v1). > > Obviously that would be preferred. Simple register-based USB MFD devices (e.g. only using control transfers) are conceivable though, and if we start seeing a lot of those (which I doubt) perhaps that part could be refactored as a regmap bus. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 01, 2014 at 07:22:39PM +0300, Octavian Purdila wrote: > On Mon, Sep 1, 2014 at 6:46 PM, Lee Jones wrote: > > On Mon, 01 Sep 2014, Octavian Purdila wrote: > > > >> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: > >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: > >> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones wrote: > >> >> > You should have a small MFD driver which controls resources and > >> >> > registers children. All other functionality should live in their > >> >> > respective drivers/X locations i.e. USB functionallity should normally > >> >> > live in drivers/usb. > >> >> > > >> >> > >> >> OK, that sounds better. I am not sure how to handle the registration > >> >> part though, since in this case we need to create the children at > >> >> runtime, from the usb probe routine. > >> >> > >> >> The only solution I see is to move the driver completely to > >> >> usb/drivers and continue to use the MFD infrastructure. Does that > >> >> sound OK to you? > >> > > >> > I have no problem with that. If this is an MFD driver, it _should_ > >> > live in drivers/mfd. However, all of that USB specific stuff > >> > defiantly should not. > >> > > >> > >> It is a multi-function driver which is using the USB interface, so I > >> am not sure where it belongs. The only driver that calls > >> mfd_add_devices and is not in drivers/mfd is the hid sensor hub > >> driver. > >> > >> BTW, the mfd/viperboard.c driver is very similar with this driver. It > >> has less USB specific stuff because the protocol is simpler, but still > >> has some. > > > > Looking at viperboard.c, it seems to use some basic generic framework > > calls to obtain some information about the device information like > > version numbers. Your driver is leaps and bounds more USB centric. > > > > Your MFD driver should know about things like; regmap, platform data, > > memory allocation, same-chip devices (children), etc. Your MFD driver > > should not need to concern itself with; endpoints, slots, URBs, USB > > device IDs and the like. The later knowledge belongs in drivers/usb. > > > > You should be calling mfd_add_devices() from within the MFD driver. > > At a guess, I would say that you need a new entry for the USB stuff in > > your mfd_cells structure. > > > > Makes sense, thanks for making clearing up what the MFD part of the > driver should do. > > Here is how I think it could work: > > * keep the usb probe routine in the MFD driver (and keep it a usb driver) > > * add a new cell for the usb part > > * pass the usb_interface via platform_data to the USB sub-driver's > platform_device probe routine and continue the USB setup there > > Lee, USB folks, is this acceptable? No, no. USB is not a function of the MFD device, it's the transport. Thus there should be no USB MFD-cell. No subdriver can work without it. And the USB id belongs in the MFD-driver in the same way that an i2c id (address) does. Just like an MFD device with i2c as a transport, this driver would function as an arbiter to a shared resource (i.e. the register space). The reason it seems much more USB-centric than an i2c-mfd driver is that that transport API is simpler and some code have also already been generalised (e.g. regmap), whereas we appear to have only two USB mfd-drivers thus far. The viperboard is perhaps a bad example in so far that it has pushed the transport details down into the subdrivers (and thus into gpio, i2c and iio subsystems) instead of handling it one place. I haven't looked at the details of the protocol for the device in question, but it might even be possible to use regmap here (as I mentioned in my comments on v1). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: add support for Diolan DLN-2 devices
On Wed, Aug 20, 2014 at 02:24:45PM +0300, Daniel Baluta wrote:
> From: Octavian Purdila
>
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
>
> https://www.diolan.com/i2c/i2c_interface.html.
>
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
>
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
>
> The functional DLN2 drivers (i2c, GPIO, etc.) will have to register
> themselves as DLN2 modules in order to send or receive data.
>
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
>
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also use an asynchronous
> mode of operation, in which case a receive callback function is going
> to be notified when messages for a specific handle are received.
>
> Because the hardware reserves handle 0 for GPIO events, the driver
> also reserves handle 0. It will be allocated to a DLN2 module only if
> it is explicitly requested.
>
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
>
> Signed-off-by: Octavian Purdila
> ---
> drivers/usb/misc/Kconfig | 6 +
> drivers/usb/misc/Makefile | 1 +
> drivers/usb/misc/dln2.c | 719
> ++
> include/linux/usb/dln2.h | 146 ++
> 4 files changed, 872 insertions(+)
> create mode 100644 drivers/usb/misc/dln2.c
> create mode 100644 include/linux/usb/dln2.h
>
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 76d7720..953f521 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -255,3 +255,9 @@ config USB_LINK_LAYER_TEST
> This driver is for generating specific traffic for Super Speed Link
> Layer Test Device. Say Y only when you want to conduct USB Super Speed
> Link Layer Test for host controllers.
> +
> +config USB_DLN2
> + tristate "Diolan DLN2 USB Driver"
> + help
> + This adds USB support for Diolan USB-I2C/SPI/GPIO
> + Master Adapter DLN-2.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 65b0402..767264e 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_USB_USS720)+= uss720.o
> obj-$(CONFIG_USB_SEVSEG) += usbsevseg.o
> obj-$(CONFIG_USB_YUREX) += yurex.o
> obj-$(CONFIG_USB_HSIC_USB3503) += usb3503.o
> +obj-$(CONFIG_USB_DLN2) += dln2.o
>
> obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
> obj-$(CONFIG_USB_LINK_LAYER_TEST)+= lvstest.o
> diff --git a/drivers/usb/misc/dln2.c b/drivers/usb/misc/dln2.c
> new file mode 100644
> index 000..5bfa850
> --- /dev/null
> +++ b/drivers/usb/misc/dln2.c
> @@ -0,0 +1,719 @@
> +/*
> + * Driver for the Diolan DLN-2 USB adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + * i2c-diolan-u2c.c
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "usb-dln2"
> +
> +#define DLN2_GENERIC_MODULE_ID 0x00
> +#define DLN2_GENERIC_CMD(cmd)DLN2_CMD(cmd,
> DLN2_GENERIC_MODULE_ID)
> +
> +/* generic commands */
> +#define CMD_GET_DEVICE_VER DLN2_GENERIC_CMD(0x30)
> +#define CMD_GET_DEVICE_SNDLN2_GENERIC_CMD(0x31)
Have you considered using regmap for register access?
> +
> +#define DLN2_HW_ID 0x200
> +
> +#define DLN2_USB_TIMEOUT 100 /* in ms */
> +
> +#define DLN2_MAX_RX_SLOTS 16
> +
> +#include
Move to the other includes.
> +
> +struct dln2_rx_context {
> + struct completion done;
> + struct urb *urb;
> + bool connected;
> +};
> +
> +struct dln2_rx_slots {
> + /* RX slots bitmap */
> + DECLARE_BITMAP(bmap, DLN2_MAX_RX_SLOTS);
You only have 16 slots and only do single bit manipulations. Just use an
unsigned long and find_first_bit, set_bit, etc, below.
> +
> + /* used to wait for a free RX slot */
> + wait_queue_head_t wq;
> +
> + /* used to wait for an RX operation to complete */
> + struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> +
> + /* avoid races between free_rx_slot and dln2_transfer_rx_cb */
> + spinlock_t lock;
> +};
> +
> +sta
