On Mon, Dec 11, 2023 at 03:41:19PM +0800, Xinhu Wu wrote:
> +config TYPEC_SPRD_PMIC
> + tristate "SPRD Serials PMICs Typec Controller"
> + help
> + Say Y or M here if your system has a SPRD PMIC Type-C port controller.
> +
> + If you choose to build this driver as a dynamically linked module, the
> + module will be called sprd_pmic_typec.ko.
> + SPRD_PMIC_TYPEC notify usb, phy, charger, and analog audio to proceed
> + with work
I do not understand these last two lines, are you sure they are correct?
> +
> +
Nit, only one blank line is needed here.
> +static irqreturn_t sprd_pmic_typec_interrupt(int irq, void *data)
> +{
> + struct sprd_pmic_typec *sc = data;
> + u32 event;
> + int ret;
> +
> + dev_info(sc->dev, "%s enter line %d\n", __func__, __LINE__);
debugging information? Please remove.
> + ret = regmap_read(sc->regmap, sc->base + SC27XX_INT_MASK, &event);
> + if (ret)
> + return ret;
> +
> + event &= sc->var_data->event_mask;
> +
> + ret = regmap_read(sc->regmap, sc->base + SC27XX_STATUS, &sc->state);
> + if (ret)
> + goto clear_ints;
> +
> + sc->state &= sc->var_data->state_mask;
> +
> + if (event & SC27XX_ATTACH_INT) {
> + ret = sprd_pmic_typec_connect(sc, sc->state);
> + if (ret)
> + dev_warn(sc->dev, "failed to register partner\n");
> + } else if (event & SC27XX_DETACH_INT) {
> + sprd_pmic_typec_disconnect(sc, sc->state);
> + }
> +
> +clear_ints:
> + regmap_write(sc->regmap, sc->base + sc->var_data->int_clr, event);
> +
> + dev_info(sc->dev, "now works as DRP and is in %d state, event %d\n",
> + sc->state, event);
When drivers work properly, they are quiet, please never spam the kernel
log for normal operations.
> +static ssize_t
> +sprd_pmic_typec_cc_polarity_role_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct sprd_pmic_typec *sc = dev_get_drvdata(dev);
> +
> + return snprintf(buf, 5, "%s\n", sprd_pmic_typec_cc_polarity_roles[sc->cc_polarity]);
sysfs_emit() please.
> +}
> +static DEVICE_ATTR_RO(sprd_pmic_typec_cc_polarity_role);
Where is this new sysfs file documented?
> + ret = sysfs_create_groups(&sc->dev->kobj, sprd_pmic_typec_groups);
You just raced with userspace and lost, and better yet:
> + if (ret < 0)
> + dev_err(sc->dev, "failed to create cc_polarity %d\n", ret);
You do not even clean up properly here.
Please use the default groups for the driver and it should work just
fine.
> + ret = sprd_pmic_typec_set_rtrim(sc);
> + if (ret < 0) {
> + dev_err(sc->dev, "failed to set typec rtrim %d\n", ret);
> + goto error;
> + }
> +
> + ret = devm_request_threaded_irq(sc->dev, sc->irq, NULL,
> + sprd_pmic_typec_interrupt,
> + IRQF_EARLY_RESUME | IRQF_ONESHOT,
> + dev_name(sc->dev), sc);
Are you sure you can use devm_() here?
> +static int sprd_pmic_typec_remove(struct platform_device *pdev)
> +{
> + struct sprd_pmic_typec *sc = platform_get_drvdata(pdev);
> +
> + sysfs_remove_groups(&sc->dev->kobj, sprd_pmic_typec_groups);
Again, should not be needed if you use the default groups of the
platform driver.
thanks,
greg k-h
