Re: [PATCH v3 08/11] gpio: sim: new testing module

[Bartosz Golaszewski]

On Wed, Mar 10, 2021 at 1:28 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>

[snip]

> > +
> > +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]

> > +
> > +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]

> > +
> > +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);
>
> > +     if (config->label[0] != '\0')
>
> I'm wondering if we need this check. Isn't core taking care of it?
>
> > +             properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
> > +                                                            config->label);
>
> > +     if (config->line_names)
>
> Ditto.
>
> > +             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