RE: [patch v2 1/2] mfd: Add Mellanox regmap core driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





> -----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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux