| Message ID | 20211216185217.1054495-8-lars@metafoo.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: Use sysfs_emit() | expand |
On Thu, Dec 16, 2021 at 07:52:11PM +0100, Lars-Peter Clausen wrote: > sysfs_emit() is preferred over raw s*printf() for sysfs attributes since it > knows about the sysfs buffer specifics and has some built-in checks for > size and alignment. I realise that the above is some copy-paste boiler plate, but none of it is really relevant here when the driver uses the attribute buffer directly with scnprintf() and a PAGE_SIZE argument. This should probably be rephrased in terms of consistency and the documentation now claiming that only sysfs_emit() should be used in show() functions (e.g. to avoid problems in drivers that would have failed to follow the previous instructions). > Use sysfs_emit() to format the custom device attributes of the lm3533 > driver. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> That said, the change itself is otherwise fine even I'm not sure it's generally worth the churn to convert all existing show() functions: Reviewed-by: Johan Hovold <johan@kernel.org> > --- > drivers/iio/light/lm3533-als.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c > index 8a621244dd01..827bc25269e9 100644 > --- a/drivers/iio/light/lm3533-als.c > +++ b/drivers/iio/light/lm3533-als.c > @@ -417,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev, > enable = 0; > } > > - return scnprintf(buf, PAGE_SIZE, "%u\n", enable); > + return sysfs_emit(buf, "%u\n", enable); > } > > static ssize_t store_thresh_either_en(struct device *dev, > @@ -474,7 +474,7 @@ static ssize_t show_zone(struct device *dev, > if (ret) > return ret; > > - return scnprintf(buf, PAGE_SIZE, "%u\n", zone); > + return sysfs_emit(buf, "%u\n", zone); > } > > enum lm3533_als_attribute_type { > @@ -530,7 +530,7 @@ static ssize_t show_als_attr(struct device *dev, > if (ret) > return ret; > > - return scnprintf(buf, PAGE_SIZE, "%u\n", val); > + return sysfs_emit(buf, "%u\n", val); > } > > static ssize_t store_als_attr(struct device *dev, Johan
On Fri, 17 Dec 2021 10:14:49 +0100 Johan Hovold <johan@kernel.org> wrote: > On Thu, Dec 16, 2021 at 07:52:11PM +0100, Lars-Peter Clausen wrote: > > sysfs_emit() is preferred over raw s*printf() for sysfs attributes since it > > knows about the sysfs buffer specifics and has some built-in checks for > > size and alignment. > > I realise that the above is some copy-paste boiler plate, but none of it > is really relevant here when the driver uses the attribute buffer > directly with scnprintf() and a PAGE_SIZE argument. > > This should probably be rephrased in terms of consistency and the > documentation now claiming that only sysfs_emit() should be used in > show() functions (e.g. to avoid problems in drivers that would have > failed to follow the previous instructions). > I've added some stuff about best practice and chances of being copied into new drives to this text. > > Use sysfs_emit() to format the custom device attributes of the lm3533 > > driver. > > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > That said, the change itself is otherwise fine even I'm not sure it's > generally worth the churn to convert all existing show() functions: Agreed it's not an activity I'd consider of high importance but there is definitely advantage in updating drivers to current best practice because it saves having to comment on it as often in reviews. > > Reviewed-by: Johan Hovold <johan@kernel.org> Thanks, Jonathan > > > --- > > drivers/iio/light/lm3533-als.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c > > index 8a621244dd01..827bc25269e9 100644 > > --- a/drivers/iio/light/lm3533-als.c > > +++ b/drivers/iio/light/lm3533-als.c > > @@ -417,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev, > > enable = 0; > > } > > > > - return scnprintf(buf, PAGE_SIZE, "%u\n", enable); > > + return sysfs_emit(buf, "%u\n", enable); > > } > > > > static ssize_t store_thresh_either_en(struct device *dev, > > @@ -474,7 +474,7 @@ static ssize_t show_zone(struct device *dev, > > if (ret) > > return ret; > > > > - return scnprintf(buf, PAGE_SIZE, "%u\n", zone); > > + return sysfs_emit(buf, "%u\n", zone); > > } > > > > enum lm3533_als_attribute_type { > > @@ -530,7 +530,7 @@ static ssize_t show_als_attr(struct device *dev, > > if (ret) > > return ret; > > > > - return scnprintf(buf, PAGE_SIZE, "%u\n", val); > > + return sysfs_emit(buf, "%u\n", val); > > } > > > > static ssize_t store_als_attr(struct device *dev, > > Johan
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c index 8a621244dd01..827bc25269e9 100644 --- a/drivers/iio/light/lm3533-als.c +++ b/drivers/iio/light/lm3533-als.c @@ -417,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev, enable = 0; } - return scnprintf(buf, PAGE_SIZE, "%u\n", enable); + return sysfs_emit(buf, "%u\n", enable); } static ssize_t store_thresh_either_en(struct device *dev, @@ -474,7 +474,7 @@ static ssize_t show_zone(struct device *dev, if (ret) return ret; - return scnprintf(buf, PAGE_SIZE, "%u\n", zone); + return sysfs_emit(buf, "%u\n", zone); } enum lm3533_als_attribute_type { @@ -530,7 +530,7 @@ static ssize_t show_als_attr(struct device *dev, if (ret) return ret; - return scnprintf(buf, PAGE_SIZE, "%u\n", val); + return sysfs_emit(buf, "%u\n", val); } static ssize_t store_als_attr(struct device *dev,
sysfs_emit() is preferred over raw s*printf() for sysfs attributes since it knows about the sysfs buffer specifics and has some built-in checks for size and alignment. Use sysfs_emit() to format the custom device attributes of the lm3533 driver. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/iio/light/lm3533-als.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)