Hello Again Lee,

After a good night sleep few things came to my mind =)

On Thu, Feb 07, 2019 at 02:00:53PM +0000, Lee Jones wrote:

On Thu, 07 Feb 2019, Matti Vaittinen wrote:

quoted

+
+static struct mfd_cell bd70528_mfd_cells[] = {
+	{ .name = "bd70528-pmic", },
+	{ .name = "bd70528-gpio", },
+	/*
+	 * We use BD71837 driver to drive the clk block. Only differences to
+	 * BD70528 clock gate are the register address and mask.
+	 */
+	{ .name = "bd718xx-clk", },
+	{ .name = "bd70528-wdt", },
+	{
+		.name = "bd70528-power",
+		.resources = &charger_irqs[0],
+		.num_resources = ARRAY_SIZE(charger_irqs),
+	},
+	{

These should be on the same line.

I know I said 'Ok' yesterday. And I can change the styling to what ever
suits you - but I am not entirely sure what you mean by this? Do you
mean that the brackets should be on same line? After a quick look to
few other MFD devices it seems to be common convention to have these on
separate lines - and such style is used also at other locations
throughout this file. 

quoted

+static const struct regmap_access_table volatile_regs = {
+	.yes_ranges = &volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(volatile_ranges),
+};
+
+static struct regmap_config bd70528_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &volatile_regs,
+	.max_register = BD70528_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};

'\n' here.

quoted

+/* bit [0] - Shutdown register */
+unsigned int bit0_offsets[] = {0};
+/* bit [1] - Power failure register */
+unsigned int bit1_offsets[] = {1};
+/* bit [2] - VR FAULT register */
+unsigned int bit2_offsets[] = {2};
+/* bit [3] - PMU register interrupts */
+unsigned int bit3_offsets[] = {3};
+/* bit [4] - Charger 1 and Charger 2 registers */
+unsigned int bit4_offsets[] = {4, 5};
+/* bit [5] - RTC register */
+unsigned int bit5_offsets[] = {6};
+/* bit [6] - GPIO register */
+unsigned int bit6_offsets[] = {7};
+/* bit [7] - Invalid operation register */
+unsigned int bit7_offsets[] = {8};

What on earth is this?

Would this comment help:
/*
 * Mapping of main IRQ register bits to sub irq register offsets so
 * that we can access corect sub IRQ registers based on bits that
 * are set in main IRQ register.
 */

/* bit [0] - Shutdown register */
unsigned int bit0_offsets[] = {0};
/* bit [1] - Power failure register */
unsigned int bit1_offsets[] = {1};
/* bit [2] - VR FAULT register */
unsigned int bit2_offsets[] = {2};
/* bit [3] - PMU register interrupts */
unsigned int bit3_offsets[] = {3};
/* bit [4] - Charger 1 and Charger 2 registers */
unsigned int bit4_offsets[] = {4, 5};
/* bit [5] - RTC register */
unsigned int bit5_offsets[] = {6};
/* bit [6] - GPIO register */
unsigned int bit6_offsets[] = {7};
/* bit [7] - Invalid operation register */
unsigned int bit7_offsets[] = {8};

quoted

+static int bd70528_wdt_set(struct bd70528 *bd70528, int enable, int *old_state)
+{

[..]

quoted

+}

Shouldn't this be one in the WDT driver?

Maybe I should explain it like this:

/*
 * Both the WDT and RTC drivers need to be able to control WDT. WDT uses
 * RTC for timeouts and setting the RTC may trigger watchdog. Thus the
 * RTC must disable the WDT when RTC time is set. We provide WDT disabling
 * code from the MFD parent as we don't want to make direct dependency
 * between RTC and WDT. Some may want to use only WDT or only RTC.
 */

#define WD_CTRL_MAGIC1 0x55
#define WD_CTRL_MAGIC2 0xAA

static int bd70528_wdt_set(struct bd70528 *bd70528, int enable, int *old_state)
{

quoted

+	/*
+	 * Disallow type setting for all IRQs by default as
+	 *  most of them do not support setting type.
+	 */
+	for (i = 0; i < ARRAY_SIZE(irqs); i++)
+		irqs[i].type.types_supported = 0;
+
+	irqs[BD70528_INT_GPIO0].type.type_reg_offset = 0;
+	irqs[BD70528_INT_GPIO0].type.type_rising_val = 0x20;
+	irqs[BD70528_INT_GPIO0].type.type_falling_val = 0x10;
+	irqs[BD70528_INT_GPIO0].type.type_level_high_val = 0x40;
+	irqs[BD70528_INT_GPIO0].type.type_level_low_val = 0x50;
+	irqs[BD70528_INT_GPIO0].type.types_supported = (IRQ_TYPE_EDGE_BOTH |
+				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
+	irqs[BD70528_INT_GPIO1].type.type_reg_offset = 2;
+	irqs[BD70528_INT_GPIO1].type.type_rising_val = 0x20;
+	irqs[BD70528_INT_GPIO1].type.type_falling_val = 0x10;
+	irqs[BD70528_INT_GPIO1].type.type_level_high_val = 0x40;
+	irqs[BD70528_INT_GPIO1].type.type_level_low_val = 0x50;
+	irqs[BD70528_INT_GPIO1].type.types_supported = (IRQ_TYPE_EDGE_BOTH |
+				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
+	irqs[BD70528_INT_GPIO2].type.type_reg_offset = 4;
+	irqs[BD70528_INT_GPIO2].type.type_rising_val = 0x20;
+	irqs[BD70528_INT_GPIO2].type.type_falling_val = 0x10;
+	irqs[BD70528_INT_GPIO2].type.type_level_high_val = 0x40;
+	irqs[BD70528_INT_GPIO2].type.type_level_low_val = 0x50;
+	irqs[BD70528_INT_GPIO2].type.types_supported = (IRQ_TYPE_EDGE_BOTH |
+				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
+	irqs[BD70528_INT_GPIO3].type.type_reg_offset = 6;
+	irqs[BD70528_INT_GPIO3].type.type_rising_val = 0x20;
+	irqs[BD70528_INT_GPIO3].type.type_falling_val = 0x10;
+	irqs[BD70528_INT_GPIO3].type.type_level_high_val = 0x40;
+	irqs[BD70528_INT_GPIO3].type.type_level_low_val = 0x50;
+	irqs[BD70528_INT_GPIO3].type.types_supported = (IRQ_TYPE_EDGE_BOTH |
+				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);

Could you please explain:

a) what you're doing here
b) why you don't mass assign them
    - seeing as most of the data is identical.

I am not sure this is what you meant by mass assignment? Something like
below?

I think this makes the code slightly more confusing yet much shorter.
What would you say? Is this what you had in mind?

        /*
         * Set IRQ typesetting information for GPIO pins 0 - 3
         */
        for (i = 0; i < 4; i++) {
                struct regmap_irq_type *type;

                type = &irqs[BD70528_INT_GPIO0 + i].type;
                type->type_reg_offset = 2 * i;
                type-<type_rising_val = 0x20;
                type->type_falling_val = 0x10;
                type->type_level_high_val = 0x40;
                type->type_level_low_val = 0x50;
                type->types_supported = (IRQ_TYPE_EDGE_BOTH |
                                IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
        }

Br,
	Matti Vaittinen

-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~