Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611145 > unrolled thread

[PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-03-28 18:10 +0200
    Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-28 19:10 +0200
      Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-03-29 16:20 +0200
        Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-29 16:50 +0200
          Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-03-30 14:50 +0200
        Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Rob Herring <robh@kernel.org> - 2017-03-29 23:30 +0200
          Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-03-30 14:50 +0200

#1611145 — [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write

Convert serdev_device_write_buf's code to be able to transfer amount of
data potentially exceeding "write room" at the moment of invocation.

To support that, also add serdev_device_write_wakeup.

Drivers wanting to use full extent of serdev_device_write
functionality are expected to provide serdev_device_write_wakeup as a
sole handler of .write_wakeup event or call it as a part of driver's
custom .write_wakeup code.

Drivers wanting to retain old serdev_device_write_buf behaviour can
replace those call to calls to serdev_device_write with timeout of
0. Providing .write_wakeup handler in such case is optional.

Cc: cphealy@gmail.com
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since v1 (see [v1]):

	- Make timeout to be a total(as opposed to per-iteration)
          timeout

	- Keep serdev_device_write_buf() as a wrapper arount
          serdev_device_write() for compatibility

[v1] https://lkml.org/lkml/2017/3/20/650

 drivers/tty/serdev/core.c | 36 +++++++++++++++++++++++++++++++-----
 include/linux/serdev.h    | 18 ++++++++++++++++--
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f4c6c90..c47c421 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
 }
 EXPORT_SYMBOL_GPL(serdev_device_close);
 
-int serdev_device_write_buf(struct serdev_device *serdev,
-			    const unsigned char *buf, size_t count)
+void serdev_device_write_wakeup(struct serdev_device *serdev)
+{
+	complete(&serdev->write_comp);
+}
+EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
+
+int serdev_device_write(struct serdev_device *serdev,
+			const unsigned char *buf, size_t count,
+			unsigned long timeout)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
+	int ret;
 
-	if (!ctrl || !ctrl->ops->write_buf)
+	if (!ctrl || !ctrl->ops->write_buf ||
+	    (timeout && !serdev->ops->write_wakeup))
 		return -EINVAL;
 
-	return ctrl->ops->write_buf(ctrl, buf, count);
+	mutex_lock(&serdev->write_lock);
+	do {
+		reinit_completion(&serdev->write_comp);
+
+		ret = ctrl->ops->write_buf(ctrl, buf, count);
+		if (ret < 0)
+			break;
+
+		buf   += ret;
+		count -= ret;
+
+	} while (count &&
+		 (timeout = wait_for_completion_timeout(&serdev->write_comp,
+							timeout)));
+	mutex_unlock(&serdev->write_lock);
+	return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
 }
-EXPORT_SYMBOL_GPL(serdev_device_write_buf);
+EXPORT_SYMBOL_GPL(serdev_device_write);
 
 void serdev_device_write_flush(struct serdev_device *serdev)
 {
@@ -232,6 +256,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
 	serdev->dev.parent = &ctrl->dev;
 	serdev->dev.bus = &serdev_bus_type;
 	serdev->dev.type = &serdev_device_type;
+	init_completion(&serdev->write_comp);
+	mutex_init(&serdev->write_lock);
 	return serdev;
 }
 EXPORT_SYMBOL_GPL(serdev_device_alloc);
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 5176cdc..535566d 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -39,12 +39,17 @@ struct serdev_device_ops {
  * @nr:		Device number on serdev bus.
  * @ctrl:	serdev controller managing this device.
  * @ops:	Device operations.
+ * @write_comp	Completion used by serdev_device_write internally
+ * @write_lock	Mutext used to esure exclusive access to the bus when
+ *		writing data with serdev_device_write()
  */
 struct serdev_device {
 	struct device dev;
 	int nr;
 	struct serdev_controller *ctrl;
 	const struct serdev_device_ops *ops;
+	struct completion write_comp;
+	struct mutex write_lock;
 };
 
 static inline struct serdev_device *to_serdev_device(struct device *d)
@@ -186,10 +191,12 @@ int serdev_device_open(struct serdev_device *);
 void serdev_device_close(struct serdev_device *);
 unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
 void serdev_device_set_flow_control(struct serdev_device *, bool);
-int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
+void serdev_device_write_wakeup(struct serdev_device *);
+int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, unsigned long);
 void serdev_device_write_flush(struct serdev_device *);
 int serdev_device_write_room(struct serdev_device *);
 
