On Wed, Mar 10, 2021 at 1:28 PM Andy Shevchenko
<andriy.shevche...@linux.intel.com> 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
