Re: [PATCH 1/4] added support for ams AS3722 PMIC in mfd

[Mark Brown]

On Wed, Aug 14, 2013 at 04:54:55PM +0200, Florian Lobmaier wrote:

> +static struct regmap_irq_chip as3722_irq_chip = {
> +	.name = "as3722",
> +	.irqs = as3722_irqs,
> +	.num_irqs = ARRAY_SIZE(as3722_irqs),
> +	.num_regs = 4,
> +	.status_base = AS3722_INTERRUPTSTATUS1_REG,
> +	.mask_base = AS3722_INTERRUPTMASK1_REG,
> +	.wake_base = 1,
> +};

wake_base looks wrong - that should be either absent or a register name.

> +static void as3722_reg_init(struct as3722 *as3722,
> +		struct as3722_reg_init *reg_data)
> +{
> +	int ret;
> +
> +	while (reg_data->reg != AS3722_REG_INIT_TERMINATE) {
> +		ret = as3722_reg_write(as3722, reg_data->reg, reg_data->val);
> +		if (ret) {
> +			dev_err(as3722->dev,
> +					"reg setup failed: %d\n", ret);
> +			return;
> +		}
> +		reg_data++;
> +	}
> +}

This looks like it might be supposed to be a register patch?

> +int as3722_read_adc(struct as3722 *as3722,
> +		enum as3722_adc_channel channel,
> +		enum as3722_adc_source source,
> +		enum as3722_adc_voltange_range voltage_range)

This should be moved over to IIO - the ADC code in MFDs predates IIO
being available.

> +static irqreturn_t as3722_onkey_press_irq(int irq, void *irq_data)

> +static irqreturn_t as3722_onkey_lpress_irq(int irq, void *irq_data)

These should be handled by an input driver.

> +static irqreturn_t as3722_temp_sd0_shutdown_irq(int irq, void *irq_data)
> +{
> +	struct as3722 *as3722 = irq_data;
> +
> +	dev_dbg(as3722->dev, "AS3722 temp SD0 shutdown triggered\n");
> +	return IRQ_HANDLED;
> +}

You probably want these to complain loudly rather than as dev_dbg(), I'm
not sure what the status of the thermal framework is.

> +	/* enable 32kHz clock output if required */
> +	if (pdata->enable_clk32out_pin)
> +		as3722_set_bits(as3722, AS3722_RTC_CONTROL_REG,
> +				AS3722_CLK32OUT_ENABLE_MASK,
> +				AS3722_CLK32OUT_ENABLE_ON);
> +	else
> +		as3722_set_bits(as3722, AS3722_RTC_CONTROL_REG,
> +				AS3722_CLK32OUT_ENABLE_MASK,
> +				AS3722_CLK32OUT_ENABLE_OFF);

This looks like a job for the clk API.

Attachment: signature.asc
Description: Digital signature