+
 /*
  * serdev device driver functions
  */
@@ -223,7 +230,8 @@ static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev
 	return 0;
 }
 static inline void serdev_device_set_flow_control(struct serdev_device *sdev, bool enable) {}
-static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsigned char *buf, size_t count)
+static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf,
+				      size_t count, unsigned long timeout)
 {
 	return -ENODEV;
 }
@@ -259,4 +267,10 @@ static inline struct device *serdev_tty_port_register(struct tty_port *port,
 static inline void serdev_tty_port_unregister(struct tty_port *port) {}
 #endif /* CONFIG_SERIAL_DEV_CTRL_TTYPORT */
 
+static inline int serdev_device_write_buf(struct serdev_device *serdev,
+					  const unsigned char *data, size_t count)
+{
+	return serdev_device_write(serdev, data, count, 0);
+}
+
 #endif /*_LINUX_SERDEV_H */
-- 
2.9.3

[toc] | [next] | [standalone]

#1611230

On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Convert serdev_device_write_buf's code to be able to transfer amount of
> data potentially exceeding "write room" at the moment of invocation.
>
> To support that, also add serdev_device_write_wakeup.
>
> Drivers wanting to use full extent of serdev_device_write
> functionality are expected to provide serdev_device_write_wakeup as a
> sole handler of .write_wakeup event or call it as a part of driver's
> custom .write_wakeup code.
>
> Drivers wanting to retain old serdev_device_write_buf behaviour can

> replace those call to calls to serdev_device_write with timeout of
> 0. Providing .write_wakeup handler in such case is optional.

Some indentation would be better if, for example, 0 will be kept on
previous line.

So, what I would see if no one objects is patch series of two:
1) introduction of new API
2) removing old one.

It will benefit for easier review and any potential code anthropologist.

> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_close);
>
> -int serdev_device_write_buf(struct serdev_device *serdev,
> -                           const unsigned char *buf, size_t count)
> +void serdev_device_write_wakeup(struct serdev_device *serdev)
> +{
> +       complete(&serdev->write_comp);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
> +
> +int serdev_device_write(struct serdev_device *serdev,
> +                       const unsigned char *buf, size_t count,
> +                       unsigned long timeout)
>  {
>         struct serdev_controller *ctrl = serdev->ctrl;
> +       int ret;
>
> -       if (!ctrl || !ctrl->ops->write_buf)
> +       if (!ctrl || !ctrl->ops->write_buf ||
> +           (timeout && !serdev->ops->write_wakeup))
>                 return -EINVAL;
>
> -       return ctrl->ops->write_buf(ctrl, buf, count);
> +       mutex_lock(&serdev->write_lock);
> +       do {
> +               reinit_completion(&serdev->write_comp);
> +
> +               ret = ctrl->ops->write_buf(ctrl, buf, count);
> +               if (ret < 0)
> +                       break;
> +

> +               buf   += ret;

Extra white spaces.

> +               count -= ret;
> +

> +       } while (count &&
> +                (timeout = wait_for_completion_timeout(&serdev->write_comp,
> +                                                       timeout)));

So, would it be better to support interrupts here and return a
corresponding error code to the user?

Besides that question, readability might be better if you use
temporary variable and pack above on one line:

unsigned long to = timeout;

} while (count && (to = ...(to)));



> +       mutex_unlock(&serdev->write_lock);
> +       return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
>  }
> -EXPORT_SYMBOL_GPL(serdev_device_write_buf);
> +EXPORT_SYMBOL_GPL(serdev_device_write);

> + * @write_comp Completion used by serdev_device_write internally

Links to the functions are func()-like. Please check kernel doc howto:s.

> + * @write_lock Mutext used to esure exclusive access to the bus when
> + *             writing data with serdev_device_write()

