Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kestrel/leds/trigger: implement led trigger driver for group pwm #23

Open
wants to merge 1 commit into
base: cp-release/lf-5.15.y
Choose a base branch
from
Open

kestrel/leds/trigger: implement led trigger driver for group pwm #23

wants to merge 1 commit into from

Conversation

amiteshsingh-cpi
Copy link

@amiteshsingh-cpi amiteshsingh-cpi commented May 23, 2023 •
edited
Loading

pca9633 i2c leds driver supports group pwm which currently not utilized
by leds-pca963x driver.
This patch exposes various registers values via sysfs to user and can be
used to achieve group blinking and dimming.

Signed-off-by: Amitesh Singh [email protected]

@amiteshsingh-cpi amiteshsingh-cpi changed the base branch from master to cp-release/lf-5.15.y May 23, 2023 18:46
@amiteshsingh-cpi amiteshsingh-cpi changed the title Ami/led driver kestrel/leds/trigger: implement led trigger driver for group pwm May 23, 2023
@amiteshsingh-cpi
Copy link
Author

amiteshsingh-cpi commented May 23, 2023 •
edited
Loading

I would eventually generate a patch out of this commit and put inside

kcb_ocpp/patches2/linux

Currently, I don't want to touch kcb.

dkori-cpi
dkori-cpi reviewed May 24, 2023
View reviewed changes
Copy link

@dkori-cpi dkori-cpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this patch is extending the capability of pca9633. I already see existing driver has capability to do so ("nxp,hw-blink"). Not sure if something is missing there or not.

Moreover, this is not trigger per say, it is just extension of hardware capability into user space. There are other example driver which has capability to do so and I am sure you must have already seen Documentation/leds.

For macaw/kestrel, kernel changes are done through patch.

@amiteshsingh-cpi
Copy link
Author

amiteshsingh-cpi commented May 24, 2023 •
edited
Loading

I think this patch is extending the capability of pca9633. I already see existing driver has capability to do so ("nxp,hw-blink"). Not sure if something is missing there or not.

nxp,blink implements blinking per led port. The upstream driver for pca9633 does not implement group pwm blinking and dimming.

Moreover, this is not trigger per say, it is just extension of hardware capability into user space. There are other example driver which has capability to do so and I am sure you must have already seen Documentation/leds.

I'm all ears if you agree that we should implement group pwm in leds-pca963x.c itself. Eventually, I would like
to raise an upstream kernel patch (which implements group pwm) in leds-pca963x.c, but most of the kernel devs
are bit grumpy these days. :) And merging a patch in upstream kernel takes time. But yes, I wish to see a patch
in upstream kernel driver for this so that we don't have to maintain it whenever there is a kernel upgrades on CP products.

This patch is a first step towards utilizing full capabilities of pca9633.

For macaw/kestrel, kernel changes are done through patch.

Yes, I would create a patch and add into

kcb_ocpp/patches2/linux

This requires some changes in kcb_ocpp BR2 config for PATCH variable and this is for other day.
This git PR is a quick way for me to generate a kestrel image build.

@dkori-cpi
Copy link

I did not go through leds-pca963x.c completely but see some variable related to PCA963X_LED_GRP_PWM/ u8 grppwm.
I think best will be to create patch against this file for additional functionality. By looking new file name and kernel config, it looks like implementation is generic but that is not case.

May be @chardin-cpi or @aswathgajendran-cpi can comment more on it but I am hesitant to separate out from original NXP driver.

@amiteshsingh-cpi
Copy link
Author

amiteshsingh-cpi commented May 24, 2023 •
edited
Loading

I did not go through leds-pca963x.c completely but see some variable related to PCA963X_LED_GRP_PWM/ u8 grppwm. I think best will be to create patch against this file for additional functionality. By looking new file name and kernel config, it looks like implementation is generic but that is not case.

There is a reason why I went for a separate trigger implementation than implementing group pwm in leds-pca963x.c.
group pwm is basically a global setting for all LEDs and leds-pca963x is applicable to individual LEDs.
With grppwm trigger, I could just enable this trigger on a led (say red) and implement dimming/blinking by setting group pwm related sysfs files.

e.g.

echo grppwm > /sys/class/leds/pca963x:red/trigger

and for setting group pwm related stuffs, just add entries to /sys/class/leds/pca963x:red/grppwm|lrdbit....

May be @chardin-cpi or @aswathgajendran-cpi can comment more on it but I am hesitant to separate out from original NXP driver.

chardin-cpi
chardin-cpi reviewed May 25, 2023
View reviewed changes
Copy link

@chardin-cpi chardin-cpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amiteshsingh-cpi - work with @carlnorum-cpi on this - he will look into approving it.

I would just make sure this Kernel Normal Form to follow linux code style and maybe just see if it gets simplified

chardin-cpi
chardin-cpi reviewed May 25, 2023
View reviewed changes
drivers/leds/trigger/ledtrig-grppwm.c Outdated
ldev = dev_get_drvdata(dev);
grppwm_data = led_get_trigger_data(ldev);

return snprintf(buf, 2, "%d", grppwm_data->ldrstate);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buf is a user PAGE_SIZE so no need to truncate state if this is 1000 or some other weird value.

Suggested change
return snprintf(buf, 2, "%d", grppwm_data->ldrstate);
return sprintf(buf, "%d", grppwm_data->ldrstate);

drivers/leds/trigger/ledtrig-grppwm.c Outdated
ldev = dev_get_drvdata(dev);
grppwm_data = led_get_trigger_data(ldev);

