linux-kernel - Re: [PATCH 2/2] regulator: Add support for MAX77686.

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]

Date:	Wed, 09 May 2012 21:54:14 +0200
From:	Sylwester Nawrocki <snjw23@...il.com>
To:	Yadwinder Singh <yadi.brar@...sung.com>
CC:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH 2/2] regulator: Add support for MAX77686.

Hi,

just a few nitpicks...

On 05/09/2012 06:24 PM, Yadwinder Singh wrote:
> From: Yadwinder Singh Brar<yadi.brar@...sung.com>
>
> Add support for PMIC/regulator portion of MAX77686 multifunction device.
> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driver
> which supports setting and getting the voltage of a regulator with I2C
> interface.
>
> Signed-off-by: Yadwinder Singh Brar<yadi.brar@...sung.com>
...
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> new file mode 100644
> index 0000000..4aa9722
> --- /dev/null
> +++ b/drivers/regulator/max77686.c
> @@ -0,0 +1,660 @@
> +/*
> + * max77686.c - Regulator driver for the Maxim 77686
> + *
> + * Copyright (C) 2012 Samsung Electronics

I believe this should read:

   + * Copyright (C) 2012 Samsung Electronics Co., Ltd.

In patch 1/2 the copyright notice is also not exactly correct.

> + * Chiwoong Byun<woong.byun@...sung.com>

s/smasung/samsung

...
> +static int max77686_get_enable_register(struct regulator_dev *rdev,
> +					int *reg, int *mask, int *pattern)
> +{
> +	int rid = rdev_get_id(rdev);
> +
> +	switch (rid) {
> +	case MAX77686_LDO1...MAX77686_LDO26:
> +		*reg = MAX77686_REG_LDO1CTRL1 + (rid - MAX77686_LDO1);
> +		*mask = 0xC0;
> +		*pattern = 0xC0;

What about using lower case for all hex numbers ?

...
> +static int max77686_get_voltage_register(struct regulator_dev *rdev,
> +					 int *_reg, int *_shift, int *_mask)
> +{
...
> +	case MAX77686_BUCK2:
> +		reg = MAX77686_REG_BUCK2DVS1;
> +		mask = 0xff;
> +		break;
> +	case MAX77686_BUCK3:
> +		reg = MAX77686_REG_BUCK3DVS1;
> +		mask = 0xff;
> +		break;
> +	case MAX77686_BUCK4:
> +		reg = MAX77686_REG_BUCK4DVS1;
> +		mask = 0xff;
> +		break;
> +	case MAX77686_BUCK5...MAX77686_BUCK9:
> +		reg = MAX77686_REG_BUCK5OUT + (rid - MAX77686_BUCK5) * 2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*_reg = reg;
> +	*_shift = shift;
> +	*_mask = mask;
> +
> +	return 0;
> +}

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/