Messages in this thread | | | From | Luca Weiss <> | Subject | Re: [PATCH v2 2/2] leds: add sgm3140 driver | Date | Sun, 05 Apr 2020 20:45:14 +0200 |
| |
Hi Andy,
On Samstag, 4. April 2020 11:58:31 CEST Andy Shevchenko wrote: > On Mon, Mar 30, 2020 at 10:49 PM Luca Weiss <luca@z3ntu.xyz> wrote: > > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver. > > > > This device is controlled by two GPIO pins, one for enabling and the > > second one for switching between torch and flash mode. > > ... > > > +config LEDS_SGM3140 > > + tristate "LED support for the SGM3140" > > + depends on LEDS_CLASS_FLASH > > + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS > > > > + depends on OF > > depends on OF || COMPILE_TEST ? > But hold on... > > ... > > > +#include <linux/of.h> > > Perhaps switch this to property.h and replace OF with more generic > device property / fwnode API? >
I didn't find clear documentation on this, the functions in drivers/base/ property.c can be used instead of the of_* (device tree) functions?
As far as I can tell, the device_property_* functions are supposed to be used for simple "give me a property for this 'struct device*'" while the fwnode_* functions are used as generic equivalent of the of_* functions?
So in this case I can replace
struct device_node *child_node; child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
with
struct fwnode_handle *child_node; child_node = fwnode_get_next_available_child_node(pdev->dev.fwnode, NULL);
and then instead of
ret = of_property_read_u32(child_node, "flash-max-timeout-us", &priv->max_timeout);
use
ret = fwnode_property_read_u32(child_node, "flash-max-timeout-us", &priv->max_timeout);
and finally instead of
init_data.fwnode = of_fwnode_handle(child_node);
I can probably directly do
init_data.fwnode = child_node;
Does that sound correct?
> ... > > > +struct sgm3140 { > > + bool enabled; > > + struct gpio_desc *flash_gpio; > > + struct gpio_desc *enable_gpio; > > + struct regulator *vin_regulator; > > + > > + /* current timeout in us */ > > + u32 timeout; > > + /* maximum timeout in us */ > > + u32 max_timeout; > > + > > > > + struct led_classdev_flash fled_cdev; > > I guess it might be slightly better to make it first member of the > struct (I didn't check but the rationale is to put more often used > members at the beginning to utilize cachelines). > > > + struct v4l2_flash *v4l2_flash; > > + > > + struct timer_list powerdown_timer; > > +}; > > ... > > > +static struct sgm3140 *flcdev_to_sgm3140(struct led_classdev_flash > > *flcdev) +{ > > + return container_of(flcdev, struct sgm3140, fled_cdev); > > +} > > ...and this becomes a no-op AFAICS (doesn't mean you need to remove it). > > ... > > > + struct device_node *child_node; > > > > + child_node = of_get_next_available_child(pdev->dev.of_node, NULL); > > > > + ret = of_property_read_u32(child_node, "flash-max-timeout-us", > > + &priv->max_timeout); > > > > + init_data.fwnode = of_fwnode_handle(child_node); > > > > + of_node_put(child_node); > > Device property / fwnode API? > > -- > With Best Regards, > Andy Shevchenko
Regards Luca
| |