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

[Pengyu Luo <mitltlatltl@gmail.com>]

On Sun, Dec 29, 2024 at 11:33 PM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> On Sat, 28 Dec 2024, Pengyu Luo wrote:
> > On Sat, Dec 28, 2024 at 8:33 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
> > > On 27/12/2024 17:13, Pengyu Luo wrote:
> > > > There are 3 variants, Huawei released first 2 at the same time.
> > >
> > > There are three variants of which Huawei released the first two
> > > simultaneously.

[skipped]

> > > > +/* Thermal Zone */
> > > > +/* Range from 0 to 0x2C, partial valid */
> > > > +static const u8 temp_reg[] = {0x05, 0x07, 0x08, 0x0E, 0x0F, 0x12, 0x15, 0x1E,
> > > > +                           0x1F, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26,
> > > > +                           0x27, 0x28, 0x29, 0x2A};
> > > > +
> > > > +int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM])
> > >
> > > int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 *temp, size_t
> > > temp_reg_num)
> > >
> > >
> > > > +{
> > > > +     /* GTMP */
> > > > +     u8 req[REQ_HDR_SIZE] = {0x02, 0x61, 1,};
> > > > +     u8 resp[RESP_HDR_SIZE + sizeof(s16)];
> > > > +     int ret, i = 0;
> > > > +
> > > > +     while (i < GAOKUN_TZ_REG_NUM) {
> > > while (i < temp_reg_num)
> > >
> >
> > It is a constant. But later, as Krzysztof suggested, I will use interfaces
> > from hwmon, then reading one at a time.
> >
> > > > +             req[INPUT_DATA_OFFSET] = temp_reg[i];
> > > > +             ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> > > > +             if (ret)
> > > > +                     return -EIO;
> > > > +             temp[i++] = *(s16 *)(resp + DATA_OFFSET);
> > >
> > > What's the point of the casting here ?
> > >
> > > memcpy(temp, resp, sizeof(s16));
> > > temp++;
> >
> > A 2Bytes symbol number in little endian, ec return it like this, so
> > casting.
>
> You should use __le16 and proper endianess conversion function then.
>

Agree

> It's bit confusing that in the declaration you used RESP_HDR_SIZE and here
> you do it with DATA_OFFSET instead. It feels DATA_OFFSET is unnecessary
> duplicate of RESP_HDR_SIZE and will easily lead confusing variation such
> as above.
>

I totally agree with you, it is duplicated.
In declaration, u8 resp[RESP_HDR_SIZE]; RESP_HDR_SIZE indicates the size.
When assigning, val = resp[DATA_OFFSET]; let us know we are extracting a
data from a response without thinking, so I added an alias. Removing it
is also fine for me.

Best wishes,
Pengyu