Re: [PATCH v3 2/2] leds: pca995x: Add support for PCA995X chips

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

 



On 7/13/23 16:07, Lee Jones wrote:

[...]

+#define PCA995X_LDRX_MASK	0x3	/* 2 bits per output state control */
+
+#define PCA995X_MODE_1_CFG	0x00	/* Auto-increment disabled. Normal mode */

Is this not the reset value?

The chip may not be power-cycled on reboot, so this register is not necessarily 0x0.

+#define PCA995X_IREFALL_CFG	0x7F	/* Half of max current gain multiplier */

PCA995X_IREFALL_FULL_CFG	0xFF
PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_MAX_CFG / 2)

?

+#define PCA995X_MAX_OUTPUTS	16	/* Supported outputs */

The define nomenclature is clear enough.

+#define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
+
+struct pca995x_led {
+	unsigned int led_no;
+	struct led_classdev ldev;
+	struct pca995x_chip *chip;
+};
+
+struct pca995x_chip {
+	struct regmap *regmap;
+	struct pca995x_led leds[PCA995X_MAX_OUTPUTS];
+	int btype;
+};

It's weird to have 2 structs reference each other.

This should not be allowed:

pca995x_led->pca995x_chip->pca995x_led->pca995x_chip->pca995x_led->pca995x_chip

Some implementation of container_of() should do the trick.

container_of() wouldn't work here because struct pca995x_chip contains array of LEDs .

The structure layout is taken from:
8325642d2757e ("leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver")

+static int pca995x_brightness_set(struct led_classdev *led_cdev,
+				  enum led_brightness brightness)
+{
+	struct pca995x_led *led = ldev_to_led(led_cdev);
+	struct pca995x_chip *chip = led->chip;
+	u8 ledout_addr, pwmout_addr;
+	int shift, ret;
+
+	pwmout_addr = (chip->btype ? PCA9955B_PWM0 : PCA9952_PWM0) + led->led_no;

All this btype stuff is a little crazy and totally not scalable.

That's right, but until NXP changes the register layout again, this should be sufficient to support the entire 995x line up.

+	ledout_addr = PCA995X_LEDOUT0 + (led->led_no / 4);
+	shift = 2 * (led->led_no % 4);

Why 2 and 4?  I suggest that they are defined.

+	switch (brightness) {
+	case LED_FULL:

https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32

LED_HALF is just PWM (default) mode, set to half duty cycle .

+		return regmap_update_bits(chip->regmap, ledout_addr,
+					  PCA995X_LDRX_MASK << shift,
+					  PCA995X_LED_ON << shift);
+	case LED_OFF:
+		return regmap_update_bits(chip->regmap, ledout_addr,
+					  PCA995X_LDRX_MASK << shift, 0);
+	default:

Are there no invalid values here?

Not to my knowledge, this should be 0..255 clamped by ldev.max_brightness further down.

+	/* Disable LED all-call address and set normal mode */
+	ret = regmap_write(chip->regmap, PCA995X_MODE1, PCA995X_MODE_1_CFG);

The formatting of "MODE1" and "MODE_1" is making me twitch!

Is this how they are referenced in the datasheet?

It's called MODE1 in datasheet, fixed.

+	if (ret)
+		return ret;
+
+	/* IREF Output current value for all LEDn outputs */
+	return regmap_write(chip->regmap,
+			    btype ? PCA9955B_IREFALL : PCA9952_IREFALL,
+			    PCA995X_IREFALL_CFG);
+}
+
+static const struct i2c_device_id pca995x_id[] = {
+	{ "pca9952", .driver_data = (kernel_ulong_t)0 /* non-B chip */ },

Defines at least please.

Are you sure these are the only 2 types of chip this driver will
support?

No, I cannot predict the future or how NXP will or won't change the register layout of it. When it cames, we can deal with that.

[...]

The rest is addressed in V4.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux