2012-08-25 18:31:21

by Mark Brown

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

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?


2012-08-31 11:24:07

by Krystian Garbaciak

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

> 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;
+}

2012-08-31 11:43:59

by Philippe Rétornaz

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

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

2012-08-31 17:17:04

by Mark Brown

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

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?