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.
