Re: [PATCH v2 2/5] platform: arm64: add Huawei Matebook E Go EC driver

[Pengyu Luo <mitltlatltl@gmail.com>]

On Tue, Jan 7, 2025 at 8:25 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
>
> Hi,
>
> > +/* -------------------------------------------------------------------------- */
> > +/* API For UCSI */
> > +
> > +int gaokun_ec_ucsi_read(struct gaokun_ec *ec,
> > +                     u8 resp[GAOKUN_UCSI_READ_SIZE])
> > +{
> > +     u8 req[] = MKREQ(0x03, 0xD5, 0);
> > +     u8 _resp[] = MKRESP(GAOKUN_UCSI_READ_SIZE);
> > +     int ret;
> > +
> > +     ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
> > +     if (ret)
> > +             return ret;
> > +
> > +     extr_resp(resp, _resp, GAOKUN_UCSI_READ_SIZE);
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_read);
> > +
> > +int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
> > +                      const u8 req[GAOKUN_UCSI_WRITE_SIZE])
> > +{
> > +     u8 _req[] = MKREQ(0x03, 0xD4, GAOKUN_UCSI_WRITE_SIZE);
> > +
> > +
> > +     refill_req(_req, req, GAOKUN_UCSI_WRITE_SIZE);
> > +
> > +     return gaokun_ec_write(ec, _req);
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_write);
> > +
> > +int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec, u8 *ureg)
> > +{
> > +     u8 req[] = MKREQ(0x03, 0xD3, 0);
> > +     u8 _resp[] = MKRESP(UCSI_REG_SIZE);
> > +     int ret;
> > +
> > +     ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
> > +     if (ret)
> > +             return ret;
> > +
> > +     extr_resp(ureg, _resp, UCSI_REG_SIZE);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_get_reg);
>
> Why not just take struct gaokun_ucsi_reg as parameter? I did not see
> this (or any of these) being used anywhere else except in your UCSI
> glue driver. So the prototype would be:
>
> 	int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec,
> 				struct gaokun_ucsi_reg *reg);
>

Understood

> > +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id)
> > +{
> > +     u8 req[] = MKREQ(0x03, 0xD2, 1);
> > +     u8 data = 1 << port_id;
> > +
> > +     if (port_id == GAOKUN_UCSI_NO_PORT_UPDATE)
> > +             data = 0;
> > +
> > +     refill_req(req, &data, 1);
> > +
> > +     return gaokun_ec_write(ec, req);
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_pan_ack);
>
> I think you should add proper kernel doc comments to these exported
> functions.
>

Oh, I forgot some of them. gaokun_ec_ucsi_get_reg and
gaokun_ec_ucsi_pan_ack deserve kernel doc comments. Judging from the name,
others are obvious, should I comment all?

> thanks,
>
> --
> heikki

Best wishes,
Pengyu