On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote:

On Fri, Dec 3, 2021 at 9:08 PM Andy Shevchenko
[off-list ref] wrote:

quoted

On Fri, Dec 03, 2021 at 02:30:00PM +0100, Bartosz Golaszewski wrote:

...

quoted

quoted

+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>

I would rather move this group below to emphasize that this is closer to GPIO
then to other APIs.

quoted

+#include <linux/sysfs.h>
+

...here.

With the number of headers in this file, I'd stick with alphabetical order.

I understand that and agree, but my point is orthogonal to this. The idea is to
emphasize that "hey. this driver has tough relations with the GPIO subsystem".
This is the way, for example, IIO does and I like it.

quoted

quoted

+#include "gpiolib.h"

...

quoted

quoted

+static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
+                            unsigned int offset, int value)

I would use up to 100 here...

quoted

+     if (test_bit(FLAG_REQUESTED, &desc->flags) &&
+         !test_bit(FLAG_IS_OUT, &desc->flags)) {

...here and so on.

But it's up to you.

Nah, the lines are broken just fine. Let's not overuse the limit.

Yes, but I would consider to join back those which are up to ~83 characters
(I already pointed out at least to one example like this).

...

quoted

quoted

+     if (sysfs_streq(buf, "pull-down"))
+             pull = 0;
+     else if (sysfs_streq(buf, "pull-up"))
+             pull = 1;
+     else
+             return -EINVAL;

sysfs_match_string() and use the very same string array in the above function
to print them?

I suppose you agree on this?

...

quoted

quoted

+     /* Default to input mode. */
+     bitmap_fill(chip->direction_map, num_lines);

More accurate is to use bitmap_set(). If we ever debug this it also helpful.

I'm not sure what you mean, this sets all bits to 1.

Nope, it may set _more_ than all bits. That's why bitmap_set() is more
accurate, because it will do exact setting.

...

quoted

quoted

+     if (strcmp(trimmed, "input") == 0)
+             dir = GPIOD_IN;
+     else if (strcmp(trimmed, "output-high") == 0)
+             dir = GPIOD_OUT_HIGH;
+     else if (strcmp(trimmed, "output-low") == 0)
+             dir = GPIOD_OUT_LOW;
+     else
+             dir = -EINVAL;

Same idea, i.e. static string array and use it above and here with help
of match_string().

It would be great but GPIOD_IN etc. are bit flags and not sequence enums.

Ah, okay, it will make rather sparse array.

-- 
With Best Regards,
Andy Shevchenko