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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130814152750.GS6427@sirena.org.uk>
Date:	Wed, 14 Aug 2013 16:27:50 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Florian Lobmaier <florian.lobmaier@....com>
Cc:	linux-kernel@...r.kernel.org, sameo@...ux.intel.com,
	lee.jones@...aro.org
Subject: Re: [PATCH 1/4] added support for ams AS3722 PMIC in mfd

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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)