RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak <vadimp@nvidia.com>]

> -----Original Message-----
> From: Pavel Machek <pavel@ucw.cz>
> Sent: Wednesday, October 21, 2020 11:34 AM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Marek Behun <marek.behun@nic.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> Hi!
> 
> > > > But why you consider it as function enumerator?
> > > > For example card48, card56 are two different devices of same type.
> > > > Both have 'status' LED.
> > >
> > > OK this is a fair point.
> > >
> > > I was thinking such because in my mind I had this idea that for an
> > > ethernet switch with interfaces lan0 - lan4 it would make sense to
> > > use the LED_FUNCTION_LAN function with function enumerator. But
> > > thinking about this now again makes me wonder if instead the lan0 -
> > > lan4 should be devicenames instead, since normally they are network
> interface names.
> > >
> > > Vadim, the reason why Pavel and I think that mlxreg (or mlxregN) is
> > > not valid devicename part (although mlxreg has to stay since many
> > > users already depend on it, as you say), is that the mlxreg name is
> > > not exposed anywhere else in Linux from userspace point of view.
> > >
> > > Devicename eth0 is okay, because it is network interface name.
> > > Devicename sda would be okay, because everyone knows it is a block
> > > device and you can access it via /dev/sda.
> > > Devicename hci0 would be okay because it is bluetooth interface
> > > accessible via hcitool.
> > > Devicenames mtd0, kbd0, mouse0 would be okay, I think.
> > >
> > > But mlxreg is not accessible via anything else in the system. Unless
> > > your systems also have something like /dev/mlxreg, that is.
> > >
> > > Do the LEDs on these cards only indicate status of the cards
> > > themselves as a whole? Or are there LEDs on these cards dedicated to
> > > their peripherals? For example if there is an ethernet port with
> > > LEDs on one of these cards, the devicename part for these LEDs
> > > should be of the device of that ethernet port, not mlxreg...
> >
> > Hi Marek,
> >
> > Each line card must have 'status' LED, indicating status of line card itself.
> > User can set non-green in case some there are some alarms on different
> > devices, equipped on this line card. It can be set blink during line card
> initialization.
> >
> > Line card could be equipped with UID LED. User can set this LED in
> > order to find physical location of line card. Sometimes it's hard to
> > see the sticker on chassis.
> >
> > Line card also equipped with per port LED, but those LEDs are handled by
> FW.
> >
> > So, the device in this case is 'line card'.
> >
> > In my previous reply I suggest name 'fru' stands for the filed replaceable unit.
> > This is not something, that is exposed in '/dev', but it describes any
> > replaceable unit within the system.
> 
> So.. you'd use the LED to locate right PCI card, or the LED would indicate that
> whole card is failing, etc...?
> 
> Could we use pci00:1b.0 as the device name? (same as lspci). Probably replace
> : with _...
> 

Hi Pavel,

Yes, STATU and UID LED indicates whole line card status/location.

Some line cards could be connected through PCIe, but not all.
From chassis management perspective they are connected by I2C.
And CPLD register map is accessed through I2C.

Following your suggestion it could be i2c-{n} as device name (pdev->id is I2C bus):

 		if (priv->pdev->id > 0)
 			sprintf(led_data->led_cdev_name, "%s%d:%s", "i2c-",
 				priv->pdev->id, data->label);
 		else
 			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
 				data->label);

Or bus name could be passed through the 'identity' filed:
		if (priv->pdev->id > 0)
			sprintf(led_data->led_cdev_name, "%s%d:%s", led_pdata->identity,
				priv->pdev->id, data->label);
		else
			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
				data->label);

Thanks,
Vadim.

> Best regards,
> 								Pavel
> --
> http://www.livejournal.com/~pavelmachek