| Message ID | 20200930014058.44460-5-luka.kovacic@sartura.hr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for the iEi Puzzle-M801 board | expand |
Hi! > +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct iei_wt61p803_puzzle_led *priv = cdev_to_iei_wt61p803_puzzle_led(cdev); > + unsigned char *resp_buf = priv->response_buffer; > + unsigned char led_power_cmd[5] = { > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > + IEI_WT61P803_PUZZLE_CMD_LED, > + IEI_WT61P803_PUZZLE_CMD_LED_POWER, > + (char)IEI_LED_OFF > + }; > + size_t reply_size; > + > + mutex_lock(&priv->lock); > + if (brightness == LED_OFF) { > + led_power_cmd[3] = (char)IEI_LED_OFF; > + priv->led_power_state = LED_OFF; > + } else { > + led_power_cmd[3] = (char)IEI_LED_ON; > + priv->led_power_state = LED_ON; > + } > + mutex_unlock(&priv->lock); > + > + return iei_wt61p803_puzzle_write_command(priv->mcu, led_power_cmd, > + sizeof(led_power_cmd), resp_buf, &reply_size); > +} Is the mutex needed? If so, should it include the iei_wt61p803_puzzle_write_command()? Does iei_wt61p803_puzzle_write_command() have internal locking to prevent two messages from being mingled? Best regards, Pavel
On Wed, Sep 30, 2020 at 9:48 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct led_classdev *cdev, > > + enum led_brightness brightness) > > +{ > > + struct iei_wt61p803_puzzle_led *priv = cdev_to_iei_wt61p803_puzzle_led(cdev); > > + unsigned char *resp_buf = priv->response_buffer; > > + unsigned char led_power_cmd[5] = { > > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > > + IEI_WT61P803_PUZZLE_CMD_LED, > > + IEI_WT61P803_PUZZLE_CMD_LED_POWER, > > + (char)IEI_LED_OFF > > + }; > > + size_t reply_size; > > + > > + mutex_lock(&priv->lock); > > + if (brightness == LED_OFF) { > > + led_power_cmd[3] = (char)IEI_LED_OFF; > > + priv->led_power_state = LED_OFF; > > + } else { > > + led_power_cmd[3] = (char)IEI_LED_ON; > > + priv->led_power_state = LED_ON; > > + } > > + mutex_unlock(&priv->lock); > > + > > + return iei_wt61p803_puzzle_write_command(priv->mcu, led_power_cmd, > > + sizeof(led_power_cmd), resp_buf, &reply_size); > > +} > > Is the mutex needed? If so, should it include the > iei_wt61p803_puzzle_write_command()? Does > iei_wt61p803_puzzle_write_command() have internal locking to prevent > two messages from being mingled? > > Best regards, > Pavel > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html Hello Pavel, The mutex isn't needed and can be removed. The function iei_wt61p803_puzzle_write_command() already handles its own mutex locking, so a separate mutex isn't required. Does brightness_set_blocking only block a single caller (each caller separately) or does it block all callers until the previous caller is finished? Kind regards, Luka
On Tue, Oct 6, 2020 at 8:41 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > On Wed, Sep 30, 2020 at 9:48 PM Pavel Machek <pavel@ucw.cz> wrote: > > > > Hi! > > > > > +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct led_classdev *cdev, > > > + enum led_brightness brightness) > > > +{ > > > + struct iei_wt61p803_puzzle_led *priv = cdev_to_iei_wt61p803_puzzle_led(cdev); > > > + unsigned char *resp_buf = priv->response_buffer; > > > + unsigned char led_power_cmd[5] = { > > > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > > > + IEI_WT61P803_PUZZLE_CMD_LED, > > > + IEI_WT61P803_PUZZLE_CMD_LED_POWER, > > > + (char)IEI_LED_OFF > > > + }; > > > + size_t reply_size; > > > + > > > + mutex_lock(&priv->lock); > > > + if (brightness == LED_OFF) { > > > + led_power_cmd[3] = (char)IEI_LED_OFF; > > > + priv->led_power_state = LED_OFF; > > > + } else { > > > + led_power_cmd[3] = (char)IEI_LED_ON; > > > + priv->led_power_state = LED_ON; > > > + } > > > + mutex_unlock(&priv->lock); > > > + > > > + return iei_wt61p803_puzzle_write_command(priv->mcu, led_power_cmd, > > > + sizeof(led_power_cmd), resp_buf, &reply_size); > > > +} > > > > Is the mutex needed? If so, should it include the > > iei_wt61p803_puzzle_write_command()? Does > > iei_wt61p803_puzzle_write_command() have internal locking to prevent > > two messages from being mingled? > > > > Best regards, > > Pavel > > > > -- > > (english) http://www.livejournal.com/~pavelmachek > > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > > Hello Pavel, > > The mutex isn't needed and can be removed. > The function iei_wt61p803_puzzle_write_command() already handles its own > mutex locking, so a separate mutex isn't required. > > Does brightness_set_blocking only block a single caller (each caller separately) > or does it block all callers until the previous caller is finished? > > Kind regards, > Luka Hello Pavel, I've sent out a new patchset, but I kept the mutex in use. It's used to make sure only one read or write request is done at once, when writing to the LED driver private structure. I have also added this description to the struct comment. Kind regards, Luka
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df24eae..8a25fb753dec 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -332,6 +332,14 @@ config LEDS_IPAQ_MICRO Choose this option if you want to use the notification LED on Compaq/HP iPAQ h3100 and h3600. +config LEDS_IEI_WT61P803_PUZZLE + tristate "LED Support for the iEi WT61P803 PUZZLE MCU" + depends on LEDS_CLASS + depends on MFD_IEI_WT61P803_PUZZLE + help + This option enables support for LEDs controlled by the iEi WT61P803 + M801 MCU. + config LEDS_HP6XX tristate "LED Support for the HP Jornada 6xx" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index c2c7d7ade0d0..cd362437fefd 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o obj-$(CONFIG_LEDS_INTEL_SS4200) += leds-ss4200.o obj-$(CONFIG_LEDS_IP30) += leds-ip30.o obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o +obj-$(CONFIG_LEDS_IEI_WT61P803_PUZZLE) += leds-iei-wt61p803-puzzle.o obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o diff --git a/drivers/leds/leds-iei-wt61p803-puzzle.c b/drivers/leds/leds-iei-wt61p803-puzzle.c new file mode 100644 index 000000000000..b5822a835116 --- /dev/null +++ b/drivers/leds/leds-iei-wt61p803-puzzle.c @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* iEi WT61P803 PUZZLE MCU LED Driver + * + * Copyright (C) 2020 Sartura Ltd. + * Author: Luka Kovacic <luka.kovacic@sartura.hr> + */ + +#include <linux/leds.h> +#include <linux/mfd/iei-wt61p803-puzzle.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/slab.h> + +enum iei_wt61p803_puzzle_led_state { + IEI_LED_OFF = 0x30, + IEI_LED_ON = 0x31, + IEI_LED_BLINK_5HZ = 0x32, + IEI_LED_BLINK_1HZ = 0x33, +}; + +/** + * struct iei_wt61p803_puzzle_led - MCU LED Driver + * + * @mcu: MCU struct pointer + * @response_buffer Global MCU response buffer allocation + * @lock: General mutex lock for LED operations + * @led_power_state: State of the front panel power LED + * @cdev: LED classdev + */ +struct iei_wt61p803_puzzle_led { + struct iei_wt61p803_puzzle *mcu; + unsigned char *response_buffer; + struct mutex lock; + int led_power_state; + struct led_classdev cdev; +}; + +static inline struct iei_wt61p803_puzzle_led *cdev_to_iei_wt61p803_puzzle_led + (struct led_classdev *led_cdev) +{ + return dev_get_drvdata(led_cdev->dev->parent); +} + +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct iei_wt61p803_puzzle_led *priv = cdev_to_iei_wt61p803_puzzle_led(cdev); + unsigned char *resp_buf = priv->response_buffer; + unsigned char led_power_cmd[5] = { + IEI_WT61P803_PUZZLE_CMD_HEADER_START, + IEI_WT61P803_PUZZLE_CMD_LED, + IEI_WT61P803_PUZZLE_CMD_LED_POWER, + (char)IEI_LED_OFF + }; + size_t reply_size; + + mutex_lock(&priv->lock); + if (brightness == LED_OFF) { + led_power_cmd[3] = (char)IEI_LED_OFF; + priv->led_power_state = LED_OFF; + } else { + led_power_cmd[3] = (char)IEI_LED_ON; + priv->led_power_state = LED_ON; + } + mutex_unlock(&priv->lock); + + return iei_wt61p803_puzzle_write_command(priv->mcu, led_power_cmd, + sizeof(led_power_cmd), resp_buf, &reply_size); +} + +static enum led_brightness +iei_wt61p803_puzzle_led_brightness_get(struct led_classdev *cdev) +{ + struct iei_wt61p803_puzzle_led *priv = + cdev_to_iei_wt61p803_puzzle_led(cdev); + int led_state; + + mutex_lock(&priv->lock); + led_state = priv->led_power_state; + mutex_unlock(&priv->lock); + + return led_state; +} + +static int iei_wt61p803_puzzle_led_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent); + struct iei_wt61p803_puzzle_led *priv; + struct led_init_data init_data = {}; + struct fwnode_handle *child; + int ret; + u32 reg; + + if (device_get_child_node_count(dev) != 1) + return -EINVAL; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->response_buffer = devm_kzalloc(dev, + IEI_WT61P803_PUZZLE_BUF_SIZE, GFP_KERNEL); + if (!priv->response_buffer) + return -ENOMEM; + + priv->mcu = mcu; + priv->led_power_state = 1; + mutex_init(&priv->lock); + dev_set_drvdata(dev, priv); + + child = device_get_next_child_node(dev, NULL); + + ret = fwnode_property_read_u32(child, "reg", ®); + if (ret || reg > 1) { + dev_err(dev, "Could not register 'reg' (%lu)\n", (unsigned long)reg); + ret = -EINVAL; + goto err_child_node; + } + + priv->cdev.brightness_set_blocking = iei_wt61p803_puzzle_led_brightness_set_blocking; + priv->cdev.brightness_get = iei_wt61p803_puzzle_led_brightness_get; + priv->cdev.max_brightness = 1; + init_data.fwnode = child; + + ret = devm_led_classdev_register_ext(dev, &priv->cdev, &init_data); + if (ret) { + dev_err(dev, "Could not register LED\n"); + goto err_child_node; + } + return 0; +err_child_node: + fwnode_handle_put(child); + return ret; +} + +static const struct of_device_id iei_wt61p803_puzzle_led_of_match[] = { + { .compatible = "iei,wt61p803-puzzle-leds" }, + { } +}; +MODULE_DEVICE_TABLE(of, iei_wt61p803_puzzle_led_of_match); + +static struct platform_driver iei_wt61p803_puzzle_led_driver = { + .driver = { + .name = "iei-wt61p803-puzzle-led", + .of_match_table = iei_wt61p803_puzzle_led_of_match, + }, + .probe = iei_wt61p803_puzzle_led_probe, +}; +module_platform_driver(iei_wt61p803_puzzle_led_driver); + +MODULE_DESCRIPTION("iEi WT61P803 PUZZLE front panel LED driver"); +MODULE_AUTHOR("Luka Kovacic <luka.kovacic@sartura.hr>"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:leds-iei-wt61p803-puzzle");
Add support for the iEi WT61P803 PUZZLE LED driver. Currently only the front panel power LED is supported. This driver depends on the iEi WT61P803 PUZZLE MFD driver. Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> Cc: Luka Perkov <luka.perkov@sartura.hr> Cc: Robert Marko <robert.marko@sartura.hr> --- drivers/leds/Kconfig | 8 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-iei-wt61p803-puzzle.c | 157 ++++++++++++++++++++++++ 3 files changed, 166 insertions(+) create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c