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

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

 





> -----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,
> > +				  &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...

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[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