checkpatch.pl has integrated spellchecker AFAIU.
Moreover, can you try harder to make that description shorter?

>  void serdev_device_write_flush(struct serdev_device *);
>  int serdev_device_write_room(struct serdev_device *);
>
> +
>  /*
>   * serdev device driver functions
>   */

This doesn't belong to the change.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1611974

On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> Convert serdev_device_write_buf's code to be able to transfer amount of
>> data potentially exceeding "write room" at the moment of invocation.
>>
>> To support that, also add serdev_device_write_wakeup.
>>
>> Drivers wanting to use full extent of serdev_device_write
>> functionality are expected to provide serdev_device_write_wakeup as a
>> sole handler of .write_wakeup event or call it as a part of driver's
>> custom .write_wakeup code.
>>
>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>
>> replace those call to calls to serdev_device_write with timeout of
>> 0. Providing .write_wakeup handler in such case is optional.
>
> Some indentation would be better if, for example, 0 will be kept on
> previous line.
>

OK, sure.

> So, what I would see if no one objects is patch series of two:
> 1) introduction of new API
> 2) removing old one.
>
> It will benefit for easier review and any potential code anthropologist.
>

Second version of the patch preserves the old API an just
re-implements it in terms of a new one. I am not sure I see the
benefit in splitting it into two patches, but I'll leave it up to Rob
to decide.

>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>  }
>>  EXPORT_SYMBOL_GPL(serdev_device_close);
>>
>> -int serdev_device_write_buf(struct serdev_device *serdev,
>> -                           const unsigned char *buf, size_t count)
>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>> +{
>> +       complete(&serdev->write_comp);
>> +}
>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>> +
>> +int serdev_device_write(struct serdev_device *serdev,
>> +                       const unsigned char *buf, size_t count,
>> +                       unsigned long timeout)
>>  {
>>         struct serdev_controller *ctrl = serdev->ctrl;
>> +       int ret;
>>
>> -       if (!ctrl || !ctrl->ops->write_buf)
>> +       if (!ctrl || !ctrl->ops->write_buf ||
>> +           (timeout && !serdev->ops->write_wakeup))
>>                 return -EINVAL;
>>
>> -       return ctrl->ops->write_buf(ctrl, buf, count);
>> +       mutex_lock(&serdev->write_lock);
>> +       do {
>> +               reinit_completion(&serdev->write_comp);
>> +
>> +               ret = ctrl->ops->write_buf(ctrl, buf, count);
>> +               if (ret < 0)
>> +                       break;
>> +
>
>> +               buf   += ret;
>
> Extra white spaces.

Which is there on purpose to re-align "+=" with "-=" on the next line.
I'll remove it.

>
>> +               count -= ret;
>> +
>
>> +       } while (count &&
>> +                (timeout = wait_for_completion_timeout(&serdev->write_comp,
>> +                                                       timeout)));
>
> So, would it be better to support interrupts here and return a
> corresponding error code to the user?
>

I don't have a use-case for that and as far as I can tell, neither SPI
nor I2C slave device API offer such functionality universally, so I am
inclined to say no. Since the change from wait_for_completion to
wait_for_completion_timeout was made per Rob's request, I'd leave it
up to him to decided about this change as well.

> Besides that question, readability might be better if you use
> temporary variable and pack above on one line:
>
> unsigned long to = timeout;
>
> } while (count && (to = ...(to)));
>

Even if you shorten 'timeout' to 'to', formatted as a single line,
it'd still exceed line length limitations.

>
>
>> +       mutex_unlock(&serdev->write_lock);
>> +       return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
>>  }
>> -EXPORT_SYMBOL_GPL(serdev_device_write_buf);
>> +EXPORT_SYMBOL_GPL(serdev_device_write);
>
>> + * @write_comp Completion used by serdev_device_write internally
>
> Links to the functions are func()-like. Please check kernel doc howto:s.

OK, will do.

>
>> + * @write_lock Mutext used to esure exclusive access to the bus when
>> + *             writing data with serdev_device_write()
>
> checkpatch.pl has integrated spellchecker AFAIU.

My bad, forgot to enable it as a git hook, will fix.

> Moreover, can you try harder to make that description shorter?
>

