> -----Original Message----- > From: Pavel Machek [mailto:pavel@xxxxxx] > Sent: Wednesday, July 26, 2017 2:19 PM > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > Cc: lee.jones@xxxxxxxxxx; robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > j.anaszewski@xxxxxxxxxxx; rpurdie@xxxxxxxxx; linux- > leds@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx > Subject: Re: [patch v1 1/2] mfd: Add Mellanox regmap core driver > > 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. Hi Pavel, Thank you very much for review. I have in to: Rob Herring and in c: devicetree@xxxxxxxxxxxxxxx Should it be separate mail, or it's OK? > > > +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, > > + ®val); > > + 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... But I don't know how many groups and attributes can be defined for particular device. On the system I am working on know I have four programmable device with different purposes and different set of attributes. > > > +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
