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

[Christian.Gromm]

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