I am all ears for suggestions alternative phrasing, otherwise, no,
that's about as hard as I try.

>>  void serdev_device_write_flush(struct serdev_device *);
>>  int serdev_device_write_room(struct serdev_device *);
>>
>> +
>>  /*
>>   * serdev device driver functions
>>   */
>
> This doesn't belong to the change.

Oops, didn't notice this. Will remove.

Thanks,
Andrey Smirnov

[toc] | [prev] | [next] | [standalone]

#1611993

On Wed, Mar 29, 2017 at 5:16 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:

>> So, what I would see if no one objects is patch series of two:
>> 1) introduction of new API
>> 2) removing old one.
>>
>> It will benefit for easier review and any potential code anthropologist.
>>
>
> Second version of the patch preserves the old API an just
> re-implements it in terms of a new one. I am not sure I see the
> benefit in splitting it into two patches, but I'll leave it up to Rob
> to decide.

Sure. At least I posted benefits I see from splitting.

+ bisectability (in case we have to revert your new API by some reason
it will be easier, hope will be not the case, though...)

>>> +       } while (count &&
>>> +                (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>> +                                                       timeout)));
>>
>> So, would it be better to support interrupts here and return a
>> corresponding error code to the user?
>>
>
> I don't have a use-case for that and as far as I can tell, neither SPI
> nor I2C slave device API offer such functionality universally, so I am
> inclined to say no. Since the change from wait_for_completion to
> wait_for_completion_timeout was made per Rob's request, I'd leave it
> up to him to decided about this change as well.

OK.

>> Besides that question, readability might be better if you use
>> temporary variable and pack above on one line:
>>
>> unsigned long to = timeout;
>>
>> } while (count && (to = ...(to)));
>>
>
> Even if you shorten 'timeout' to 'to', formatted as a single line,
> it'd still exceed line length limitations.

How many? If we are talking about 2-3 characters, that's okay to leave
them on one line.

>>> + * @write_lock Mutext used to esure exclusive access to the bus when
>>> + *             writing data with serdev_device_write()
>>
>> checkpatch.pl has integrated spellchecker AFAIU.
>
> My bad, forgot to enable it as a git hook, will fix.
>
>> Moreover, can you try harder to make that description shorter?
>>
>
> I am all ears for suggestions alternative phrasing, otherwise, no,
> that's about as hard as I try.

First of all, "used to" is (closer) equivalent to was.
Second, Mutex is one letter longer than Lock (here is important that
is just a kind of lock).
Third, "exclusive" is implied by Mutex / Lock word.
Fourth, "access to the bus when writing data" too verbose.

So, my suggestion is (two variants):
a) "Lock to serialize bus access when writing data."
b) "Lock to serialize access when writing data with serdev_device_write()."

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1613036

On Wed, Mar 29, 2017 at 7:43 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 29, 2017 at 5:16 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>
>>> So, what I would see if no one objects is patch series of two:
>>> 1) introduction of new API
>>> 2) removing old one.
>>>
>>> It will benefit for easier review and any potential code anthropologist.
>>>
>>
>> Second version of the patch preserves the old API an just
>> re-implements it in terms of a new one. I am not sure I see the
>> benefit in splitting it into two patches, but I'll leave it up to Rob
>> to decide.
>
> Sure. At least I posted benefits I see from splitting.
>
> + bisectability (in case we have to revert your new API by some reason
> it will be easier, hope will be not the case, though...)
>
>>>> +       } while (count &&
>>>> +                (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>>> +                                                       timeout)));
>>>
>>> So, would it be better to support interrupts here and return a
>>> corresponding error code to the user?
>>>
>>
>> I don't have a use-case for that and as far as I can tell, neither SPI
>> nor I2C slave device API offer such functionality universally, so I am
>> inclined to say no. Since the change from wait_for_completion to
>> wait_for_completion_timeout was made per Rob's request, I'd leave it
>> up to him to decided about this change as well.
>
> OK.
>
>>> Besides that question, readability might be better if you use
>>> temporary variable and pack above on one line:
>>>
>>> unsigned long to = timeout;
>>>
>>> } while (count && (to = ...(to)));
>>>
>>
>> Even if you shorten 'timeout' to 'to', formatted as a single line,
>> it'd still exceed line length limitations.
>
> How many? If we are talking about 2-3 characters, that's okay to leave
> them on one line.

