RE: [patch v2 2/2] drivers/platform/x86: add mlxcpld-hotplug driver registration to mlx-platform driver

[Vadim Pasternak]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@xxxxxxxxxxxxxxx]
> Sent: Thursday, October 27, 2016 9:25 PM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; dvhart@xxxxxxxxxxxxx;
> fengguang.wu@xxxxxxxxx
> Cc: davem@xxxxxxxxxxxxx; geert@xxxxxxxxxxxxxx; akpm@linux-
> foundation.org; kvalo@xxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> mchehab@xxxxxxxxxx; linux@xxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> platform-driver-x86@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx
> Subject: Re: [patch v2 2/2] drivers/platform/x86: add mlxcpld-hotplug driver
> registration to mlx-platform driver
> 
> Same comment regarding series (Use --thread to git send-email)
> 
> On Thu, 2016-10-27 at 19:55 +0000, Vadim Pasternak wrote:
> > Add calls for mlxcpld-hotplug platform driver
> > registration/unregistration and add platform hotplug data
> > configurations. This driver, when registered within system will handle
> > system hot-plug events for the power suppliers, power cables and fans
> > (insertion and removing). These events are controlled through CPLD
> > Lattice device.
> >
> > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> 
> > v1->v2:
> > ÂComments pointed out by Andy:
> > Â - Remove "from sender" from the message body;
> > Â - Add more information to commit message. (When CPLD spec will be
> > ÂÂÂÂavailable at Mellanox portal - MAINTAINERS Web-page info is to
> > ÂÂÂÂbe updated);
> > Â - Do not change include order.
> 
> Same comment. Put it below '---' line.
> 
> > ---
> >
> 
> > @@ -121,7 +123,87 @@ static struct i2c_mux_reg_platform_data
> > mlxplat_mux_data[] = {
> >
> > Â};
> >
> 
> > -static struct platform_device *mlxplat_dev;
> 
> Why did you move it? Seems like it's not needed in this patch.

Just by mistake.
Sorry.

> 
> > +/* Platform hotplug devices */
> > +static struct mlxcpld_hotplug_device mlxplat_mlxcpld_hotplug_psu[] =
> 
> I_really_like_long_names_of_variables. No, I don't.

I keep prefix mlxplat_mlxcpld for consistency.
Maybe I can remove " hotplug_" to make variable shorter?