return snprintf(buf, 2, "%d", grppwm_data->ldrstateall);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Suggested change
return snprintf(buf, 2, "%d", grppwm_data->ldrstateall);
return sprintf(buf, "%d", grppwm_data->ldrstateall);

drivers/leds/trigger/ledtrig-grppwm.c
Comment on lines +22 to +26
#define BIT_LDR3 6
#define BIT_LDR2 4
#define BIT_LDR1 2
#define BIT_LDR0 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are shifts - so, maybe reference to that

Suggested change
#define BIT_LDR3 6
#define BIT_LDR2 4
#define BIT_LDR1 2
#define BIT_LDR0 0
#define BITSHIFT_LDR3 6
#define BITSHIFT_LDR2 4
#define BITSHIFT_LDR1 2
#define BITSHIFT_LDR0 0

drivers/leds/trigger/ledtrig-grppwm.c
Comment on lines +60 to +79
/**
* Group duty cycle control
*/
#define REG_GRPPWM 0x06

/**
* Group frequency
*/
#define REG_GRPFREQ 0x07

/**
* LED output state
*/
#define REG_LEDOUT 0x08

/**
* Mode register 2
*/
#define REG_MODE2 0x01
Copy link

@chardin-cpi chardin-cpi May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Group duty cycle control
*/
#define REG_GRPPWM 0x06
/**
* Group frequency
*/
#define REG_GRPFREQ 0x07
/**
* LED output state
*/
#define REG_LEDOUT 0x08
/**
* Mode register 2
*/
#define REG_MODE2 0x01
#define REG_GRPPWM 0x06 /* group duty cycle */
#define REG_GRPFREQ 0x07 /* group frequency */
#define REG_LEDOUT 0x08 /* output state */
#define REG_MODE2 0x01 /* mode register */

drivers/leds/trigger/ledtrig-grppwm.c Outdated
ldev = dev_get_drvdata(dev);
grppwm_data = led_get_trigger_data(ldev);

return snprintf(buf, 4, "%d", grppwm_data->grppwm);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return snprintf(buf, 4, "%d", grppwm_data->grppwm);
return sprintf(buf, "%d", grppwm_data->grppwm);

drivers/leds/trigger/ledtrig-grppwm.c
return 0;
}
if (kstrtol(buf, 10, &val))
return -ENOMEM;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return -ENOMEM;
return -EINVAL;

drivers/leds/trigger/ledtrig-grppwm.c Outdated

if (!grppwm_data->i2c_client) {
pr_err("i2c client is not present");
return 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return 0;
return -ENODEV;

drivers/leds/trigger/ledtrig-grppwm.c Outdated
ldev = dev_get_drvdata(dev);
grppwm_data = led_get_trigger_data(ldev);

return snprintf(buf, 2, "%d", grppwm_data->grpctrlmode);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return snprintf(buf, 2, "%d", grppwm_data->grpctrlmode);
return sprintf(buf, "%d", grppwm_data->grpctrlmode);

drivers/leds/trigger/ledtrig-grppwm.c Outdated
ldev = dev_get_drvdata(dev);
grppwm_data = led_get_trigger_data(ldev);

return snprintf(buf, 4, "%d", grppwm_data->onoffratio_perc);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return snprintf(buf, 4, "%d", grppwm_data->onoffratio_perc);
return sprintf(buf, "%d", grppwm_data->onoffratio_perc);

@amiteshsingh-cpi amiteshsingh-cpi force-pushed the ami/led_driver branch 2 times, most recently from 262e274 to 4a30279 Compare May 27, 2023 13:55
@amiteshsingh-cpi
Copy link
Author

Thanks @chardin-cpi for the review. I tested this driver code during weekend and encountered following errors.

[May28 12:45] i2c i2c-4: Failed to register i2c client nxp,pca9633 at 0x62 (-16)

This is due to restriction, rightly so, set by kernel driver framework for i2c_new_client_device https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L913

I'm going to add an export function in leds_pca963x.c which returns i2c_client pointer.

@amiteshsingh-cpi
Copy link
Author

amiteshsingh-cpi commented May 28, 2023 •
edited
Loading

I shall take care of formatting and other review comments after I'm done with testing.
Hopeful to get the testing done today.
I do have to change my vimrc for kernel code formatting. It's been a while since I wrote a driver. The old mojo will be back soon. :)

@amiteshsingh-cpi amiteshsingh-cpi force-pushed the ami/led_driver branch 2 times, most recently from 36234f4 to a1733a7 Compare May 29, 2023 05:08
@amiteshsingh-cpi
kestrel/leds/trigger: implement led trigger driver for group pwm
ac955b2 
pca9633 i2c leds driver supports group pwm which currently not utilized
by leds-pca963x driver.
This patch exposes various registers values via sysfs to user and can be
used to achieve group blinking and dimming.

Signed-off-by: Amitesh Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chardin-cpi chardin-cpi chardin-cpi left review comments

@dkori-cpi dkori-cpi dkori-cpi left review comments

@ydutta-cpi ydutta-cpi Awaiting requested review from ydutta-cpi

@shubhamsharma-cpi shubhamsharma-cpi Awaiting requested review from shubhamsharma-cpi

@nakulchauhan-cpi nakulchauhan-cpi Awaiting requested review from nakulchauhan-cpi

@varinderbabbar-cpi varinderbabbar-cpi Awaiting requested review from varinderbabbar-cpi

@waseemkhan-cpi waseemkhan-cpi Awaiting requested review from waseemkhan-cpi

@carlnorum-cpi carlnorum-cpi Awaiting requested review from carlnorum-cpi

@bobwolff-cpi bobwolff-cpi Awaiting requested review from bobwolff-cpi

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amiteshsingh-cpi @dkori-cpi @chardin-cpi