From: "Enrico Weigelt, metux IT consult" <lkml@metux.net>
To: Henning Schild <henning.schild@siemens.com>,
linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
platform-driver-x86@vger.kernel.org,
linux-watchdog@vger.kernel.org
Cc: Srikanth Krishnakar <skrishnakar@gmail.com>,
Jan Kiszka <jan.kiszka@siemens.com>,
Gerd Haeussler <gerd.haeussler.ext@siemens.com>,
Guenter Roeck <linux@roeck-us.net>,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Mark Gross <mgross@linux.intel.com>,
Hans de Goede <hdegoede@redhat.com>, Pavel Machek <pavel@ucw.cz>,
Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
Date: Thu, 18 Mar 2021 11:25:10 +0100 [thread overview]
Message-ID: <546c300c-cb13-1074-de37-68ed6fad4b27@metux.net> (raw)
In-Reply-To: <20210315095710.7140-3-henning.schild@siemens.com>
On 15.03.21 10:57, Henning Schild wrote:
Hi,
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
> new file mode 100644
> index 000000000000..0f7e6320e10d
> --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for LEDs
> + *
> + * Copyright (c) Siemens AG, 2018-2021
> + *
> + * Authors:
> + * Henning Schild <henning.schild@siemens.com>
> + * Jan Kiszka <jan.kiszka@siemens.com>
> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> + */
> +
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/spinlock.h>
> +
> +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
> +
> +struct simatic_ipc_led {
> + unsigned int value; /* mask for io and offset for mem */
> + char name[32];
> + struct led_classdev cdev;
> +};
> +
> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" },
> + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" },
> + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" },
> + {0, ""},
> +};
Wouldn't it be better to name them like they're labeled on the device,
as shown on page #19 of the manual, or perhaps a little bit more
generic nameing (eg. power, status, error, maint) ?
> +/* the actual start will be discovered with pci, 0 is a placeholder */
> +struct resource simatic_ipc_led_mem_res =
> + DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
> +
> +static void *simatic_ipc_led_memory;
> +
hmm, could there *ever* be multiple instances of the driver ?
Wouldn't it be better to put this in the device priv data instead ?
> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> + {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"},
> + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"},
> + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"},
> + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"},
> + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"},
> + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"},
> + {0, ""},
> +};
> +
> +static struct resource simatic_ipc_led_io_res =
> + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, KBUILD_MODNAME);
> +
> +static DEFINE_SPINLOCK(reg_lock);
Does this protect global data structures ? If not, I'd rather put it
into the device priv data instead.
BTW: doesn't have struct led_classdev already have a lock that
can be used ? Can multiple calls to led ops (within the same device)
at the same time happen at all, or does led core already serialize
that ?
> +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
> + enum led_brightness brightness)
> +{
> + struct simatic_ipc_led *led =
> + container_of(led_cd, struct simatic_ipc_led, cdev);
> + unsigned long flags;
> + unsigned int val;
> +
> + spin_lock_irqsave(®_lock, flags);
> +
> + val = inw(SIMATIC_IPC_LED_PORT_BASE);
> + if (brightness == LED_OFF)
> + outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE);
> + else
> + outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE);
Don't we already have an helper for setting or clearing bits in IO
registers (that already does the read + set/clear + write at once) ?
Does that really need to be protected by lock ?
(can happen multiple calls to that func from different threads happen
at all ?)
Is the port really *always* the same, so it really can be a const ?
<snip>
> +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> +{
> + struct simatic_ipc_platform *plat;
> + struct device *dev = &pdev->dev;
> + struct simatic_ipc_led *ipcled;
> + struct led_classdev *cdev;
> + struct resource *res;
> + int err, type;
> + u32 *p;
> +
> + plat = pdev->dev.platform_data;
Maybe put this into swnode ?
IIRC, the consensus is not to introduce new platform data structs
anymore, instead legacy pdata to swnode some day.
> + switch (plat->devmode) {
> + case SIMATIC_IPC_DEVICE_227D:
> + case SIMATIC_IPC_DEVICE_427E:
> + res = &simatic_ipc_led_io_res;
> + ipcled = simatic_ipc_leds_io;
> + /* the 227D is high on while 427E is low on, invert the struct
> + * we have
> + */
> + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
> + while (ipcled->value) {
> + ipcled->value = swab16(ipcled->value);
Uff, better use explicit endian conversion macros (eg. be*_to_cpu()) for
that.
Also, I wouldn't change those global structs, instead put those data
into device priv data and make the global stuff const. You could also
use the same field for both port-io and mmap'ed variants. And adding
regmap to the equation, could use the same led ops for both. (IMHO,
the little bit of overhead by regmap shouldn't matter here)
> + ipcled++;
> + }
> + ipcled = simatic_ipc_leds_io;
> + }
> + type = IORESOURCE_IO;
> + if (!devm_request_region(dev, res->start,
> + resource_size(res),
> + KBUILD_MODNAME)) {
> + dev_err(dev,
> + "Unable to register IO resource at %pR\n",
> + res);
> + return -EBUSY;
> + }
> + break;
> + case SIMATIC_IPC_DEVICE_127E:
> + res = &simatic_ipc_led_mem_res;
> + ipcled = simatic_ipc_leds_mem;
> + type = IORESOURCE_MEM;
> +
> + /* get GPIO base from PCI */
> + res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
> + if (res->start == 0)
> + return -ENODEV;
Where does that device actually sit on ? Some generic card ? Some ASIC
or FPGA ?
It seems this driver is instantiated by another one, which already knows
what device we're actually dealing with (as it sets plat->devmode).
Why not letting that parent device also tell the io resource to this
driver ?
> + while (ipcled->value) {
> + cdev = &ipcled->cdev;
> + cdev->brightness_set = simatic_ipc_led_set_io;
> + cdev->brightness_get = simatic_ipc_led_get_io;
> + if (type == IORESOURCE_MEM) {
> + cdev->brightness_set = simatic_ipc_led_set_mem;
> + cdev->brightness_get = simatic_ipc_led_get_mem;
> + }
Why not if/else ?
> + cdev->max_brightness = LED_ON;
> + cdev->name = ipcled->name;
> +
> + err = devm_led_classdev_register(dev, cdev);
> + if (err < 0)
> + return err;
> + ipcled++;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver led_driver = {
Why not calling it simatic_ipc_led_driver ?
> + .probe = simatic_ipc_leds_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + },
> +};
> +
> +module_platform_driver(led_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
>
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
next prev parent reply other threads:[~2021-03-18 10:26 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-15 9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-03-15 9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
2021-03-15 10:31 ` Andy Shevchenko
2021-03-15 16:30 ` Henning Schild
2021-03-17 19:13 ` Henning Schild
2021-03-17 20:03 ` Hans de Goede
2021-03-18 11:30 ` Enrico Weigelt, metux IT consult
2021-03-18 11:45 ` Hans de Goede
2021-03-26 9:55 ` Henning Schild
2021-03-26 12:21 ` Hans de Goede
2021-03-15 9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
2021-03-15 10:48 ` Andy Shevchenko
2021-03-15 11:19 ` Pavel Machek
2021-03-15 11:26 ` Andy Shevchenko
2021-03-15 12:40 ` Henning Schild
2021-03-18 11:38 ` Enrico Weigelt, metux IT consult
2021-03-27 9:46 ` Henning Schild
2021-04-01 16:20 ` Enrico Weigelt, metux IT consult
2021-03-27 9:56 ` Henning Schild
2021-03-18 10:27 ` Enrico Weigelt, metux IT consult
2021-03-18 12:40 ` Alexander Dahl
2021-03-23 17:45 ` Henning Schild
2021-03-26 16:33 ` Henning Schild
2021-03-18 10:25 ` Enrico Weigelt, metux IT consult [this message]
2021-03-27 11:16 ` Henning Schild
2021-03-27 11:41 ` Henning Schild
2021-03-15 9:57 ` [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
2021-03-15 15:10 ` Guenter Roeck
2021-03-29 16:28 ` Henning Schild
2021-03-15 9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
2021-03-15 10:14 ` Henning Schild
2021-03-15 10:19 ` Hans de Goede
2021-03-15 12:09 ` Henning Schild
2021-03-15 14:58 ` [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries Henning Schild
2021-03-15 16:31 ` Hans de Goede
2021-03-15 17:00 ` Henning Schild
2021-03-15 18:01 ` Hans de Goede
2021-03-16 5:47 ` Henning Schild
2021-03-16 9:43 ` Hans de Goede
2021-03-15 13:25 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs kernel test robot
2021-03-15 13:25 ` kernel test robot
2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
2021-03-15 16:08 ` Henning Schild
2021-08-02 9:21 ` Henning Schild
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=546c300c-cb13-1074-de37-68ed6fad4b27@metux.net \
--to=lkml@metux.net \
--cc=andy.shevchenko@gmail.com \
--cc=gerd.haeussler.ext@siemens.com \
--cc=hdegoede@redhat.com \
--cc=henning.schild@siemens.com \
--cc=jan.kiszka@siemens.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mgross@linux.intel.com \
--cc=pavel@ucw.cz \
--cc=platform-driver-x86@vger.kernel.org \
--cc=skrishnakar@gmail.com \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.