On Wed, Mar 10, 2021 at 1:28 PM Andy Shevchenko
[off-list ref] wrote:

[snip]

quoted

+
+static ssize_t gpio_sim_sysfs_line_show(struct device *dev,
+                                     struct device_attribute *attr,
+                                     char *buf)
+{
+     struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
+     struct gpio_sim_chip *chip = dev_get_drvdata(dev);
+     int ret;
+
+     mutex_lock(&chip->lock);
+     ret = sprintf(buf, "%u\n", !!test_bit(line_attr->offset, chip->values));

Shouldn't we use sysfs_emit() in a new code?

TIL it exists. :) I'll use it.

[snip]

quoted

+
+static ssize_t gpio_sim_config_dev_name_show(struct config_item *item,
+                                          char *page)
+{
+     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+     struct platform_device *pdev;
+     int ret;
+
+     mutex_lock(&config->lock);
+     pdev = config->pdev;
+     if (pdev)
+             ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
+     else
+             ret = sprintf(page, "n/a\n");

I dunno '/' (slash) is a good character to be handled in a shell.
I would prefer 'none' or 'not available' (I think space is easier,
because the rules to escape much simpler: need just to take it into
quotes, while / needs to be escaped separately).

My test cases work fine with 'n/a' but I can change it to 'none' if
it's less controversial.

[snip]

Also don't know what the rules about using s*printf() in the configfs.
Maybe we have sysfs_emit() analogue or it doesn't applicable here at all.
Greg?

There's no configfs_emit() or anything similar. Output for simple
attributes must simply not exceed 4096 bytes. It used to be PAGE_SIZE,
now it's defined in fs/configfs/file.c as SIMPLE_ATTR_SIZE. There's no
need to check the length of the string here though as we're only
showing what we received from the user-space anyway and configfs makes
sure we don't get more than SIMPLE_ATTR_SIZE in the store callback.

[snip]

quoted

+
+static int gpio_sim_config_commit_item(struct config_item *item)
+{
+     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+     struct property_entry properties[GPIO_SIM_MAX_PROP];
+     struct platform_device_info pdevinfo;
+     struct platform_device *pdev;
+     unsigned int prop_idx = 0;
+
+     memset(&pdevinfo, 0, sizeof(pdevinfo));
+     memset(properties, 0, sizeof(properties));
+
+     mutex_lock(&config->lock);
+
+     properties[prop_idx++] = PROPERTY_ENTRY_U32("gpio-sim,nr-gpios",
+                                                 config->num_lines);

quoted

+     if (config->label[0] != '\0')

I'm wondering if we need this check. Isn't core taking care of it?

quoted

+             properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
+                                                            config->label);

quoted

+     if (config->line_names)

Ditto.

quoted

+             properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+                                             "gpio-line-names",
+                                             config->line_names,
+                                             config->num_line_names);
+

But I would be creating empty properties for nothing. Better to just
not have them at all.

[snip]

Bartosz