Re: [patch v1 1/2] mfd: Add Mellanox regmap core driver

[Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3530 bytes --]

Hi!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mellanox,mlxreg-core
> @@ -0,0 +1,347 @@
> +Mellanox programmable device control.
> +-------------------------------------
> +This binding defines the device control interface over I2C bus for Mellanox BMC
> +based switches.

You'll need to cc dt maintainers here.

> +Required properties:
> +- compatible = "mellanox,mlxreg-i2c",
> +	       "mellanox,mlxreg-i2c-16",
> +	       "mellanox,mlxreg-core";
> +- #address-cells : must be 1;
> +- #size-cells : must be 0;
> +- reg : I2C address;

> +			cpu_watchdog {
> +				reg = <0x1d>;
> +				mask = <0xef>;
> +			};
> +
> +			cpu_kernel_panic {
> +				reg = <0x1d>;
> +				mask = <0xef>;
> +			};
> +			bmc_warm_reset {
> +				reg = <0x71>;
> +				mask = <0xfe>;
> +			};

Extra newline.

> +static ssize_t mlxreg_core_attr_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct mlxreg_core_priv_data *priv = dev_get_drvdata(dev);
> +	int index = to_sensor_dev_attr_2(attr)->index;
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	struct mlxreg_core_data *data;
> +	u32 regval = 0;
> +	int ret;
> +
> +	switch (nr) {
> +	case MLXREG_CORE_ATTR_PSU:
> +	case MLXREG_CORE_ATTR_FAN:
> +		/* Bit = 0 : item is functional. */
> +		ret = regmap_read(priv->regmap,
> +				  priv->item[mlxreg_core_grp[nr].type]->reg,
> +				  &regval);
> +		if (ret)
> +			goto access_error;
> +
> +		regval = !!!(regval & BIT(mlxreg_core_get_pos(priv,
> +					  &mlxreg_core_grp[nr],

Uff. You really really mean it. But one ! should be enough.

> +static int
> +mlxreg_core_add_attr_group(struct mlxreg_core_priv_data *priv,
> +			   struct mlxreg_core_grp *grp, int id)
> +{
> +	struct mlxreg_core_data *data = priv->item[grp->type]->data;
> +	int i;
> +
> +	/* Skip group with negative access value. */
> +	if (grp->access < 0)
> +		return id;
> +
> +	priv->item[grp->type]->ind = id;
> +	for (i = 0; i < priv->item[grp->type]->count; i++, id++, data++) {
> +		priv->mlxreg_core_attr[id] =
> +				&priv->mlxreg_core_dev_attr[id].dev_attr.attr;
> +		if (grp->use_grp_name)
> +			priv->mlxreg_core_attr[id]->name =
> +				devm_kasprintf(priv->dev, GFP_KERNEL,
> +					       "%s%u", grp->name, id %
> +						priv->item[grp->type]->count
> +						+ 1);
> +		else
> +			priv->mlxreg_core_attr[id]->name =
> +				devm_kasprintf(priv->dev, GFP_KERNEL,
> +					       data->label);

Is it really neccessary to dynamically generate it? Static table could
be simpler...

> +static void
> +mlxreg_core_work_helper(struct mlxreg_core_priv_data *priv,
> +			struct mlxreg_core_item *item, u8 is_inverse,
> +			u16 off, u32 mask, u32 *cache)
> +{
> +	struct mlxreg_core_data *data;
> +	u32 asserted, regval, bit;
> +	int ret;
> +
> +	/*
> +	 * Validate if item related to received signal type is valid.
> +	 * It should never happen, excepted the situation when some
> +	 * piece of hardware is broken. In such situation just produce
> +	 * error message and return. Caller must continue to handle the
> +	 * signals from other devices if any.
> +	 */
> +	if (unlikely(!item)) {
> +		dev_err(priv->dev, "False signal is received: register at offset 0x%02x, mask 0x%02x.\n",
> +			off, mask);

Slightly too long line.

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]