Even so, at this point we are definitely in "personal preference"
territory and I'd rather avoid line wrapping.

>
>>>> + * @write_lock Mutext used to esure exclusive access to the bus when
>>>> + *             writing data with serdev_device_write()
>>>
>>> checkpatch.pl has integrated spellchecker AFAIU.
>>
>> My bad, forgot to enable it as a git hook, will fix.
>>
>>> Moreover, can you try harder to make that description shorter?
>>>
>>
>> I am all ears for suggestions alternative phrasing, otherwise, no,
>> that's about as hard as I try.
>
> First of all, "used to" is (closer) equivalent to was.
> Second, Mutex is one letter longer than Lock (here is important that
> is just a kind of lock).
> Third, "exclusive" is implied by Mutex / Lock word.
> Fourth, "access to the bus when writing data" too verbose.
>
> So, my suggestion is (two variants):
> a) "Lock to serialize bus access when writing data."
> b) "Lock to serialize access when writing data with serdev_device_write()."
>

I'll use option "a". Option "b" still exceeds line limit, and I'd
rather not have that.

Thanks,
Andrey Smirnov

[toc] | [prev] | [next] | [standalone]

#1612375

On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:
>>> Convert serdev_device_write_buf's code to be able to transfer amount of
>>> data potentially exceeding "write room" at the moment of invocation.
>>>
>>> To support that, also add serdev_device_write_wakeup.
>>>
>>> Drivers wanting to use full extent of serdev_device_write
>>> functionality are expected to provide serdev_device_write_wakeup as a
>>> sole handler of .write_wakeup event or call it as a part of driver's
>>> custom .write_wakeup code.
>>>
>>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>>
>>> replace those call to calls to serdev_device_write with timeout of
>>> 0. Providing .write_wakeup handler in such case is optional.
>>
>> Some indentation would be better if, for example, 0 will be kept on
>> previous line.
>>
>
> OK, sure.
>
>> So, what I would see if no one objects is patch series of two:
>> 1) introduction of new API
>> 2) removing old one.
>>
>> It will benefit for easier review and any potential code anthropologist.
>>
>
> Second version of the patch preserves the old API an just
> re-implements it in terms of a new one. I am not sure I see the
> benefit in splitting it into two patches, but I'll leave it up to Rob
> to decide.

I think it is fine as-is, but maybe the subject now is a bit misleading.

>>> --- a/drivers/tty/serdev/core.c
>>> +++ b/drivers/tty/serdev/core.c
>>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(serdev_device_close);
>>>
>>> -int serdev_device_write_buf(struct serdev_device *serdev,
>>> -                           const unsigned char *buf, size_t count)
>>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>>> +{
>>> +       complete(&serdev->write_comp);
>>> +}
>>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>>> +
>>> +int serdev_device_write(struct serdev_device *serdev,
>>> +                       const unsigned char *buf, size_t count,
>>> +                       unsigned long timeout)
>>>  {
>>>         struct serdev_controller *ctrl = serdev->ctrl;
>>> +       int ret;
>>>
>>> -       if (!ctrl || !ctrl->ops->write_buf)
>>> +       if (!ctrl || !ctrl->ops->write_buf ||
>>> +           (timeout && !serdev->ops->write_wakeup))
>>>                 return -EINVAL;
>>>
>>> -       return ctrl->ops->write_buf(ctrl, buf, count);
>>> +       mutex_lock(&serdev->write_lock);
>>> +       do {
>>> +               reinit_completion(&serdev->write_comp);
>>> +
>>> +               ret = ctrl->ops->write_buf(ctrl, buf, count);
>>> +               if (ret < 0)
>>> +                       break;
>>> +
>>
>>> +               buf   += ret;
>>
>> Extra white spaces.
>
> Which is there on purpose to re-align "+=" with "-=" on the next line.
> I'll remove it.
>
>>
>>> +               count -= ret;
>>> +
>>
>>> +       } while (count &&
>>> +                (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>> +                                                       timeout)));
>>
>> So, would it be better to support interrupts here and return a
>> corresponding error code to the user?
>>
>
> I don't have a use-case for that and as far as I can tell, neither SPI
> nor I2C slave device API offer such functionality universally, so I am
> inclined to say no. Since the change from wait_for_completion to
> wait_for_completion_timeout was made per Rob's request, I'd leave it
> up to him to decided about this change as well.