> 
> > +static struct mlxcpld_hotplug_device mlxplat_mlxcpld_hotplug_pwr[] =
> > +static struct mlxcpld_hotplug_device mlxplat_mlxcpld_hotplug_fan[] =
> > +static struct mlxcpld_hotplug_platform_data
> > mlxplat_mlxcpld_hotplug_default_data = {
> 
> 
> > +	.top_aggr_offset = (MLXPLAT_CPLD_LPC_REG_BASE_ADRR | 0x3a),
> 
> | ?! Can you elaborate why it's ORred and not just added?
> 
> Besides that redundant parens.

Will remove parens and change "or" to + (no special reason).

> 
> > +	.top_aggr_mask = 0x48,
> > +	.top_aggr_psu_mask = 0x08,
> 
> And above explanation might shed a light why you are using interesting
> masks like 0b101.

Will add comments for masks (just 0x40 is masking in CPLD aggregation register PSU and PWR events, 0x08 - FAN events) for default system type.

> 
> > +	.psu_reg_offset = (MLXPLAT_CPLD_LPC_REG_BASE_ADRR | 0x58),
> > +	.psu_mask = 0x03,
> 
> GENMASK()?

OK, will change it to
.psu_mask = GENMASK(0, 1) and other appearances.

> 
> > +	.psu_count = ARRAY_SIZE(mlxplat_mlxcpld_hotplug_psu),
> > +	.psu = mlxplat_mlxcpld_hotplug_psu,
> > +	.top_aggr_pwr_mask = 0x08,
> > +	.pwr_reg_offset = (MLXPLAT_CPLD_LPC_REG_BASE_ADRR | 0x64),
> > +	.pwr_mask = 0x03,
> > +	.pwr_count = ARRAY_SIZE(mlxplat_mlxcpld_hotplug_pwr),
> > +	.pwr = mlxplat_mlxcpld_hotplug_pwr,
> > +	.top_aggr_fan_mask = 0x40,
> > +	.fan_reg_offset = (MLXPLAT_CPLD_LPC_REG_BASE_ADRR | 0x88),
> > +	.fan_mask = 0x0f,
> > +	.fan_count = ARRAY_SIZE(mlxplat_mlxcpld_hotplug_fan),
> > +	.fan = mlxplat_mlxcpld_hotplug_fan,
> 
> Wouldn't be cleaner like this:
> 
> struct hotplug_dev_pdata {
>  reg_offset;
>  mask;
>  count;
>  void *somethig;
> };
> 
> struct hotplug_pdata {
>  /* top stuff, will see later how to put it here */
> 
>  struct hotplug_dev_data psu;
>  struct hotplug_dev_data pwr;
>  struct hotplug_dev_data fan;
> };

I will consider this change for the next patch.

> 
> > +};
> > +
> > +/* Platform hotplug MSN21xx system family data */
> > +static
> > +struct mlxcpld_hotplug_platform_data
> > mlxplat_mlxcpld_hotplug_msn21xx_data = {
> > +	.top_aggr_offset = (MLXPLAT_CPLD_LPC_REG_BASE_ADRR | 0x3a),
> > +	.top_aggr_mask = 0x04,
> > +	.top_aggr_pwr_mask = 0x04,
> > +	.pwr_reg_offset = (MLXPLAT_CPLD_LPC_REG_BASE_ADRR | 0x64),
> > +	.pwr_mask = 0x03,
> > +	.pwr_count = ARRAY_SIZE(mlxplat_mlxcpld_hotplug_pwr),
> > +};
> > +
> > +static struct resource mlxplat_mlxcpld_hotplug_resources[] = {
> > +	[0] = DEFINE_RES_IRQ_NAMED(17, "mlxcpld-hotplug"),
> > +};
> > +
> > +struct platform_device *mlxplat_dev;
> > +struct mlxcpld_hotplug_platform_data *mlxplat_hotplug;
> >
> > Âstatic int __init mlxplat_dmi_default_matched(const struct
> > dmi_system_id *dmi)
> > Â{
> > @@ -132,6 +214,7 @@ static int __init
> > mlxplat_dmi_default_matched(const struct dmi_system_id *dmi)
> > Â		mlxplat_mux_data[i].n_values =
> > Â				ARRAY_SIZE(mlxplat_default_channels[i
> > ]);
> > Â	}
> 
> > +	mlxplat_hotplug = &mlxplat_mlxcpld_hotplug_default_data;
> 
> Is it only one hotplug device at a time? Otherwise it will be affected
> by race conditions.
> 

Only one CPLD per box, which handles all the events.

> >
> > Â	return 1;
> > Â};
> > @@ -145,6 +228,7 @@ static int __init
> > mlxplat_dmi_msn21xx_matched(const struct dmi_system_id *dmi)
> > Â		mlxplat_mux_data[i].n_values =
> > Â				ARRAY_SIZE(mlxplat_msn21xx_channels);
> > Â	}
> > +	mlxplat_hotplug = &mlxplat_mlxcpld_hotplug_msn21xx_data;
> >
> > Â	return 1;
> > Â};
> > @@ -230,6 +314,16 @@ static int __init mlxplat_init(void)
> > Â		}
> > Â	}
> >
> > +	priv->pdev_hotplug = platform_device_register_resndata(
> > +				&mlxplat_dev->dev, "mlxcpld-hotplug",
> 
> > -1,
> 
> Isn't it something like PLATFORM_DEVID_AUTO / NONE ?

Will change -1 to PLATFORM_DEVID_NONE.

> 
> > +				mlxplat_mlxcpld_hotplug_resources,
> > +				ARRAY_SIZE(mlxplat_mlxcpld_hotplug_re
> > sources),
> > +				mlxplat_hotplug,
> > sizeof(*mlxplat_hotplug));
> > +	if (IS_ERR(priv->pdev_hotplug)) {
> > +		err = PTR_ERR(priv->pdev_hotplug);
> > +		goto fail_platform_mux_register;
> > +	}
> > +
> > Â	return 0;
> >
> > Âfail_platform_mux_register:
> > @@ -248,6 +342,8 @@ static void __exit mlxplat_exit(void)
> > Â	struct mlxplat_priv *priv =
> > platform_get_drvdata(mlxplat_dev);
> > Â	int i;
> >
> > +	platform_device_unregister(priv->pdev_hotplug);
> > +
> > Â	for (i = ARRAY_SIZE(mlxplat_mux_data) - 1; i >= 0 ; i--)
> > Â		platform_device_unregister(priv->pdev_mux[i]);
> >
> 
> --
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Intel Finland Oy

Thank you for review,
Vadim.