Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> <henning.schild@siemens.com> wrote:
>>
>> From: Henning Schild <henning.schild@siemens.com>
>>
>> This mainly implements detection of these devices and will allow
>> secondary drivers to work on such machines.
>>
>> The identification is DMI-based with a vendor specific way to tell them
>> apart in a reliable way.
>>
>> Drivers for LEDs and Watchdogs will follow to make use of that platform
>> detection.
> 
>> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> 
> The order is wrong taking into account the From line in the body. So,
> it's not clear who is the author, who is a co-developer, and who is
> the committer (one person may utilize few roles).
> Check for the rest of the series as well (basically this is the rule
> of thumb to recheck entire code for the comment you have got at any
> single place of it).
> 
> ...
> 
>> +config SIEMENS_SIMATIC_IPC
>> +       tristate "Siemens Simatic IPC Class driver"
>> +       depends on PCI
>> +       help
>> +         This Simatic IPC class driver is the central of several drivers. It
>> +         is mainly used for system identification, after which drivers in other
>> +         classes will take care of driving specifics of those machines.
>> +         i.e. leds and watchdogs
> 
> LEDs
> watchdog. (missed period and singular form)
> 
> Module name?
> 
>>  endif # X86_PLATFORM_DEVICES
> 
> Not sure about the ordering of the section in Kconfig, but it is up to
> maintainers.
> 
> ...
> 
>> +# Siemens Simatic Industrial PCs
>> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC)      += simatic-ipc.o
> 
> Ditto.
> 
> ...
> 
>> + * Siemens SIMATIC IPC driver for LEDs
> 
> It seems to be used in patch 4 which is not LED related. Also, why is
> it here if it's for LEDs?
> 
> ...
> 
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
> 
> Replace with SPDX
> 
> ...
> 
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
> 
> Ordered?
> 
>> +#include <linux/platform_data/x86/simatic-ipc.h>
> 
> ...
> 
>> +static int register_platform_devices(u32 station_id)
>> +{
>> +       int i;
>> +       u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
>> +       u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> 
> Reversed xmas tree order?
> 
>> +       platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(device_modes); i++) {
>> +               if (device_modes[i].station_id == station_id) {
>> +                       ledmode = device_modes[i].led_mode;
>> +                       wdtmode = device_modes[i].wdt_mode;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
>> +               platform_data.devmode = ledmode;
> 
>> +               ipc_led_platform_device = platform_device_register_data
>> +                       (NULL, KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
>> +                        &platform_data, sizeof(struct simatic_ipc_platform));
> 
> Strange indentation (second line).
> 
>> +               if (IS_ERR(ipc_led_platform_device))
>> +                       return PTR_ERR(ipc_led_platform_device);
> 
>> +               pr_debug(KBUILD_MODNAME ": device=%s created\n",
>> +                        ipc_led_platform_device->name);
> 
> Utilize pr_fmt() instead of adding prefixes like this.
> 
>> +       }
> 
>> +       if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
>> +               platform_data.devmode = wdtmode;
>> +               ipc_wdt_platform_device = platform_device_register_data
>> +                       (NULL, KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE,
>> +                        &platform_data, sizeof(struct simatic_ipc_platform));
>> +               if (IS_ERR(ipc_wdt_platform_device))
>> +                       return PTR_ERR(ipc_wdt_platform_device);
>> +
>> +               pr_debug(KBUILD_MODNAME ": device=%s created\n",
>> +                        ipc_wdt_platform_device->name);
>> +       }
> 
> Same comments as above.
> 
>> +       if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
>> +           wdtmode == SIMATIC_IPC_DEVICE_NONE) {
>> +               pr_warn(KBUILD_MODNAME
>> +                       ": unsupported IPC detected, station id=%08x\n",
>> +                       station_id);
> 
> Ugly indentation. With pr_fmt() in use will be much better.
> 
>> +               return -EINVAL;
> 
>> +       } else {
> 
> Redundant.
> 
>> +               return 0;
>> +       }
>> +}
> 
> ...
> 
>> +/*
>> + * Get membase address from PCI, used in leds and wdt modul. Here we read
>> + * the bar0. The final address calculation is done in the appropriate modules
> 
> bar -> BAR
> Missed period.
> 
>> + */
> 
>> +
> 
> Unneeded blank line.
> 
>> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
>> +{
>> +       u32 bar0 = 0;
> 
>> +#ifdef CONFIG_PCI
> 
> It's ugly besides the fact that you have a dependency.
> 
>> +       struct pci_bus *bus;
> 
> Missed blank line.
> 
>> +       /*
>> +        * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
>> +        * to have a quick look at it, before we hide it again.
>> +        * Also grab the pci rescan lock so that device does not get discovered
>> +        * and remapped while it is visible.
>> +        * This code is inspired by drivers/mfd/lpc_ich.c
>> +        */
>> +       bus = pci_find_bus(0, 0);
>> +       pci_lock_rescan_remove();
>> +       pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
>> +       pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
>> +
>> +       bar0 &= ~0xf;
>> +       pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
>> +       pci_unlock_rescan_remove();
>> +#endif /* CONFIG_PCI */
>> +       return bar0;
>> +}
>> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
> 
> Oy vey! I know what this is and let's do it differently. I have some
> (relatively old) patch series I can send you privately for testing.

This bit stood out the most to me too, it would be good if we can this fixed
in some cleaner work. So I'm curious how things will look with Andy's work
integrated.

Also I don't think this should be exported. Instead this (or its replacement)
should be used to get the address for an IOMEM resource to add the platform 
devices when they are instantiated. Then the platform-dev drivers can just
use the regular functions to get their resources instead of relying on this
module.

Regards,

Hans




> 
> ...
> 
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
> 
> Not needed when you have SPDX.
> 
> ...
> 
>> +#include <linux/pci.h>
> 
> Wrong header. Should be types.h
> 
> ...
> 
>> +#include <linux/dmi.h>
>> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> 
> Missed headers. You need to include ones that the code below is a
> direct user of.
> 
> Like types.h
>