Honestly, I don't know. It's added easily enough if needed later.

Rob

[toc] | [prev] | [next] | [standalone]

#1613031

On Wed, Mar 29, 2017 at 2:25 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>>>> Convert serdev_device_write_buf's code to be able to transfer amount of
>>>> data potentially exceeding "write room" at the moment of invocation.
>>>>
>>>> To support that, also add serdev_device_write_wakeup.
>>>>
>>>> Drivers wanting to use full extent of serdev_device_write
>>>> functionality are expected to provide serdev_device_write_wakeup as a
>>>> sole handler of .write_wakeup event or call it as a part of driver's
>>>> custom .write_wakeup code.
>>>>
>>>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>>>
>>>> replace those call to calls to serdev_device_write with timeout of
>>>> 0. Providing .write_wakeup handler in such case is optional.
>>>
>>> Some indentation would be better if, for example, 0 will be kept on
>>> previous line.
>>>
>>
>> OK, sure.
>>
>>> So, what I would see if no one objects is patch series of two:
>>> 1) introduction of new API
>>> 2) removing old one.
>>>
>>> It will benefit for easier review and any potential code anthropologist.
>>>
>>
>> Second version of the patch preserves the old API an just
>> re-implements it in terms of a new one. I am not sure I see the
>> benefit in splitting it into two patches, but I'll leave it up to Rob
>> to decide.
>
> I think it is fine as-is, but maybe the subject now is a bit misleading.
>

OK, I'll modify the subject to be more representative of the change.

>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
>>>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(serdev_device_close);
>>>>
>>>> -int serdev_device_write_buf(struct serdev_device *serdev,
>>>> -                           const unsigned char *buf, size_t count)
>>>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>>>> +{
>>>> +       complete(&serdev->write_comp);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>>>> +
>>>> +int serdev_device_write(struct serdev_device *serdev,
>>>> +                       const unsigned char *buf, size_t count,
>>>> +                       unsigned long timeout)
>>>>  {
>>>>         struct serdev_controller *ctrl = serdev->ctrl;
>>>> +       int ret;
>>>>
>>>> -       if (!ctrl || !ctrl->ops->write_buf)
>>>> +       if (!ctrl || !ctrl->ops->write_buf ||
>>>> +           (timeout && !serdev->ops->write_wakeup))
>>>>                 return -EINVAL;
>>>>
>>>> -       return ctrl->ops->write_buf(ctrl, buf, count);
>>>> +       mutex_lock(&serdev->write_lock);
>>>> +       do {
>>>> +               reinit_completion(&serdev->write_comp);
>>>> +
>>>> +               ret = ctrl->ops->write_buf(ctrl, buf, count);
>>>> +               if (ret < 0)
>>>> +                       break;
>>>> +
>>>
>>>> +               buf   += ret;
>>>
>>> Extra white spaces.
>>
>> Which is there on purpose to re-align "+=" with "-=" on the next line.
>> I'll remove it.
>>
>>>
>>>> +               count -= ret;
>>>> +
>>>
>>>> +       } while (count &&
>>>> +                (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>>> +                                                       timeout)));
>>>
>>> So, would it be better to support interrupts here and return a
>>> corresponding error code to the user?
>>>
>>
>> I don't have a use-case for that and as far as I can tell, neither SPI
>> nor I2C slave device API offer such functionality universally, so I am
>> inclined to say no. Since the change from wait_for_completion to
>> wait_for_completion_timeout was made per Rob's request, I'd leave it
>> up to him to decided about this change as well.
>
> Honestly, I don't know. It's added easily enough if needed later.

OK, I'll keep things as is for now.

Thanks,
Andrey Smirnov

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web