Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





+static int pcm512x_gpio_get_direction(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pcm512x->regmap, PCM512x_GPIO_EN, &val);
+	if (ret < 0)
+		return ret;

+	val = (val >> offset) & 1;
+
+	/* val is 0 for input, 1 for output, return inverted */
+	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

This better to read as simple conditional, like

	if (val & BIT(offset))
		return ..._OUT;
	return ..._IN;

+}

ok


...

+static int pcm512x_gpio_direction_output(struct gpio_chip *chip,
+					 unsigned int offset,
+					 int value)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	unsigned int reg;
+	int ret;
+
+	/* select Register GPIOx output for OUTPUT_x (1..6) */
+	reg = PCM512x_GPIO_OUTPUT_1 + offset;

+	ret = regmap_update_bits(pcm512x->regmap, reg, 0x0f, 0x02);

Magic numbers detected.

+	if (ret < 0)

Drop unnecessary ' < 0' parts where it makes sense, like here.

did you mean use  if (ret) or drop the test altogether?

There's no standard style for regmap functions so I used what was used in the rest of this driver.

Mark?


+		return ret;
+

+	/* enable output x */

(1)

+	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
+				 BIT(offset), BIT(offset));
+	if (ret < 0)
+		return ret;
+
+	/* set value */

(2)

With this (1)->(2) ordering it may be a glitch. So, first set value (if
hardware allows you, otherwise it seems like a broken one), and then switch
it to output.

good suggestion, thanks.


+	return regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
+				  BIT(offset), value << offset);

You are using many times BIT(offset) mask above, perhaps
	int mask = BIT(offset);

Also, more robust is to use ternary here: 'value ? BIT(offset) : 0'.
Rationale: think what happen with value != 1 (theoretical possibility in the
future).

ok


+}

...

+static int pcm512x_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{

+	return (val >> offset) & 1;

Don't forget to use BIT() macro.

	return !!(val & BIT(offset));

There's a point where this becomes less readable IMHO, but fine.
The !! gives me a headache...

+static void pcm512x_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	int ret;
+
+	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
+				 BIT(offset), value << offset);

value ? BIT(offset) : 0

ok


+	if (ret < 0)

+		pr_debug("%s: regmap_update_bits failed: %d\n", __func__, ret);

No __func__ in debug messages.
Use dev_dbg() when we have struct device available.

Not sure we do, will look into this.

+static const struct gpio_chip template_chip = {

Give better name, please. E.g. pcm512x_gpio_chip.

ok

+	/* expose 6 GPIO pins, numbered from 1 to 6 */
+	pcm512x->chip = template_chip;
+	pcm512x->chip.parent = dev;
+
+	ret = devm_gpiochip_add_data(dev, &pcm512x->chip, pcm512x);

+	if (ret != 0) {

if (ret)

ok



[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux