Messages in this thread | | | Subject | Re: [PATCH V5 1/6] drivers/mfd: sensehat: Raspberry Pi Sense HAT core driver | From | Nicolas Saenz Julienne <> | Date | Mon, 13 Dec 2021 12:49:48 +0100 |
| |
Hi Charles,
On Fri, 2021-12-10 at 17:10 -0500, Charles Mirabile wrote: > This patch adds the core driver file, containing the regmap configuration > needed to communicate with the board over I2C. We also add the header > file shared by all three drivers, containing common data and definitions. > In addition, we add a config option to toggle compilation of the driver. > > Signed-off-by: Charles Mirabile <cmirabil@redhat.com> > Co-developed-by: Mwesigwa Guma <mguma@redhat.com> > Signed-off-by: Mwesigwa Guma <mguma@redhat.com> > Co-developed-by: Joel Savitz <jsavitz@redhat.com> > Signed-off-by: Joel Savitz <jsavitz@redhat.com> > --- > drivers/mfd/Kconfig | 8 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/sensehat-core.c | 157 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/sensehat.h | 51 ++++++++++++ > 4 files changed, 217 insertions(+) > create mode 100644 drivers/mfd/sensehat-core.c > create mode 100644 include/linux/mfd/sensehat.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 3fb480818599..e6de22f98c0e 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -11,6 +11,14 @@ config MFD_CORE > select IRQ_DOMAIN > default n > > +config MFD_SENSEHAT_CORE > + tristate "Raspberry Pi Sense HAT core functions" > + depends on I2C > + select MFD_CORE > + help > + This is the core driver for the Raspberry Pi Sense HAT. This provides > + the necessary functions to communicate with the hardware. > + > config MFD_CS5535 > tristate "AMD CS5535 and CS5536 southbridge core functions" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 0b1b629aef3e..2b012e3d497d 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -263,6 +263,7 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > obj-$(CONFIG_MFD_ROHM_BD957XMUF) += rohm-bd9576.o > obj-$(CONFIG_MFD_STMFX) += stmfx.o > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > +obj-$(CONFIG_MFD_SENSEHAT_CORE) += sensehat-core.o > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o > obj-$(CONFIG_MFD_QCOM_PM8008) += qcom-pm8008.o > > diff --git a/drivers/mfd/sensehat-core.c b/drivers/mfd/sensehat-core.c > new file mode 100644 > index 000000000000..c5b6f4648d88 > --- /dev/null > +++ b/drivers/mfd/sensehat-core.c > @@ -0,0 +1,157 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Raspberry Pi Sense HAT core driver > + * http://raspberrypi.org > + * > + * Copyright (C) 2015 Raspberry Pi > + * Copyright (C) 2021 Charles Mirabile, Mwesigwa Guma, Joel Savitz > + * > + * Original Author: Serge Schneider > + * Revised for upstream Linux by: Charles Mirabile, Mwesigwa Guma, Joel Savitz > + * > + * This driver is based on wm8350 implementation and was refactored to use the > + * misc device subsystem rather than the deprecated framebuffer subsystem. > + */ > + > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/i2c.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/regmap.h> > +#include <linux/mfd/sensehat.h> > + > +#define SENSEHAT_WAI 0xF0 > +#define SENSEHAT_VER 0xF1 > + > +#define SENSEHAT_ID 's' > + > +static struct platform_device * > +sensehat_client_dev_register(struct sensehat *sensehat, const char *name); > + > +static struct regmap_config sensehat_config; > + > +static int sensehat_probe(struct i2c_client *i2c, > + const struct i2c_device_id *id) > +{ > + int ret; > + unsigned int reg; > + > + struct sensehat *sensehat = > + devm_kzalloc(&i2c->dev, sizeof(*sensehat), GFP_KERNEL); > + > + if (!sensehat) > + return -ENOMEM; > + > + i2c_set_clientdata(i2c, sensehat); > + sensehat->dev = &i2c->dev; > + sensehat->i2c_client = i2c; > + > + sensehat->regmap = > + devm_regmap_init_i2c(sensehat->i2c_client, &sensehat_config); > + > + if (IS_ERR(sensehat->regmap)) { > + dev_err(sensehat->dev, "Failed to initialize sensehat regmap"); > + return PTR_ERR(sensehat->regmap); > + } > + > + ret = regmap_read(sensehat->regmap, SENSEHAT_WAI, ®); > + if (ret < 0) { > + dev_err(sensehat->dev, "failed to read from device"); > + return ret; > + } > + > + if (reg != SENSEHAT_ID) { > + dev_err(sensehat->dev, "expected device ID %i, got %i", > + SENSEHAT_ID, ret); > + return -EINVAL; > + } > + > + ret = regmap_read(sensehat->regmap, SENSEHAT_VER, ®); > + if (ret < 0) { > + dev_err(sensehat->dev, > + "Unable to get sensehat firmware version"); > + return ret; > + } > + > + dev_info(sensehat->dev, "Raspberry Pi Sense HAT firmware version %i\n", > + reg);
I think both these sections and sensehat_client_dev_register() could be avoided with a properly configured struct mfd_cell and a call to devm_mfd_add_devices(). Actually, you're not making use of the mfd subsystem at all in this driver, which is odd. For an example see 'drivers/mfd/mp2629.c'.
It's easy to get all these device registration operations wrong, so it's always perfered to use the subsytem facilities when available.
> + > +#ifdef CONFIG_JOYSTICK_SENSEHAT > + sensehat->joystick.pdev = > + sensehat_client_dev_register(sensehat, "sensehat-joystick"); > + > + if (IS_ERR(sensehat->joystick.pdev)) { > + dev_err(sensehat->dev, "failed to register sensehat-joystick"); > + return PTR_ERR(sensehat->joystick.pdev); > + } > +#endif > +#ifdef CONFIG_SENSEHAT_DISPLAY > + > + sensehat->display.pdev = > + sensehat_client_dev_register(sensehat, "sensehat-display"); > + > + if (IS_ERR(sensehat->display.pdev)) { > + dev_err(sensehat->dev, "failed to register sensehat-display"); > + return PTR_ERR(sensehat->display.pdev); > + } > +#endif > + > + return 0; > +} > + > +static struct platform_device * > +sensehat_client_dev_register(struct sensehat *sensehat, const char *name) > +{ > + long ret = -ENOMEM; > + struct platform_device *pdev = > + platform_device_alloc(name, PLATFORM_DEVID_AUTO); > + > + if (!pdev) > + goto alloc_fail; > + > + pdev->dev.parent = sensehat->dev; > + platform_set_drvdata(pdev, sensehat); > + > + ret = platform_device_add(pdev); > + if (ret) > + goto add_fail; > + > + ret = devm_add_action_or_reset( > + sensehat->dev, (void *)platform_device_unregister, pdev); > + if (ret) > + goto alloc_fail; > + > + return pdev; > + > +add_fail: > + platform_device_put(pdev); > +alloc_fail: > + return ERR_PTR(ret); > +} > + > +static struct regmap_config sensehat_config = { > + .name = "sensehat", > + .reg_bits = 8, > + .val_bits = 8, > +}; > + > +static const struct i2c_device_id sensehat_i2c_id[] = { > + { .name = "sensehat" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, sensehat_i2c_id); > + > +static struct i2c_driver sensehat_driver = { > + .driver = { .name = "sensehat" }, > + .probe = sensehat_probe, > + .id_table = sensehat_i2c_id, > +}; > + > +module_i2c_driver(sensehat_driver); > + > +MODULE_DESCRIPTION("Raspberry Pi Sense HAT core driver"); > +MODULE_AUTHOR("Serge Schneider <serge@raspberrypi.org>"); > +MODULE_LICENSE("GPL");
-- Nicolás Sáenz
| |