Re: [PATCH v2 1/2] staging: most: dim2: do not double-register the same device

From: Christian.Gromm
Date: Tue Oct 12 2021 - 08:59:43 EST


On Mon, 2021-10-11 at 09:11 +0300, Nikita Yushchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Commit 723de0f9171e ("staging: most: remove device from interface
> structure") moved registration of driver-provided struct device to
> the most subsystem.
>
> Dim2 used to register the same struct device to provide a custom
> device
> attribute. This causes double-registration of the same struct device.
>
> Fix that by moving the custom attribute to driver's dev_groups.
> This moves attribute to the platform_device object, which is a better
> location for platform-specific attributes anyway.
>
> Fixes: 723de0f9171e ("staging: most: remove device from interface
> structure")
> Signed-off-by: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> ---
> Changes from v1:
> - use ATTRIBUTE_GROUPS()
> - use sysfs_emit()
>
> drivers/staging/most/dim2/Makefile | 2 +-
> drivers/staging/most/dim2/dim2.c | 24 ++++++++-------
> drivers/staging/most/dim2/sysfs.c | 49 ----------------------------
> --
> drivers/staging/most/dim2/sysfs.h | 11 -------
> 4 files changed, 14 insertions(+), 72 deletions(-)
> delete mode 100644 drivers/staging/most/dim2/sysfs.c
>
> diff --git a/drivers/staging/most/dim2/Makefile
> b/drivers/staging/most/dim2/Makefile
> index 861adacf6c72..5f9612af3fa3 100644
> --- a/drivers/staging/most/dim2/Makefile
> +++ b/drivers/staging/most/dim2/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_MOST_DIM2) += most_dim2.o
>
> -most_dim2-objs := dim2.o hal.o sysfs.o
> +most_dim2-objs := dim2.o hal.o
> diff --git a/drivers/staging/most/dim2/dim2.c
> b/drivers/staging/most/dim2/dim2.c
> index e8b03fa90e80..96cb5280a385 100644
> --- a/drivers/staging/most/dim2/dim2.c
> +++ b/drivers/staging/most/dim2/dim2.c
> @@ -118,7 +118,8 @@ struct dim2_platform_data {
> (((p)[1] == 0x18) && ((p)[2] == 0x05) && ((p)[3] == 0x0C) &&
> \
> ((p)[13] == 0x3C) && ((p)[14] == 0x00) && ((p)[15] == 0x0A))
>
> -bool dim2_sysfs_get_state_cb(void)
> +static ssize_t state_show(struct device *dev, struct
> device_attribute *attr,
> + char *buf)
> {
> bool state;
> unsigned long flags;
> @@ -127,9 +128,18 @@ bool dim2_sysfs_get_state_cb(void)
> state = dim_get_lock_state();
> spin_unlock_irqrestore(&dim_lock, flags);
>
> - return state;
> + return sysfs_emit(buf, "%s\n", state ? "locked" : "");
> }
>
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *dim2_attrs[] = {
> + &dev_attr_state.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dim2);
> +
> /**
> * dimcb_on_error - callback from HAL to report miscommunication
> between
> * HDM and HAL
> @@ -874,16 +884,8 @@ static int dim2_probe(struct platform_device
> *pdev)
> goto err_stop_thread;
> }
>
> - ret = dim2_sysfs_probe(&dev->dev);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to create sysfs
> attribute\n");
> - goto err_unreg_iface;
> - }
> -
> return 0;
>
> -err_unreg_iface:
> - most_deregister_interface(&dev->most_iface);
> err_stop_thread:
> kthread_stop(dev->netinfo_task);
> err_shutdown_dim:
> @@ -906,7 +908,6 @@ static int dim2_remove(struct platform_device
> *pdev)
> struct dim2_hdm *dev = platform_get_drvdata(pdev);
> unsigned long flags;
>
> - dim2_sysfs_destroy(&dev->dev);
> most_deregister_interface(&dev->most_iface);
> kthread_stop(dev->netinfo_task);
>
> @@ -1100,6 +1101,7 @@ static struct platform_driver dim2_driver = {
> .driver = {
> .name = "hdm_dim2",
> .of_match_table = dim2_of_match,
> + .dev_groups = dim2_groups,
> },
> };
>
> diff --git a/drivers/staging/most/dim2/sysfs.c
> b/drivers/staging/most/dim2/sysfs.c
> deleted file mode 100644
> index c85b2cdcdca3..000000000000
> --- a/drivers/staging/most/dim2/sysfs.c
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * sysfs.c - MediaLB sysfs information
> - *
> - * Copyright (C) 2015, Microchip Technology Germany II GmbH & Co. KG
> - */
> -
> -/* Author: Andrey Shvetsov <andrey.shvetsov@xxxxxx> */
> -
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> -#include <linux/kernel.h>
> -#include "sysfs.h"
> -#include <linux/device.h>
> -
> -static ssize_t state_show(struct device *dev, struct
> device_attribute *attr,
> - char *buf)
> -{
> - bool state = dim2_sysfs_get_state_cb();
> -
> - return sprintf(buf, "%s\n", state ? "locked" : "");
> -}
> -
> -static DEVICE_ATTR_RO(state);
> -
> -static struct attribute *dev_attrs[] = {
> - &dev_attr_state.attr,
> - NULL,
> -};
> -
> -static struct attribute_group dev_attr_group = {
> - .attrs = dev_attrs,
> -};
> -
> -static const struct attribute_group *dev_attr_groups[] = {
> - &dev_attr_group,
> - NULL,
> -};
> -
> -int dim2_sysfs_probe(struct device *dev)
> -{
> - dev->groups = dev_attr_groups;
> - return device_register(dev);
> -}
> -
> -void dim2_sysfs_destroy(struct device *dev)
> -{
> - device_unregister(dev);
> -}
> diff --git a/drivers/staging/most/dim2/sysfs.h
> b/drivers/staging/most/dim2/sysfs.h
> index 24277a17cff3..09115cf4ed00 100644
> --- a/drivers/staging/most/dim2/sysfs.h
> +++ b/drivers/staging/most/dim2/sysfs.h
> @@ -16,15 +16,4 @@ struct medialb_bus {
> struct kobject kobj_group;
> };
>
> -struct device;
> -
> -int dim2_sysfs_probe(struct device *dev);
> -void dim2_sysfs_destroy(struct device *dev);
> -
> -/*
> - * callback,
> - * must deliver MediaLB state as true if locked or false if unlocked
> - */
> -bool dim2_sysfs_get_state_cb(void);
> -
> #endif /* DIM2_SYSFS_H */
> --
> 2.30.2
>
>
>

Acked-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>

thanks,
Chris