Re: [RFC PATCH 2/8] regulator: Add Dialog DA906x voltage regulators support.

Re: [RFC PATCH 2/8] regulator: Add Dialog DA906x voltage regulators support.

[Mark Brown]

On Fri, Aug 24, 2012 at 02:55:00PM +0100, Krystian Garbaciak wrote:

> +static int da906x_set_voltage(struct regulator_dev *rdev,
> +				int min_uV, int max_uV, unsigned *selector)
> +{
> +	struct da906x_regulator *regl = rdev_get_drvdata(rdev);
> +	const struct field *fvol = &regl->info->voltage;
> +	int ret;
> +	unsigned val;
> +
> +	val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
> +	if (val < 0)
> +		return -EINVAL;
> +
> +	val = (val + fvol->offset) << fvol->shift;
> +	ret = da906x_reg_update(regl->hw, fvol->addr, fvol->mask, val);
> +	if (ret >= 0)
> +		*selector = val;
> +
> +	return ret;
> +}

This is just set_voltage_sel_regmap().

> +static int da906x_enable(struct regulator_dev *rdev)
> +{
> +	struct da906x_regulator *regl = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	if (regl->info->suspend.mask) {
> +		/* Make sure to exit from suspend mode on enable */
> +		ret = da906x_reg_clear_bits(regl->hw, regl->info->suspend.addr,
> +					    regl->info->suspend.mask);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* BUCKs need mode update after wake-up from suspend state. */
> +		ret = da906x_update_mode_internal(regl, SYS_STATE_NORMAL);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return regulator_enable_regmap(rdev);

If suspend_mask is optional the regulators using it should just use the
standard operation.

> +/* Regulator event handlers */
> +irqreturn_t da906x_ldo_lim_event(int irq, void *data)

By "event handler" you mean "interrupt"

> +	bits = da906x_reg_read(hw, DA906X_REG_STATUS_D);
> +	if (bits < 0)
> +		return IRQ_HANDLED;

If you fail to detect an interrupt you report that you handled one...?

> +	if (!da906x_pdata) {
> +		dev_err(&pdev->dev, "No platform init data supplied\n");
> +		return -ENODEV;
> +	}

Platform data should be totally optional.

> +	bcores_merged = (ret & DA906X_BCORE_MERGE) ? true : false;
> +	bmem_bio_merged = (ret & DA906X_BUCK_MERGE) ? true : false;

The use of the ternery operation here is even worse than normal, you can
assign the values directly.

Re: [PATCH 1/8] mfd: Add Dialog DA906x core driver.

[Mark Brown]

On Fri, Aug 24, 2012 at 02:50:00PM +0100, Krystian Garbaciak wrote:

> This is MFD module providing access to registers and interrupts of DA906x
> series PMIC. It is used by other functional modules, registered as MFD cells.
> Driver uses regmap with paging to access extended register list. Register map
> is divided into two pages, where the second page is used during initialisation.

Your selection of people to CC here appears both large and random...

> +inline unsigned int da906x_to_range_reg(u16 reg)
> +{
> +	return reg + DA906X_MAPPING_BASE;
> +}

I've no real idea what this stuff is all about, it at least needs some
comments somewhere.  The fact that you're just adding a constant offset
to all registers is at best odd.

> +	if (pdata->flags & DA906X_FLG_NO_CACHE)
> +		config = &da906x_no_cache_regmap_config;

No, why would anyone ever want this and why would this not apply to all
other drivers?

> +static const struct i2c_device_id da906x_i2c_id[] = {
> +	{"da906x", PMIC_DA9063},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, da906x_i2c_id);

List the actual devices here.

> +#define DA906X_IRQ_BASE_OFFSET	0

Hrm?

Re: [RFC PATCH 2/8] regulator: Add Dialog DA906x voltage regulators support.

[Krystian Garbaciak]

> > +static int da906x_set_voltage(struct regulator_dev *rdev,
> > +				int min_uV, int max_uV, unsigned *selector)
> > +{
> > +	struct da906x_regulator *regl = rdev_get_drvdata(rdev);
> > +	const struct field *fvol = &regl->info->voltage;
> > +	int ret;
> > +	unsigned val;
> > +
> > +	val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
> > +	if (val < 0)
> > +		return -EINVAL;
> > +
> > +	val = (val + fvol->offset) << fvol->shift;
> > +	ret = da906x_reg_update(regl->hw, fvol->addr, fvol->mask, val);
> > +	if (ret >= 0)
> > +		*selector = val;
> > +
> > +	return ret;
> > +}
> 
> This is just set_voltage_sel_regmap().

Because, for some regulators, this is required: val += fvol->offset,
I was only able to reduce it to the following form.

+static int da9063_set_voltage(struct regulator_dev *rdev,
+			      int min_uV, int max_uV, unsigned *selector)
+{
+	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
+	const struct field *fvol = &regl->info->voltage;
+	int ret;
+	unsigned val;
+
+	val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
+	if (val < 0)
+		return -EINVAL;
+
+	val += fvol->offset;
+
+	ret = regulator_set_voltage_sel_regmap(rdev, val);
+	if (ret >= 0)
+		*selector = val;
+
+	return ret;
+}

> > +static int da906x_enable(struct regulator_dev *rdev)
> > +{
> > +	struct da906x_regulator *regl = rdev_get_drvdata(rdev);
> > +	int ret;
> > +
> > +	if (regl->info->suspend.mask) {
> > +		/* Make sure to exit from suspend mode on enable */
> > +		ret = da906x_reg_clear_bits(regl->hw, regl->info->suspend.addr,
> > +					    regl->info->suspend.mask);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* BUCKs need mode update after wake-up from suspend state. */
> > +		ret = da906x_update_mode_internal(regl, SYS_STATE_NORMAL);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return regulator_enable_regmap(rdev);
> 
> If suspend_mask is optional the regulators using it should just use the
> standard operation.

I guess, you meant here to call regulator_enable_regmap() directly for
regulators NOT using suspend.mask. Then, I will do it.

> > +/* Regulator event handlers */
> > +irqreturn_t da906x_ldo_lim_event(int irq, void *data)
> 
> By "event handler" you mean "interrupt"

Yes. I think, I will update the comment line.

> > +	bits = da906x_reg_read(hw, DA906X_REG_STATUS_D);
> > +	if (bits < 0)
> > +		return IRQ_HANDLED;
> 
> If you fail to detect an interrupt you report that you handled one...?

For me there is no sensible return value for this case.
However, I consider changing the reaction on read failure by reporting event
for all regulators, that might cause this interrupt:
+	bits = da9063_reg_read(hw, DA9063_REG_STATUS_D);
+	if (bits < 0)
+		bits = ~0;

I will update the driver according to your remaining comments.

Thank you for your comments,
Krystian

Re: [RFC PATCH 2/8] regulator: Add Dialog DA906x voltage regulators support.

[Mark Brown]

On Wed, Aug 29, 2012 at 03:50:00PM +0100, Krystian Garbaciak wrote:

> Because, for some regulators, this is required: val += fvol->offset,
> I was only able to reduce it to the following form.

What on earth makes you say this?  The above is obviously linear.

Besides, you're missing several points here.  One is that you should be
using the framework features, another is that you should be implementing
_sel.

> > > +	bits = da906x_reg_read(hw, DA906X_REG_STATUS_D);
> > > +	if (bits < 0)
> > > +		return IRQ_HANDLED;

> > If you fail to detect an interrupt you report that you handled one...?

> For me there is no sensible return value for this case.

IRQ_NONE.

Re: [RFC PATCH 2/8] regulator: Add Dialog DA906x voltage regulators support.

[Krystian Garbaciak]

> On Wed, Aug 29, 2012 at 03:50:00PM +0100, Krystian Garbaciak wrote:
> 
> > Because, for some regulators, this is required: val += fvol->offset,
> > I was only able to reduce it to the following form.
> 
> What on earth makes you say this?  The above is obviously linear.
> 
> Besides, you're missing several points here.  One is that you should be
> using the framework features, another is that you should be implementing
> _sel.

Sorry, I've missed an obvious thing here. Instead of adding selector offset at
runtime, I can substract apropriate voltage from .min_uV. Thanks for pointing
this out.

Re: [PATCH 1/8] mfd: Add Dialog DA906x core driver.

[Krystian Garbaciak]

> On Fri, Aug 24, 2012 at 02:50:00PM +0100, Krystian Garbaciak wrote:
> 
> > This is MFD module providing access to registers and interrupts of DA906x
> > series PMIC. It is used by other functional modules, registered as MFD cells.
> > Driver uses regmap with paging to access extended register list. Register map
> > is divided into two pages, where the second page is used during initialisation.
> 
> Your selection of people to CC here appears both large and random...

I've added any maintainer for my modules from maintainer list.

> > +inline unsigned int da906x_to_range_reg(u16 reg)
> > +{
> > +	return reg + DA906X_MAPPING_BASE;
> > +}
> 
> I've no real idea what this stuff is all about, it at least needs some
> comments somewhere.  The fact that you're just adding a constant offset
> to all registers is at best odd.

I will comment it precisely for next version:

+/* Adding virtual register range starting from address DA9063_MAPPING_BASE.
+   It will be used for registers requiring page switching, which in our case
+   are virtually all PMIC registers.
+   Registers from 0 to 255 are used only as a page window and are volatile,
+   except DA9063_REG_PAGE_CON register (page selector), which is cachable. */
+static const struct regmap_range_cfg da9063_range_cfg[] = {
+	{
+		.range_min = DA9063_MAPPING_BASE,
+		.range_max = DA9063_MAPPING_BASE +
+			     ARRAY_SIZE(da9063_reg_flg) - 1,
+		.selector_reg = DA9063_REG_PAGE_CON,
+		.selector_mask = 1 << DA9063_I2C_PAGE_SEL_SHIFT,
+		.selector_shift = DA9063_I2C_PAGE_SEL_SHIFT,
+		.window_start = 0,
+		.window_len = 256,
+	}
+};

.. and here:

+/* Access to any PMIC register is passed through virtual register range,
+   starting at DA9063_MAPPING_BASE. For those registers, paging is required. */
+inline unsigned int da9063_to_range_reg(u16 reg)
+{
+	return reg + DA9063_MAPPING_BASE;
+}

Re: [PATCH 1/8] mfd: Add Dialog DA906x core driver.

[Philippe Rétornaz]

Le vendredi 31 août 2012 12:20:00 Krystian Garbaciak a écrit :
> > On Fri, Aug 24, 2012 at 02:50:00PM +0100, Krystian Garbaciak wrote:
> > > This is MFD module providing access to registers and interrupts of
> > > DA906x
> > > series PMIC. It is used by other functional modules, registered as MFD
> > > cells. Driver uses regmap with paging to access extended register list.
> > > Register map is divided into two pages, where the second page is used
> > > during initialisation.> 
> > Your selection of people to CC here appears both large and random...
> 
> I've added any maintainer for my modules from maintainer list.

You should filter manually what get_maintainer.pl tells you, sometimes it's 
too verbose.

I'm not maintainer of any of the files/tree this patch is touching.

Anyways, it's quite pleasing to see that get_maintainer knows me :)

Regards,

Philippe

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] mfd: Add Dialog DA906x core driver.

[Mark Brown]

On Fri, Aug 31, 2012 at 12:20:00PM +0100, Krystian Garbaciak wrote:
> > On Fri, Aug 24, 2012 at 02:50:00PM +0100, Krystian Garbaciak wrote:

> > Your selection of people to CC here appears both large and random...

> I've added any maintainer for my modules from maintainer list.

You don't need to CC every single persojn on every single patch, and
quite a few of these people are clearly not active in development.

> > > +inline unsigned int da906x_to_range_reg(u16 reg)
> > > +{
> > > +	return reg + DA906X_MAPPING_BASE;
> > > +}

> > I've no real idea what this stuff is all about, it at least needs some
> > comments somewhere.  The fact that you're just adding a constant offset
> > to all registers is at best odd.

> I will comment it precisely for next version:

This still makes very little sense - this function appears to be
accomplishing very little.  You're adding a constant offset to every
single register address that gets used.  Why are we doing this
dynamically at runtime?