Re: [PATCH v5 1/1] platform/chrome: add a driver for HPS

[Dan Callaghan]

Excerpts from Tzung-Bi Shih’s message of 2022-10-12 19:46:51 +1100:
> On Wed, Oct 12, 2022 at 03:09:18PM +1100, Dan Callaghan wrote:
> > ---
>
> It doesn't need a cover letter if the series only has 1 patch in general.
> Instead, it could put additional information (and changelogs) after "---".

Understood, I'll omit the cover letter in future postings.

> > diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
> [...]
> > +static int hps_i2c_probe(struct i2c_client *client)
> > +{
> > +     struct hps_drvdata *hps;
> > +     int ret;
> > +
> > +     hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL);
> > +     if (!hps)
> > +             return -ENOMEM;
> > +
> > +     memset(&hps->misc_device, 0, sizeof(hps->misc_device));
>
> The memset can be dropped.  `hps` is z-allocated.

I'll take this out.

> > +     hps->misc_device.parent = &client->dev;
> > +     hps->misc_device.minor = MISC_DYNAMIC_MINOR;
> > +     hps->misc_device.name = "cros-hps";
> > +     hps->misc_device.fops = &hps_fops;
> > +
> > +     i2c_set_clientdata(client, hps);
> > +     hps->client = client;
>
> To be neat, I would prefer to insert a blank line here.

Sure, will add one.

> > +     hps->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(hps->enable_gpio)) {
> > +             ret = PTR_ERR(hps->enable_gpio);
> > +             dev_err(&client->dev, "failed to get enable gpio: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = misc_register(&hps->misc_device);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to initialize misc device: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     hps_set_power(hps, false);
>
> IIUC, the GPIO will raise to HIGH in the first place, and then fall
> to LOW until here.  Is it an expected behavior?  How about gpiod_get()
> with GPIOD_OUT_LOW?

It might seem a little unusual, but it is intentional. The enable line is
already high when we enter the kernel from firmware. Acquiring the GPIO
line with GPIOD_OUT_HIGH preserves its existing state (high) in case later
steps fail.

We power off the periphal only once the driver is successfully bound and has
taken control of its power state.

> > +static int hps_i2c_remove(struct i2c_client *client)
> > +{
> > +     struct hps_drvdata *hps = i2c_get_clientdata(client);
> > +
> > +     pm_runtime_disable(&client->dev);
> > +     misc_deregister(&hps->misc_device);
> > +     hps_set_power(hps, true);
>
> Why does it need to raise the GPIO again when removing the device?

Similar to the above, we want to preserve the default power state
(i.e. powered on) whenever the driver is not bound to the device.

This behaviour made sense to us mainly because we were originally controlling
the peripheral entirely from userspace, so it was always powered on by default.

Do you think this behaviour is acceptable, or do we need to change it?

-- 
Dan Callaghan <dcallagh@xxxxxxxxxxxx>
Software Engineer, Google