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