> -----Original Message----- > From: Lee Jones [mailto:lee.jones@xxxxxxxxxx] > Sent: Tuesday, August 08, 2017 10:02 AM > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > Cc: robh+dt@xxxxxxxxxx; pavel@xxxxxx; devicetree@xxxxxxxxxxxxxxx; > j.anaszewski@xxxxxxxxxxx; rpurdie@xxxxxxxxx; linux- > leds@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx; jacek.anaszewski@xxxxxxxxx > Subject: Re: [patch v2 1/2] mfd: Add Mellanox regmap core driver > > On Tue, 08 Aug 2017, Vadim Pasternak wrote: > > > -----Original Message----- > > > From: Lee Jones [mailto:lee.jones@xxxxxxxxxx] > > > Sent: Monday, August 07, 2017 6:59 PM > > > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > > > Cc: robh+dt@xxxxxxxxxx; pavel@xxxxxx; devicetree@xxxxxxxxxxxxxxx; > > > j.anaszewski@xxxxxxxxxxx; rpurdie@xxxxxxxxx; linux- > > > leds@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx; jacek.anaszewski@xxxxxxxxx > > > Subject: Re: [patch v2 1/2] mfd: Add Mellanox regmap core driver > > > > > > On Wed, 26 Jul 2017, Vadim Pasternak wrote: > > > > > > > This patch adds core regmap platform driver for Mellanox BMC cards > > > > with the programmable devcies based chassis control. The device > > > > logics, controlled by software includes: > > > > - Interrupt handling for chassis, ASIC, CPU events; > > > > - LED handling; > > > > - Exposes through sysfs mux, reset signals, reset cause > > > > notification; The patch provides support for the access to > > > > programmable device through the register map and allows dynamic > > > > device tree manipulation at runtime for removable devices. > > > > > > > > This driver requires activator driver, which responsibility is to > > > > create register map and pass it to regmap core. Such activator > > > > could be based for example on I2C, SPI or MMIO interface. > > > > > > > > Drivers exposes the number of hwmon attributes to sysfs according > > > > to the attribute groups, defined in the device tree. These > > > > attributes will be located for example in /sys/class/hwmon/hwmonX > > > > folder, which is a symbolic link to for example: > > > > /sys/bus/i2c/devices/4-0072/mlxreg-core.1138/hwmon/hwmon10. > > > > The attributes are divided to the groups, like in the below example: > > > > MUX nodes > > > > - safe_bios_disable > > > > - spi_burn_bios_ci > > > > - spi_burn_ncsi > > > > - uart_sel > > > > Reset nodes: > > > > - sys_power_cycle > > > > - bmc_upgrade > > > > - asic_reset > > > > Cause of reset nodes: > > > > - cpu_kernel_panic > > > > - cpu_shutdown > > > > - bmc_warm_reset > > > > PSU nodes' statuses > > > > - psu1 > > > > - psu2 > > > > FAN nodes' statuses: > > > > - fan1 > > > > - fan2 > > > > Power cable nodes' statuses: > > > > - pwr1 > > > > - pwr2 > > > > Asic nodes' statuses: > > > > - asic1 > > > > - asic2 > > > > General purpose RW nodes: > > > > - version > > > > > > > > Drivers also probes LED platform driver, in case device tree > > > > description contains LED nodes. > > > > > > > > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > > > > --- > > > > v1->v2: > > > > Comments pointed out by Pavel: > > > > - Remove extra new line in mellanox,mlxreg-core; > > > > - Replace three NOT with one in mlxreg_core_attr_show; > > > > - Make error message in mlxreg_core_work_helper shorter; > > > > - Make attribute assignment more readable; > > > > - Separate mellanox,mlxreg-core file for sending to DT mainataners. > > > > Comments pointed out by Jacek: > > > > - Since brightness_set_blocking is used instead of > > > > brightness_set, three fields from mlxreg_core_led_data. > > > > --- > > > > .../devicetree/bindings/vendor-prefixes.txt | 1 + > > > > MAINTAINERS | 7 + > > > > drivers/mfd/Kconfig | 15 + > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/mlxreg-core.c | 1257 > ++++++++++++++++++++ > > > > include/linux/platform_data/mlxreg.h | 87 ++ > > > > 6 files changed, 1368 insertions(+) create mode 100644 > > > > drivers/mfd/mlxreg-core.c create mode 100644 > > > > include/linux/platform_data/mlxreg.h > > > > > > I'm not sure what this driver is, but it isn't an MFD driver. MFD > > > drivers purpose is to set up shared resources, then register child > > > device drivers which live in other subsystems. Since this device > > > appears to be networking related, perhaps it might find a suitable > > > home in drivers/net. Failing that, you need to split out each of > > > the functions into their own subsystems; drivers/input, > > > drivers/hwmon, drivers/reset, etc and supply an MFD driver to register > them all. > > > > Hi Lee, > > > > Thank you for your reply. > > > > This driver sets the shared resources of programmable (CPLD or FPGA) > devices. This devices are used to control different functionalities, like LED, > Watchdog, PWM, IC. > > System can be equipped with several programmable devices, each of them > can contain logic for LED, WD, PWM, IC. Configuration is coming from DTS. > > > > Actually it doesn't deal with the networking at all. > > In core driver I put code, which parses device table tree, exposes several > types of registers to sysfs and then register a child device. > > > > This version of the driver has been provided for system with BMC card. This > system is equipped with four CPLDs. One of these CPLD has LED control logic, > all of them have IC logic. > > On these system WD and PWM are controlled by BMC SoC (Aspeed 2520). > > > > Registers with reset and reset cause info, just exposed to the user space > through sysfs and in BMC they just used by IPMI demon and doesn't have > special handling in kernel and doesn't require kernel driver. > > > > I am planning to add the drivers for WD and PWM in the future, when we'll > have the systems required such functionalities. > > Currently I support only LED driver, which is located in drivers/leds. > > And there is no special place for IC, so I located this code within the > regmap core driver. > > > > Main purpose of IC part is hotplug control for the dynamic units or > components, for example for PSUs, FANs. > > For example the below record specifies programmable device register and > mask, which triggers probing or removing relevant PSU controller driver: > > pwr { > > aggr_mask = <0x08>; > > reg = <0x64>; > > mask = <0x03>; > > phandles = <&psu1_ctrl &psu2_ctrl>; > > }; > > > > So, currently I have mfd related driver in drivers/leds, and going to have > also drivers in drivers/watchdog and drivers/hwmon (or drivers/pwm). > > And for IC functionality there is no suitable place, so it seems to be logical > to not split it into the separate driver and keep it within mlxreg-core. > > > > What do you think? > > Why don't you extend the support you already provide in drivers/platform? > This seems like a logical place for an all encompassing device which cannot be > split out to individual subsystems. There is a problem to extend for that the existing driver drivers/platform/x86/mlxcpld-hotplug.c. It has been provided for x86 arch and I am planning to extend it with ACPI support. But in mlxreg-core I am using DTS, which is not supported for x86. Here I am using the next functions for probing and removing hotplug entities: static void mlxreg_core_dev_enable(struct device_node *np) { /* Enable and create device. */ of_update_property(np, &mlxreg_core_device_en); } static void mlxreg_core_dev_disable(struct device_node *np) { /* Disable and unregister platform device. */ of_update_property(np, &mlxreg_core_device_dis); of_node_clear_flag(np, OF_POPULATED); } And such code can't be placed under drivers/platform/x86/. Thanks, Vadim. > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
